Merge bitcoin#10244: Refactor: separate gui from wallet and node

9960137 Add developer notes about blocking GUI code (Russell Yanofsky)
9a61eed Use WalletBalances struct in Qt (Russell Yanofsky)
56f33ca Remove direct bitcoin calls from qt/sendcoinsdialog.cpp (Russell Yanofsky)
e872c93 Remove direct bitcoin access from qt/guiutil.cpp (Russell Yanofsky)
5884558 Remove direct bitcoin calls from qt transaction table files (Russell Yanofsky)
3cab2ce Remove direct bitcoin calls from qt/paymentserver.cpp (Russell Yanofsky)
3ec2ebc Remove direct bitcoin calls from qt/addresstablemodel.cpp (Russell Yanofsky)
827de03 Remove direct bitcoin calls from qt/coincontroldialog.cpp (Russell Yanofsky)
a0704a8 Remove most direct bitcoin calls from qt/walletmodel.cpp (Russell Yanofsky)
90d4640 Remove direct bitcoin calls from qt/optionsdialog.cpp (Russell Yanofsky)
582daf6 Remove direct bitcoin calls from qt/rpcconsole.cpp (Russell Yanofsky)
3034a46 Remove direct bitcoin calls from qt/bantablemodel.cpp (Russell Yanofsky)
e0b66a3 Remove direct bitcoin calls from qt/peertablemodel.cpp (Russell Yanofsky)
d7c2c95 Remove direct bitcoin calls from qt/intro.cpp (Russell Yanofsky)
fe6f27e Remove direct bitcoin calls from qt/clientmodel.cpp (Russell Yanofsky)
5fba3af Remove direct bitcoin calls from qt/splashscreen.cpp (Russell Yanofsky)
c2f672f Remove direct bitcoin calls from qt/utilitydialog.cpp (Russell Yanofsky)
3d619e9 Remove direct bitcoin calls from qt/bitcoingui.cpp (Russell Yanofsky)
c0f2756 Remove direct bitcoin calls from qt/optionsmodel.cpp (Russell Yanofsky)
71e0d90 Remove direct bitcoin calls from qt/bitcoin.cpp (Russell Yanofsky)
ea73b84 Add src/interface/README.md (Russell Yanofsky)

