Skip to content

Commit 5f0c6a7

Browse files
committed
Merge #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 #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/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 #10102. Thanks to @JeremyRubin for the suggestion of splitting them out. Commits: - [`ea73b84d2d` Add src/interface/README.md](bitcoin/bitcoin@ea73b84) - [`71e0d90876` Remove direct bitcoin calls from qt/bitcoin.cpp](bitcoin/bitcoin@71e0d90) - [`c0f2756be5` Remove direct bitcoin calls from qt/optionsmodel.cpp](bitcoin/bitcoin@c0f2756) - [`3d619e9d36` Remove direct bitcoin calls from qt/bitcoingui.cpp](bitcoin/bitcoin@3d619e9) - [`c2f672fb19` Remove direct bitcoin calls from qt/utilitydialog.cpp](bitcoin/bitcoin@c2f672f) - [`5fba3af21e` Remove direct bitcoin calls from qt/splashscreen.cpp](bitcoin/bitcoin@5fba3af) - [`fe6f27e6ea` Remove direct bitcoin calls from qt/clientmodel.cpp](bitcoin/bitcoin@fe6f27e) - [`d7c2c95948` Remove direct bitcoin calls from qt/intro.cpp](bitcoin/bitcoin@d7c2c95) - [`e0b66a3b7c` Remove direct bitcoin calls from qt/peertablemodel.cpp](bitcoin/bitcoin@e0b66a3) - [`3034a462a5` Remove direct bitcoin calls from qt/bantablemodel.cpp](bitcoin/bitcoin@3034a46) - [`582daf6d22` Remove direct bitcoin calls from qt/rpcconsole.cpp](bitcoin/bitcoin@582daf6) - [`90d4640b7e` Remove direct bitcoin calls from qt/optionsdialog.cpp](bitcoin/bitcoin@90d4640) - [`a0704a8996` Remove most direct bitcoin calls from qt/walletmodel.cpp](bitcoin/bitcoin@a0704a8) - [`827de038ab` Remove direct bitcoin calls from qt/coincontroldialog.cpp](bitcoin/bitcoin@827de03) - [`3ec2ebcd9b` Remove direct bitcoin calls from qt/addresstablemodel.cpp](bitcoin/bitcoin@3ec2ebc) - [`3cab2ce5f9` Remove direct bitcoin calls from qt/paymentserver.cpp](bitcoin/bitcoin@3cab2ce) - [`58845587e1` Remove direct bitcoin calls from qt transaction table files](bitcoin/bitcoin@5884558) - [`e872c93ee8` Remove direct bitcoin access from qt/guiutil.cpp](bitcoin/bitcoin@e872c93) - [`56f33ca349` Remove direct bitcoin calls from qt/sendcoinsdialog.cpp](bitcoin/bitcoin@56f33ca) - [`9a61eed1fc` Use WalletBalances struct in Qt](bitcoin/bitcoin@9a61eed) - [`9960137697` Add developer notes about blocking GUI code](bitcoin/bitcoin@9960137) Tree-SHA512: 7b9eff2f37d4ea21972d7cc6a3dbe144248595d6c330524396d867f3cd2841d666cdc040fd3605af559dab51b075812402f61d628d16cf13719335c1d8bf8ed3
2 parents 2b54155 + 9960137 commit 5f0c6a7

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

65 files changed

+2250
-1087
lines changed

doc/developer-notes.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,19 @@ GUI
625625
should not interact with the user. That's where View classes come in. The converse also
626626
holds: try to not directly access core data structures from Views.
627627

