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.

Sign up for GitHub

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

Jump to bottom

Parse params directly instead of through node (partial revert #10244) #35

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Parse params directly instead of through node (partial revert #10244) #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e133631
gui: Partially revert #10244 gArgs and Params changes
ryanofsky Aug 23, 2018
91aced7
gui: Remove unused interfaces::Node references
ryanofsky Aug 23, 2018
102abff
gui: Replace interface::Node references with pointers
ryanofsky Aug 23, 2018
519cae8
gui: Delay interfaces::Node initialization
ryanofsky Jul 6, 2020
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
19 changes: 2 additions & 17 deletions src/interfaces/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,8 @@ class NodeImpl : public Node
{
public:
NodeImpl(NodeContext* context) { setContext(context); }
void initError(const bilingual_str& message) override { InitError(message); }
bool parseParameters(int argc, const char* const argv[], std::string& error) override
{
return gArgs.ParseParameters(argc, argv, error);
}
bool readConfigFiles(std::string& error) override { return gArgs.ReadConfigFiles(error, true); }
void forceSetArg(const std::string& arg, const std::string& value) override { gArgs.ForceSetArg(arg, value); }
bool softSetArg(const std::string& arg, const std::string& value) override { return gArgs.SoftSetArg(arg, value); }
bool softSetBoolArg(const std::string& arg, bool value) override { return gArgs.SoftSetBoolArg(arg, value); }
void selectParams(const std::string& network) override { SelectParams(network); }
bool initSettings(std::string& error) override { return gArgs.InitSettings(error); }
uint64_t getAssumedBlockchainSize() override { return Params().AssumedBlockchainSize(); }
uint64_t getAssumedChainStateSize() override { return Params().AssumedChainStateSize(); }
std::string getNetwork() override { return Params().NetworkIDString(); }
void initLogging() override { InitLogging(gArgs); }
void initParameterInteraction() override { InitParameterInteraction(gArgs); }
void initLogging() override { InitLogging(*Assert(m_context->args)); }
void initParameterInteraction() override { InitParameterInteraction(*Assert(m_context->args)); }
bilingual_str getWarnings() override { return GetWarnings(true); }
uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
bool baseInitialize() override
Expand Down Expand Up @@ -109,7 +95,6 @@ class NodeImpl : public Node
StopMapPort();
}
}
void setupServerArgs() override { return SetupServerArgs(*m_context); }
bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
size_t getNodeCount(CConnman::NumConnections flags) override
{
Expand Down
38 changes: 0 additions & 38 deletions src/interfaces/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,41 +55,6 @@ class Node
public:
virtual ~Node() {}

//! Send init error.
virtual void initError(const bilingual_str& message) = 0;

//! Set command line arguments.
virtual bool parseParameters(int argc, const char* const argv[], std::string& error) = 0;

//! Set a command line argument
virtual void forceSetArg(const std::string& arg, const std::string& value) = 0;

//! Set a command line argument if it doesn't already have a value
virtual bool softSetArg(const std::string& arg, const std::string& value) = 0;

//! Set a command line boolean argument if it doesn't already have a value
virtual bool softSetBoolArg(const std::string& arg, bool value) = 0;

//! Load settings from configuration file.
virtual bool readConfigFiles(std::string& error) = 0;

//! Choose network parameters.
virtual void selectParams(const std::string& network) = 0;

//! Read and update <datadir>/settings.json file with saved settings. This
//! needs to be called after selectParams() because the settings file
//! location is network-specific.
virtual bool initSettings(std::string& error) = 0;

//! Get the (assumed) blockchain size.
virtual uint64_t getAssumedBlockchainSize() = 0;

//! Get the (assumed) chain state size.
virtual uint64_t getAssumedChainStateSize() = 0;

//! Get network name.
virtual std::string getNetwork() = 0;

//! Init logging.
virtual void initLogging() = 0;

Expand Down Expand Up @@ -117,9 +82,6 @@ class Node
//! Return whether shutdown was requested.
virtual bool shutdownRequested() = 0;

//! Setup arguments
virtual void setupServerArgs() = 0;

//! Map port.
virtual void mapPort(bool use_upnp) = 0;

Expand Down
79 changes: 46 additions & 33 deletions src/qt/bitcoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,19 @@
#include <qt/walletmodel.h>
#endif // ENABLE_WALLET

#include <init.h>
#include <interfaces/handler.h>
#include <interfaces/node.h>
#include <node/context.h>
#include <node/ui_interface.h>
#include <noui.h>
#include <uint256.h>
#include <util/system.h>
#include <util/threadnames.h>
#include <util/translation.h>
#include <validation.h>

#include <boost/signals2/connection.hpp>
#include <memory>

#include <QApplication>
Expand Down Expand Up @@ -193,10 +196,9 @@ void BitcoinCore::shutdown()
static int qt_argc = 1;
static const char* qt_argv = "bitcoin-qt";

BitcoinApplication::BitcoinApplication(interfaces::Node& node):
BitcoinApplication::BitcoinApplication():
QApplication(qt_argc, const_cast<char **>(&qt_argv)),
coreThread(nullptr),
m_node(node),
optionsModel(nullptr),
clientModel(nullptr),
window(nullptr),
Expand Down Expand Up @@ -246,38 +248,47 @@ void BitcoinApplication::createPaymentServer()

void BitcoinApplication::createOptionsModel(bool resetSettings)
{
optionsModel = new OptionsModel(m_node, this, resetSettings);
optionsModel = new OptionsModel(this, resetSettings);
}

void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
{
window = new BitcoinGUI(m_node, platformStyle, networkStyle, nullptr);
window = new BitcoinGUI(node(), platformStyle, networkStyle, nullptr);

pollShutdownTimer = new QTimer(window);
connect(pollShutdownTimer, &QTimer::timeout, window, &BitcoinGUI::detectShutdown);
}

void BitcoinApplication::createSplashScreen(const NetworkStyle *networkStyle)
{
SplashScreen *splash = new SplashScreen(m_node, nullptr, networkStyle);
assert(!m_splash);
m_splash = new SplashScreen(nullptr, networkStyle);
// We don't hold a direct pointer to the splash screen after creation, but the splash
// screen will take care of deleting itself when finish() happens.
splash->show();
connect(this, &BitcoinApplication::splashFinished, splash, &SplashScreen::finish);
connect(this, &BitcoinApplication::requestedShutdown, splash, &QWidget::close);
m_splash->show();
connect(this, &BitcoinApplication::splashFinished, m_splash, &SplashScreen::finish);
connect(this, &BitcoinApplication::requestedShutdown, m_splash, &QWidget::close);
}

void BitcoinApplication::setNode(interfaces::Node& node)
{
assert(!m_node);
m_node = &node;
if (optionsModel) optionsModel->setNode(*m_node);
if (m_splash) m_splash->setNode(*m_node);
}

bool BitcoinApplication::baseInitialize()
{
return m_node.baseInitialize();
return node().baseInitialize();
}

void BitcoinApplication::startThread()
{
if(coreThread)
return;
coreThread = new QThread(this);
BitcoinCore *executor = new BitcoinCore(m_node);
BitcoinCore *executor = new BitcoinCore(node());
executor->moveToThread(coreThread);

/* communication to and from thread */
Expand All @@ -298,8 +309,8 @@ void BitcoinApplication::parameterSetup()
// print to the console unnecessarily.
gArgs.SoftSetBoolArg("-printtoconsole", false);

m_node.initLogging();
m_node.initParameterInteraction();
InitLogging(gArgs);
InitParameterInteraction(gArgs);
}

void BitcoinApplication::InitializePruneSetting(bool prune)
Expand Down Expand Up @@ -331,7 +342,7 @@ void BitcoinApplication::requestShutdown()
window->unsubscribeFromCoreSignals();
// Request node shutdown, which can interrupt long operations, like
// rescanning a wallet.
m_node.startShutdown();
node().startShutdown();
// Unsetting the client model can cause the current thread to wait for node
// to complete an operation, like wait for a RPC execution to complete.
window->setClientModel(nullptr);
Expand All @@ -353,7 +364,7 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
{
// Log this only after AppInitMain finishes, as then logging setup is guaranteed complete
qInfo() << "Platform customization:" << platformStyle->getName();
clientModel = new ClientModel(m_node, optionsModel);
clientModel = new ClientModel(node(), optionsModel);
window->setClientModel(clientModel, &tip_info);
#ifdef ENABLE_WALLET
if (WalletModel::isWalletEnabled()) {
Expand Down Expand Up @@ -437,9 +448,9 @@ int GuiMain(int argc, char* argv[])
std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);

// Subscribe to global signals from core
std::unique_ptr<interfaces::Handler> handler_message_box = node->handleMessageBox(noui_ThreadSafeMessageBox);
std::unique_ptr<interfaces::Handler> handler_question = node->handleQuestion(noui_ThreadSafeQuestion);
std::unique_ptr<interfaces::Handler> handler_init_message = node->handleInitMessage(noui_InitMessage);
boost::signals2::scoped_connection handler_message_box = ::uiInterface.ThreadSafeMessageBox_connect(noui_ThreadSafeMessageBox);
boost::signals2::scoped_connection handler_question = ::uiInterface.ThreadSafeQuestion_connect(noui_ThreadSafeQuestion);
boost::signals2::scoped_connection handler_init_message = ::uiInterface.InitMessage_connect(noui_InitMessage);

// Do not refer to data directory yet, this can be overridden by Intro::pickDataDirectory

Expand All @@ -453,15 +464,15 @@ int GuiMain(int argc, char* argv[])
QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
#endif

BitcoinApplication app(*node);
BitcoinApplication app;

/// 2. Parse command-line options. We do this after qt in order to show an error if there are problems parsing these
// Command-line options take precedence:
node->setupServerArgs();
SetupServerArgs(node_context);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could add an unused reference to the argsmanager that is supposed to be used by gui code (instead of the global).

Would either be const ArgsManager& args = *node_context.args or = gArgs?

Feel free to ingore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #35 (comment)

nit: Could add an unused reference to the argsmanager that is supposed to be used by gui code (instead of the global).

I started implementing this but stopped because there are a number of other uses of gArgs in this function that I thought it would be better not to change here to avoid increasing the size of the PR.

Bigger picture, gArgs is used pretty widely in many parts of qt code and I think that if it's going to be removed, it'd be best to do it in a focused PR updating all the references at the same time.

SetupUIArgs(gArgs);
std::string error;
if (!node->parseParameters(argc, argv, error)) {
node->initError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
if (!gArgs.ParseParameters(argc, argv, error)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: (if feedback is addressed)

Could use

Suggested change
if (!gArgs.ParseParameters(argc, argv, error)) {
if (!args.ParseParameters(argc, argv, error)) {

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: #35 (comment)

Could use

(same feedback)

InitError(strprintf(Untranslated("Error parsing command line arguments: %s\n"), error));
// Create a message box, because the gui has neither been created nor has subscribed to core signals
QMessageBox::critical(nullptr, PACKAGE_NAME,
// message can not be translated because translations have not been initialized
Expand All @@ -487,7 +498,7 @@ int GuiMain(int argc, char* argv[])
// Show help message immediately after parsing command-line options (for "-lang") and setting locale,
// but before showing splash screen.
if (HelpRequested(gArgs) || gArgs.IsArgSet("-version")) {
HelpMessageDialog help(*node, nullptr, gArgs.IsArgSet("-version"));
HelpMessageDialog help(nullptr, gArgs.IsArgSet("-version"));
help.showOrPrint();
return EXIT_SUCCESS;
}
Expand All @@ -497,18 +508,18 @@ int GuiMain(int argc, char* argv[])
bool did_show_intro = false;
bool prune = false; // Intro dialog prune check box
// Gracefully exit if the user cancels
if (!Intro::showIfNeeded(*node, did_show_intro, prune)) return EXIT_SUCCESS;
if (!Intro::showIfNeeded(did_show_intro, prune)) return EXIT_SUCCESS;

/// 6. Determine availability of data directory and parse bitcoin.conf
/// - Do not call GetDataDir(true) before this step finishes
if (!CheckDataDirOption()) {
node->initError(strprintf(Untranslated("Specified data directory \"%s\" does not exist.\n"), gArgs.GetArg("-datadir", "")));
InitError(strprintf(Untranslated("Specified data directory \"%s\" does not exist.\n"), gArgs.GetArg("-datadir", "")));
QMessageBox::critical(nullptr, PACKAGE_NAME,
QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", ""))));
return EXIT_FAILURE;
}
if (!node->readConfigFiles(error)) {
node->initError(strprintf(Untranslated("Error reading configuration file: %s\n"), error));
if (!gArgs.ReadConfigFiles(error, true)) {
InitError(strprintf(Untranslated("Error reading configuration file: %s\n"), error));
QMessageBox::critical(nullptr, PACKAGE_NAME,
QObject::tr("Error: Cannot parse configuration file: %1.").arg(QString::fromStdString(error)));
return EXIT_FAILURE;
Expand All @@ -522,18 +533,18 @@ int GuiMain(int argc, char* argv[])

// Check for -chain, -testnet or -regtest parameter (Params() calls are only valid after this clause)
try {
node->selectParams(gArgs.GetChainName());
SelectParams(gArgs.GetChainName());
} catch(std::exception &e) {
node->initError(Untranslated(strprintf("%s\n", e.what())));
InitError(Untranslated(strprintf("%s\n", e.what())));
QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error: %1").arg(e.what()));
return EXIT_FAILURE;
}
#ifdef ENABLE_WALLET
// Parse URIs on command line -- this can affect Params()
PaymentServer::ipcParseCommandLine(*node, argc, argv);
PaymentServer::ipcParseCommandLine(argc, argv);
#endif
if (!node->initSettings(error)) {
node->initError(Untranslated(error));
if (!gArgs.InitSettings(error)) {
InitError(Untranslated(error));
QMessageBox::critical(nullptr, PACKAGE_NAME, QObject::tr("Error initializing settings: %1").arg(QString::fromStdString(error)));
return EXIT_FAILURE;
}
Expand Down Expand Up @@ -587,6 +598,8 @@ int GuiMain(int argc, char* argv[])
if (gArgs.GetBoolArg("-splash", DEFAULT_SPLASHSCREEN) && !gArgs.GetBoolArg("-min", false))
app.createSplashScreen(networkStyle.data());

app.setNode(*node);

int rv = EXIT_SUCCESS;
try
{
Expand All @@ -609,10 +622,10 @@ int GuiMain(int argc, char* argv[])
}
} catch (const std::exception& e) {
PrintExceptionContinue(&e, "Runaway exception");
app.handleRunawayException(QString::fromStdString(node->getWarnings().translated));
app.handleRunawayException(QString::fromStdString(app.node().getWarnings().translated));
} catch (...) {
PrintExceptionContinue(nullptr, "Runaway exception");
app.handleRunawayException(QString::fromStdString(node->getWarnings().translated));
app.handleRunawayException(QString::fromStdString(app.node().getWarnings().translated));
}
return rv;
}
10 changes: 8 additions & 2 deletions src/qt/bitcoin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#endif

#include <QApplication>
#include <assert.h>
#include <memory>

#include <interfaces/node.h>
Expand All @@ -20,6 +21,7 @@ class NetworkStyle;
class OptionsModel;
class PaymentServer;
class PlatformStyle;
class SplashScreen;
class WalletController;
class WalletModel;

Expand Down Expand Up @@ -54,7 +56,7 @@ class BitcoinApplication: public QApplication
{
Q_OBJECT
public:
explicit BitcoinApplication(interfaces::Node& node);
explicit BitcoinApplication();
~BitcoinApplication();

#ifdef ENABLE_WALLET
Expand Down Expand Up @@ -88,6 +90,9 @@ class BitcoinApplication: public QApplication
/// Setup platform style
void setupPlatformStyle();

interfaces::Node& node() const { assert(m_node); return *m_node; }
void setNode(interfaces::Node& node);

public Q_SLOTS:
void initializeResult(bool success, interfaces::BlockAndHeaderTipInfo tip_info);
void shutdownResult();
Expand All @@ -102,7 +107,6 @@ public Q_SLOTS:

private:
QThread *coreThread;
interfaces::Node& m_node;
OptionsModel *optionsModel;
ClientModel *clientModel;
BitcoinGUI *window;
Expand All @@ -114,6 +118,8 @@ public Q_SLOTS:
int returnValue;
const PlatformStyle *platformStyle;
std::unique_ptr<QWidget> shutdownWindow;
SplashScreen* m_splash = nullptr;
interfaces::Node* m_node = nullptr;

void startThread();
};
Expand Down
4 changes: 2 additions & 2 deletions src/qt/bitcoingui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
updateWindowTitle();

rpcConsole = new RPCConsole(node, _platformStyle, nullptr);
helpMessageDialog = new HelpMessageDialog(node, this, false);
helpMessageDialog = new HelpMessageDialog(this, false);
#ifdef ENABLE_WALLET
if(enableWallet)
{
Expand Down Expand Up @@ -821,7 +821,7 @@ void BitcoinGUI::aboutClicked()
if(!clientModel)
return;

HelpMessageDialog dlg(m_node, this, true);
HelpMessageDialog dlg(this, true);
dlg.exec();
}

Expand Down
Loading