Pull request description:

  This is a refactoring PR that does not change behavior in any way. This change:

  1. Creates abstract [`Node`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-local/src/interface/node.h) and [`Wallet`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-local/src/interface/wallet.h) interfaces in [`src/interface/`](https://github.com/ryanofsky/bitcoin/tree/pr/ipc-local/src/interface)
  1. Updates Qt code to call the new interfaces. This largely consists of diffs of the form:

  ```diff
  -    InitLogging();
  -    InitParameterInteraction();
  +    node.initLogging();
  +    node.initParameterInteraction();
  ```

  This change allows followup PR bitcoin#10102 (makes `bitcoin-qt` control `bitcoind` over an IPC socket) to work without any significant updates to Qt code. Additionally:

  * It provides a single place to describe the interface between GUI and daemon code.
  * It can make better GUI testing possible, because Node and Wallet objects have virtual methods that can be overloaded for mocking.
  * It can be used to help make the GUI more responsive (see bitcoin#10504)

  Other notes:

  * I used python scripts [hide-globals.py](https://github.com/ryanofsky/home/blob/master/src/2017/hide-globals/hide-globals.py) and [replace-syms.py](https://github.com/ryanofsky/home/blob/master/src/2017/hide-globals/replace-syms.py) to identify all the places where Qt code was accessing libbitcoin global variables and calling functions accessing those global variables.
  * These changes were originally part of bitcoin#10102. Thanks to @JeremyRubin for the suggestion of splitting them out.

  Commits:

  - [`ea73b84d2d` Add src/interface/README.md](bitcoin@ea73b84)
  - [`71e0d90876` Remove direct bitcoin calls from qt/bitcoin.cpp](bitcoin@71e0d90)
  - [`c0f2756be5` Remove direct bitcoin calls from qt/optionsmodel.cpp](bitcoin@c0f2756)
  - [`3d619e9d36` Remove direct bitcoin calls from qt/bitcoingui.cpp](bitcoin@3d619e9)
  - [`c2f672fb19` Remove direct bitcoin calls from qt/utilitydialog.cpp](bitcoin@c2f672f)
  - [`5fba3af21e` Remove direct bitcoin calls from qt/splashscreen.cpp](bitcoin@5fba3af)
  - [`fe6f27e6ea` Remove direct bitcoin calls from qt/clientmodel.cpp](bitcoin@fe6f27e)
  - [`d7c2c95948` Remove direct bitcoin calls from qt/intro.cpp](bitcoin@d7c2c95)
  - [`e0b66a3b7c` Remove direct bitcoin calls from qt/peertablemodel.cpp](bitcoin@e0b66a3)
  - [`3034a462a5` Remove direct bitcoin calls from qt/bantablemodel.cpp](bitcoin@3034a46)
  - [`582daf6d22` Remove direct bitcoin calls from qt/rpcconsole.cpp](bitcoin@582daf6)
  - [`90d4640b7e` Remove direct bitcoin calls from qt/optionsdialog.cpp](bitcoin@90d4640)
  - [`a0704a8996` Remove most direct bitcoin calls from qt/walletmodel.cpp](bitcoin@a0704a8)
  - [`827de038ab` Remove direct bitcoin calls from qt/coincontroldialog.cpp](bitcoin@827de03)
  - [`3ec2ebcd9b` Remove direct bitcoin calls from qt/addresstablemodel.cpp](bitcoin@3ec2ebc)
  - [`3cab2ce5f9` Remove direct bitcoin calls from qt/paymentserver.cpp](bitcoin@3cab2ce)
  - [`58845587e1` Remove direct bitcoin calls from qt transaction table files](bitcoin@5884558)
  - [`e872c93ee8` Remove direct bitcoin access from qt/guiutil.cpp](bitcoin@e872c93)
  - [`56f33ca349` Remove direct bitcoin calls from qt/sendcoinsdialog.cpp](bitcoin@56f33ca)
  - [`9a61eed1fc` Use WalletBalances struct in Qt](bitcoin@9a61eed)
  - [`9960137697` Add developer notes about blocking GUI code](bitcoin@9960137)

Tree-SHA512: 7b9eff2f37d4ea21972d7cc6a3dbe144248595d6c330524396d867f3cd2841d666cdc040fd3605af559dab51b075812402f61d628d16cf13719335c1d8bf8ed3

doc/developer-notes.md

@@ -625,6 +625,19 @@ GUI
     should not interact with the user. That's where View classes come in. The converse also
     holds: try to not directly access core data structures from Views.
 
+- Avoid adding slow or blocking code in the GUI thread. In particular do not
+  add new `interface::Node` and `interface::Wallet` method calls, even if they
+  may be fast now, in case they are changed to lock or communicate across
+  processes in the future.
+
+  Prefer to offload work from the GUI thread to worker threads (see
+  `RPCExecutor` in console code as an example) or take other steps (see
+  https://doc.qt.io/archives/qq/qq27-responsive-guis.html) to keep the GUI
+  responsive.
+
+  - *Rationale*: Blocking the GUI thread can increase latency, and lead to
+    hangs and deadlocks.
+
 Subtrees
 ----------
 

src/Makefile.am

@@ -7,6 +7,7 @@ DIST_SUBDIRS = secp256k1 univalue
 AM_LDFLAGS = $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(GPROF_LDFLAGS) $(SANITIZER_LDFLAGS)
 AM_CXXFLAGS = $(HARDENED_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS)
 AM_CPPFLAGS = $(HARDENED_CPPFLAGS)
+AM_LIBTOOLFLAGS = --preserve-dup-deps
 EXTRA_LIBRARIES =
 
 if EMBEDDED_UNIVALUE
@@ -104,6 +105,9 @@ BITCOIN_CORE_H = \
   httpserver.h \
   indirectmap.h \
   init.h \
+  interface/handler.h \
+  interface/node.h \
+  interface/wallet.h \
   key.h \
   key_io.h \
   keystore.h \
@@ -245,6 +249,7 @@ endif
 libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
 libbitcoin_wallet_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 libbitcoin_wallet_a_SOURCES = \
+  interface/wallet.cpp \
   wallet/crypter.cpp \
   wallet/db.cpp \
   wallet/feebumper.cpp \
@@ -357,6 +362,8 @@ libbitcoin_util_a_SOURCES = \
   compat/glibcxx_sanity.cpp \
   compat/strnlen.cpp \
   fs.cpp \
+  interface/handler.cpp \
+  interface/node.cpp \
   random.cpp \
   rpc/protocol.cpp \
   rpc/util.cpp \

src/Makefile.qt.include

@@ -402,7 +402,7 @@ if TARGET_WINDOWS
 endif
 qt_bitcoin_qt_LDADD = qt/libbitcoinqt.a $(LIBBITCOIN_SERVER)
 if ENABLE_WALLET
-qt_bitcoin_qt_LDADD += $(LIBBITCOIN_WALLET)
+qt_bitcoin_qt_LDADD += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
 endif
 if ENABLE_ZMQ
 qt_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
@@ -411,7 +411,7 @@ qt_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL)
   $(BOOST_LIBS) $(QT_LIBS) $(QT_DBUS_LIBS) $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
 qt_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
-qt_bitcoin_qt_LIBTOOLFLAGS = --tag CXX
+qt_bitcoin_qt_LIBTOOLFLAGS = $(AM_LIBTOOLFLAGS) --tag CXX
 
 #locale/foo.ts -> locale/foo.qm
 QT_QM=$(QT_TS:.ts=.qm)

src/Makefile.qttest.include

@@ -52,7 +52,7 @@ nodist_qt_test_test_bitcoin_qt_SOURCES = $(TEST_QT_MOC_CPP)
 
 qt_test_test_bitcoin_qt_LDADD = $(LIBBITCOINQT) $(LIBBITCOIN_SERVER)
 if ENABLE_WALLET
-qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_WALLET)
+qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
 endif
 if ENABLE_ZMQ
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)

src/interface/README.md

@@ -0,0 +1,17 @@
+# Internal c++ interfaces
+
+The following interfaces are defined here:
+
+* [`Chain`](chain.h) — used by wallet to access blockchain and mempool state. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
+
+* [`Chain::Client`](chain.h) — used by node to start & stop `Chain` clients. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
+
+* [`Node`](node.h) — used by GUI to start & stop bitcoin node. Added in [#10244](https://github.com/bitcoin/bitcoin/pull/10244).
+
+* [`Wallet`](wallet.h) — used by GUI to access wallets. Added in [#10244](https://github.com/bitcoin/bitcoin/pull/10244).
+
+* [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
+
+* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
+
+The interfaces above define boundaries between major components of bitcoin code (node, wallet, and gui), making it possible for them to run in different processes, and be tested, developed, and understood independently. These interfaces are not currently designed to be stable or to be used externally.

src/interface/handler.cpp

@@ -0,0 +1,33 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <interface/handler.h>
+
+#include <util.h>
+
+#include <boost/signals2/connection.hpp>
+#include <memory>
+#include <utility>
+
+namespace interface {
+namespace {
+
+class HandlerImpl : public Handler
+{
+public:
+    HandlerImpl(boost::signals2::connection connection) : m_connection(std::move(connection)) {}
+
+    void disconnect() override { m_connection.disconnect(); }
+
+    boost::signals2::scoped_connection m_connection;
+};
+
+} // namespace
+
+std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection)
+{
+    return MakeUnique<HandlerImpl>(std::move(connection));
+}
+
+} // namespace interface

src/interface/handler.h

@@ -0,0 +1,35 @@
+// Copyright (c) 2018 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_INTERFACE_HANDLER_H
+#define BITCOIN_INTERFACE_HANDLER_H
+
+#include <memory>
+
+namespace boost {
+namespace signals2 {
+class connection;
+} // namespace signals2
+} // namespace boost
+
+namespace interface {
+
+//! Generic interface for managing an event handler or callback function
+//! registered with another interface. Has a single disconnect method to cancel
+//! the registration and prevent any future notifications.
+class Handler
+{
+public:
+    virtual ~Handler() {}
+
+    //! Disconnect the handler.
+    virtual void disconnect() = 0;
+};
+
+//! Return handler wrapping a boost signal connection.
+std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection);
+
+} // namespace interface
+
+#endif // BITCOIN_INTERFACE_HANDLER_H