628+
- Avoid adding slow or blocking code in the GUI thread. In particular do not
629+
add new `interface::Node` and `interface::Wallet` method calls, even if they
630+
may be fast now, in case they are changed to lock or communicate across
631+
processes in the future.
632+
633+
Prefer to offload work from the GUI thread to worker threads (see
634+
`RPCExecutor` in console code as an example) or take other steps (see
635+
https://doc.qt.io/archives/qq/qq27-responsive-guis.html) to keep the GUI
636+
responsive.
637+
638+
- *Rationale*: Blocking the GUI thread can increase latency, and lead to
639+
hangs and deadlocks.
640+
628641
Subtrees
629642
----------
630643

src/Makefile.am

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ DIST_SUBDIRS = secp256k1 univalue
77
AM_LDFLAGS = $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(GPROF_LDFLAGS) $(SANITIZER_LDFLAGS)
88
AM_CXXFLAGS = $(HARDENED_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS)
99
AM_CPPFLAGS = $(HARDENED_CPPFLAGS)
10+
AM_LIBTOOLFLAGS = --preserve-dup-deps
1011
EXTRA_LIBRARIES =
1112

1213
if EMBEDDED_UNIVALUE
@@ -104,6 +105,9 @@ BITCOIN_CORE_H = \
104105
httpserver.h \
105106
indirectmap.h \
106107
init.h \
108+
interface/handler.h \
109+
interface/node.h \
110+
interface/wallet.h \
107111
key.h \
108112
key_io.h \
109113
keystore.h \
@@ -245,6 +249,7 @@ endif
245249
libbitcoin_wallet_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
246250
libbitcoin_wallet_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
247251
libbitcoin_wallet_a_SOURCES = \
252+
interface/wallet.cpp \
248253
wallet/crypter.cpp \
249254
wallet/db.cpp \
250255
wallet/feebumper.cpp \
@@ -357,6 +362,8 @@ libbitcoin_util_a_SOURCES = \
357362
compat/glibcxx_sanity.cpp \
358363
compat/strnlen.cpp \
359364
fs.cpp \
365+
interface/handler.cpp \
366+
interface/node.cpp \
360367
random.cpp \
361368
rpc/protocol.cpp \
362369
rpc/util.cpp \

src/Makefile.qt.include

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ if TARGET_WINDOWS
402402
endif
403403
qt_bitcoin_qt_LDADD = qt/libbitcoinqt.a $(LIBBITCOIN_SERVER)
404404
if ENABLE_WALLET
405-
qt_bitcoin_qt_LDADD += $(LIBBITCOIN_WALLET)
405+
qt_bitcoin_qt_LDADD += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
406406
endif
407407
if ENABLE_ZMQ
408408
qt_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
@@ -411,7 +411,7 @@ qt_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL)
411411
$(BOOST_LIBS) $(QT_LIBS) $(QT_DBUS_LIBS) $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
412412
$(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
413413
qt_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
414-
qt_bitcoin_qt_LIBTOOLFLAGS = --tag CXX
414+
qt_bitcoin_qt_LIBTOOLFLAGS = $(AM_LIBTOOLFLAGS) --tag CXX
415415

416416
#locale/foo.ts -> locale/foo.qm
417417
QT_QM=$(QT_TS:.ts=.qm)

src/Makefile.qttest.include

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ nodist_qt_test_test_bitcoin_qt_SOURCES = $(TEST_QT_MOC_CPP)
5252

5353
qt_test_test_bitcoin_qt_LDADD = $(LIBBITCOINQT) $(LIBBITCOIN_SERVER)
5454
if ENABLE_WALLET
55-
qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_WALLET)
55+
qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_UTIL) $(LIBBITCOIN_WALLET)
5656
endif
5757
if ENABLE_ZMQ
5858
qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)

src/interface/README.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# Internal c++ interfaces
2+
3+
The following interfaces are defined here:
4+
5+
* [`Chain`](chain.h) — used by wallet to access blockchain and mempool state. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
6+
7+
* [`Chain::Client`](chain.h) — used by node to start & stop `Chain` clients. Added in [#10973](https://github.com/bitcoin/bitcoin/pull/10973).
8+
9+
* [`Node`](node.h) — used by GUI to start & stop bitcoin node. Added in [#10244](https://github.com/bitcoin/bitcoin/pull/10244).
10+
11+
* [`Wallet`](wallet.h) — used by GUI to access wallets. Added in [#10244](https://github.com/bitcoin/bitcoin/pull/10244).
12+
13+
* [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
14+
15+
* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
16+
17+
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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <interface/handler.h>
6+
7+
#include <util.h>
8+
9+
#include <boost/signals2/connection.hpp>
10+
#include <memory>
11+
#include <utility>
12+
13+
namespace interface {
14+
namespace {
15+
16+
class HandlerImpl : public Handler
17+
{
18+
public:
19+
HandlerImpl(boost::signals2::connection connection) : m_connection(std::move(connection)) {}
20+
21+
void disconnect() override { m_connection.disconnect(); }
22+
23+
boost::signals2::scoped_connection m_connection;
24+
};
25+
26+
} // namespace
27+
28+
std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection)
29+
{
30+
return MakeUnique<HandlerImpl>(std::move(connection));
31+
}
32+
33+
} // namespace interface

src/interface/handler.h

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef BITCOIN_INTERFACE_HANDLER_H
6+
#define BITCOIN_INTERFACE_HANDLER_H
7+
8+
#include <memory>
9+
10+
namespace boost {
11+
namespace signals2 {
12+
class connection;
13+
} // namespace signals2
14+
} // namespace boost
15+
16+
namespace interface {
17+
18+
//! Generic interface for managing an event handler or callback function
19+
//! registered with another interface. Has a single disconnect method to cancel
20+
//! the registration and prevent any future notifications.
21+
class Handler
22+
{
23+
public:
24+
virtual ~Handler() {}
25+
26+
//! Disconnect the handler.
27+
virtual void disconnect() = 0;
28+
};
29+
30+
//! Return handler wrapping a boost signal connection.
31+
std::unique_ptr<Handler> MakeHandler(boost::signals2::connection connection);
32+
33+
} // namespace interface
34+
35+
#endif // BITCOIN_INTERFACE_HANDLER_H

0 commit comments

Comments
 (0)