Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle chain from menu (using Settings) #414

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ GUI changes
- UTXOs which are locked via the GUI are now stored persistently in the
wallet database, so are not lost on node shutdown or crash. (#23065)

- Toggle chain from mainnet to testnet, signet or regtest. The GUI now always sets `-chain`,
so any occurrences of `-testnet`, `-signet` or `-regtest` in `bitcoin.conf` should
be replace with `chain=test`, `chain=signet` or `chain=regtest` respectively. (bitcoin-core/gui#414)
Comment on lines +193 to +195
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: toggle chain via menu" (12431e0)

I think it would be better if these notes were clearer about what behavior is. What happens if you don't follow the advice? Is this saying if you run use -testnet the parameter will be silently ignored because the GUI overrides it internally?

If so, I think this might rise to the level of a bug because command line arguments by design are supposed to override GUI settings and other configuration sources. If not, and there is an explicit error about conflicting settings, that is probably ok. But I think ideally GUI setting would still be overridden if -chain or -testnet or -signet or -regtest were specified. It seems like it should be trivial to implement that instead of requiring users to read this and change their setups.


- The Bech32 checkbox has been replaced with a dropdown for all address types, including the new Bech32m (BIP-350) standard for Taproot enabled wallets.

Low-level changes
Expand Down
4 changes: 4 additions & 0 deletions src/qt/bitcoin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,10 @@ int GuiMain(int argc, char* argv[])
// - QSettings() will use the new application name after this, resulting in network-specific settings
// - Needs to be done before createOptionsModel

QSettings settings;
std::string chain = settings.value("chain", "main").toString().toStdString();
gArgs.SoftSetArg("-chain", chain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: toggle chain via menu" (12431e0)

It might be good to show a message on startup the first time bitcoin is started after switching networks.

The toggle chain menu could set a boolean "switched_chain" QSetting, and this code could check for the "switched_chain" setting, clear it, and show a message like "Bitcoin Core is connecting to new network {network}. Use the "Switch chain" menu to switch networks again." (It could be a dialog with a "Do not show this message in the future checkbox" if users switch network repeatedly.) But I think this would provide a useful alert and a good second chance to recover if someone clicked through the first dialog not expecting bitcoin to be starting from scratch again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #414 (comment)

It might be good to show a message on startup the first time bitcoin is started after switching networks. [...]

Replying to myself, maybe a better startup message would be: "Bitcoin-Qt configuration has changed and will now connect to the {Testnet} network instead of the {Mainnet} network. {Mainnet} wallets will not be loaded while connected to the {Testnet} network. Also {Testnet} coins are separate from actual bitcoins, and are not supposed to have any value." Dialog could include a checkbox "Do not show this message again," and buttons "Continue connecting to {Testnet} network" / "Exit and switch back to {Mainnet network}" for a safe escape.

(And again I really think ideally none of these dialogs would be necessary and new networks would just open in new windows without closing existing ones. But showing clear messages could at least avoid pitfalls if only one window can be open at a time)


// Check for chain settings (Params() calls are only valid after this clause)
try {
SelectParams(gArgs.GetChainName());
Expand Down
41 changes: 41 additions & 0 deletions src/qt/bitcoingui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,46 @@ void BitcoinGUI::createMenuBar()
appMenuBar = menuBar();
#endif

// Add chain selection submenu
QAction* chain_selection_action{nullptr};
QMenu* chain_selection_menu{nullptr};
chain_selection_action = new QAction(tr("&Switch chain"), this);
chain_selection_action->setStatusTip(tr("Restart application using a different (test) network"));
chain_selection_menu = new QMenu(this);

connect(chain_selection_menu, &QMenu::aboutToShow, [this, chain_selection_menu] {
chain_selection_menu->clear();
const std::vector<std::pair<QString, QString>> chains = {{"main", "&Bitcoin"}, {"test", "&Testnet"}, {"regtest", "&Regtest"}, {"signet", "&Signet"}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: toggle chain via menu" (12431e0)

Interesting the text is is "Bitcoin" instead of "Mainnet". I think that it is good because it implies the other networks are not the Bitcoin network. Another way to do this while being a little more consistent could be to make the choices "Mainnet (Bitcoin)" / "Testnet" / "Regtest" / "Signet"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noted this as well - IMO "Bitcoin" is a better way to go - "Mainnet" may actually confuse new users at this point.

for (auto chain : chains) {
const bool is_current = gArgs.GetChainName() == chain.first.toStdString();
QAction* action = chain_selection_menu->addAction(chain.second);
action->setCheckable(true);
action->setChecked(is_current);
action->setEnabled(!is_current);

connect(action, &QAction::triggered, [this, chain] {
//: Switch to the mainnet, testnet, signet or regtest chain.
QMessageBox::StandardButton btn_ret_val = QMessageBox::question(this, tr("Switch chain"),
//: Switching between mainnet, testnet, signet or regtest chain requires a restart.
tr("Client restart required to switch chain.\n\nClient will be shut down. Do you want to proceed?"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "gui: toggle chain via menu" (12431e0)

I think it would be good for the dialog to describe what the option actually does. I'm concerned that someone could click this without really understanding it, and then next time they start bitcoin they find their wallets are missing and bitcoin is doing a new sync to a different network.

I think ideally dialog would say something like. "Are you sure you want to switch networks from main to test? Testnet coins are separate from actual bitcoins, and are not supposed to have any value. Switching networks
requires restarting Bitcoin Core, and any existing wallets from the current network will be hidden while using the other network. It will be possible to switch back to the current network by using the "Switch chain" menu again and restarting." It could also be good to change the affirmative button text from "Yes" to something more explicit like "Switch to {network name} network and exit."

QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);

if (btn_ret_val == QMessageBox::Cancel) return;

// QSettings are stored seperately for each network. Switch application name
// to mainnet before storing the selected chain.
QScopedPointer<const NetworkStyle> networkStyle(NetworkStyle::instantiate("main"));
assert(!networkStyle.isNull());
QApplication::setApplicationName(networkStyle->getAppName());

QSettings settings;
settings.setValue("chain", chain.first);
Q_EMIT quitRequested();
});
}
});
chain_selection_action->setMenu(chain_selection_menu);

// Configure the menus
QMenu *file = appMenuBar->addMenu(tr("&File"));
if(walletFrame)
Expand Down Expand Up @@ -483,6 +523,7 @@ void BitcoinGUI::createMenuBar()
settings->addSeparator();
}
settings->addAction(optionsAction);
settings->addAction(chain_selection_action);

QMenu* window_menu = appMenuBar->addMenu(tr("&Window"));

Expand Down