This creates an incomplete Chain interface in src/interfaces/ and begins to update wallet code to use it.
#10973 builds on this, changing the wallet to use the new interface to access chain state, instead of using CBlockIndex pointers and global variables like chainActive.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
755@@ -754,7 +756,7 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
756 return EncodeHexTx(mergedTx);
757 }
758
759-UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)
760+UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)
chain unused?
Edit: Oh, used in a follow-up PR. Sorry!
Is it intentional that the interfaces code uses a different coding style than the rest of the code?
If so, does this need documenting somewhere, and if not, can it be converted?
master :-)
1767- if (pindex && fAbortRescan) {
1768- WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, progress_current);
1769- } else if (pindex && ShutdownRequested()) {
1770- WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current);
1771+ if (!block_hash.IsNull() && fAbortRescan) {
1772+ WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);
block_height has not been initialised?
This triggers undefined behaviour if the optional block_height has not been initialised?
Yes, and previously it would segfault if pindex were null, but these things will not happen at the time this function is called. I think the code in this function is not beautiful and could be rewritten, but I’m trying here to change existing code as little as possible, so it is easier for reviewers to verify that that behavior is unchanged.
1769- } else if (pindex && ShutdownRequested()) {
1770- WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current);
1771+ if (!block_hash.IsNull() && fAbortRescan) {
1772+ WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);
1773+ } else if (!block_hash.IsNull() && ShutdownRequested()) {
1774+ WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", *block_height, progress_current);
block_height has not been initialised?
33+ }
34+ Optional<int> getBlockHeight(const uint256& hash) override
35+ {
36+ auto it = ::mapBlockIndex.find(hash);
37+ if (it != ::mapBlockIndex.end() && it->second) {
38+ if (::chainActive.Contains(it->second)) {
if statement above?
Concept ACK. Thanks for splitting #10973 into more manageable sized PRs.
msvc-autogen.py changesfc7442f compiles on macOS 10.14, tests and bench pass, bitcoind and QT don’t show any strange behavior.
463@@ -461,6 +464,7 @@ endif
464 bitcoind_LDADD = \
465 $(LIBBITCOIN_SERVER) \
466 $(LIBBITCOIN_WALLET) \
467+ $(LIBBITCOIN_SERVER) \
$(LIBBITCOIN_SERVER) here. I can see one above.
In commit “Remove direct node->wallet calls in init.cpp” (eb1709c1af21881283f3c3962ebabb5de9423a56)
I don’t know why do you add $(LIBBITCOIN_SERVER) here. I can see one above.
Our libraries are static and have circular dependencies, so for unix linkers which process libraries in command line order and only pull in subsets of object files needed to resolve previously unresolved symbols, it’s often necessary to list the same libraries multiple times. For background:
https://stackoverflow.com/questions/45135/why-does-the-order-in-which-libraries-are-linked-sometimes-cause-errors-in-gcc https://eli.thegreenplace.net/2013/07/09/library-order-in-static-linking#circular-dependency
In this specific case, there’s a mutual dependency between wallet and server libraries, and dropping server after wallet leads to:
0 CXXLD bitcoind
1libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `interfaces::(anonymous namespace)::WalletImpl::handleUnload(std::function<void ()>)':
2/home/russ/src/bitcoin/src/interfaces/wallet.cpp:444: undefined reference to `interfaces::MakeHandler(boost::signals2::connection)'
3libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `interfaces::(anonymous namespace)::WalletImpl::handleShowProgress(std::function<void (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int)>)':
4/home/russ/src/bitcoin/src/interfaces/wallet.cpp:448: undefined reference to `interfaces::MakeHandler(boost::signals2::connection)'
5libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `interfaces::(anonymous namespace)::WalletImpl::handleStatusChanged(std::function<void ()>)':
6/home/russ/src/bitcoin/src/interfaces/wallet.cpp:452: undefined reference to `interfaces::MakeHandler(boost::signals2::connection)'
7libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `interfaces::(anonymous namespace)::WalletImpl::handleAddressBookChanged(std::function<void (boost::variant<CNoDestination, CKeyID, CScriptID, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown> const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, ChangeType)>)':
8/home/russ/src/bitcoin/src/interfaces/wallet.cpp:456: undefined reference to `interfaces::MakeHandler(boost::signals2::connection)'
9libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o): In function `interfaces::(anonymous namespace)::WalletImpl::handleTransactionChanged(std::function<void (uint256 const&, ChangeType)>)':
10/home/russ/src/bitcoin/src/interfaces/wallet.cpp:462: undefined reference to `interfaces::MakeHandler(boost::signals2::connection)'
11libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):/home/russ/src/bitcoin/src/interfaces/wallet.cpp:467: more undefined references to `interfaces::MakeHandler(boost::signals2::connection)' follow
12clang: error: linker command failed with exit code 1 (use -v to see invocation)
I noticed d0af21f adds a bunch of methods to interfaces/chain.cpp. I assume those are moved from somewhere else, but I don’t see their counterparts removed in this PR. Why is that?
The new Chain d0af21fab95ee453a1fee4242ceb89ca79d568eb methods aren’t moved from someplace else. They are fairly simple utility functions that wrapCBlockIndex/chainActive APIs so the wallet doesn’t access chain state directly.
Also worth noting that d0af21fab95ee453a1fee4242ceb89ca79d568eb is the main commit in this PR. The other commits are basically just mechanical changes preparing for d0af21fab95ee453a1fee4242ceb89ca79d568eb.
msvc-autogen.py change looks good to me.
would be good to get @ken2812221’s feedback on the msvc-autogen.py changes
ken2812221 did previously review those changes: #10973 (comment). They were in the first commit of the PR at that time: d487d953c9a10b11693ba8af62f5b54a77ec7686.
There were some comments in IRC that the PR description here might need to be improved. Would welcome any suggestions. #10973 has a more complete description of the direction this PR starts to go in.
re: #14437 (comment)
Is it intentional that the interfaces code uses a different coding style than the rest of the code?
If so, does this need documenting somewhere, and if not, can it be converted?
I’ll create a PR to document the slight style change. It’s used in gui code so I adopted it for #10244 and #10102, sticking with it in #10973 and here for consistency, and since I do like it.
499+public:
500+ WalletClientImpl(Chain& chain, std::vector<std::string> wallet_filenames)
501+ : m_chain(chain), m_wallet_filenames(std::move(wallet_filenames))
502+ {
503+ }
504+ void registerRpcs() override { RegisterWalletRPCCommands(::tableRPC); }
in commit e615ac5960
nit: I’d prefer to use return even in the void context. It is more verbose but seems safer in context of future changes. (Feel free to ignore this style nit)
41+ {
42+ const Optional<int> tip_height = getHeight();
43+ const Optional<int> height = getBlockHeight(hash);
44+ return tip_height && height ? *tip_height - *height + 1 : 0;
45+ }
46+ uint256 getBlockHash(int height) override { return ::chainActive[height]->GetBlockHash(); }
104@@ -38,6 +105,16 @@ class Chain
105 //! method is temporary and is only used in a few places to avoid changing
106 //! behavior while code is transitioned to use the Chain::Lock interface.
107 virtual std::unique_ptr<Lock> assumeLocked() = 0;
108+
109+ //! Return whether node has the block and optionally return block metadata or contents.
110+ virtual bool findBlock(const uint256& hash,
in commit c4b750f510
nit: Could state that block->IsNull() indicates a read-from-disk error.
536@@ -538,6 +537,7 @@ UniValue importwallet(const JSONRPCRequest& request)
537 throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
538 }
539
540+ Optional<int> tip_height;
in commit c4b750f:
Why is this needed?
100@@ -101,7 +101,11 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo
101 {
102 entry.pushKV("blockhash", wtx.hashBlock.GetHex());
103 entry.pushKV("blockindex", wtx.nIndex);
104- entry.pushKV("blocktime", LookupBlockIndex(wtx.hashBlock)->GetBlockTime());
105+ int64_t block_time;
106+ if (!chain.findBlock(wtx.hashBlock, nullptr /* CBlock */, &block_time)) {
in commit c4b750f:
The name of the argument is block, not CBlock?
100@@ -101,7 +101,11 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo
101 {
102 entry.pushKV("blockhash", wtx.hashBlock.GetHex());
103 entry.pushKV("blockindex", wtx.nIndex);
104- entry.pushKV("blocktime", LookupBlockIndex(wtx.hashBlock)->GetBlockTime());
105+ int64_t block_time;
106+ if (!chain.findBlock(wtx.hashBlock, nullptr /* CBlock */, &block_time)) {
107+ throw std::logic_error("invalid wallet transaction block hash");
in commit c4b750f:
It is not immediately clear where this exceptions goes to. Crashes the node?
I’d prefer if we sticked to jsonrpc exceptions in the rpc module, so that broken rpc code would not crash the node.
59+struct SCOPED_LOCKABLE LockAnnotation
60+{
61+ template <typename Mutex>
62+ explicit LockAnnotation(Mutex& mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
63+ {
64+ }
in commit 0b2cf7c3a8edd6feb4a8504675960c9cfcbc613c:
Could add the runtime assert that the lock is taken?
AssertLockHeld
re: #14437 (review)
That would make this depend on sync.h and require a bitcoin debug mutex instead of working with any mutex. It would also print a less useful file/line number than a standalone assert. Better imo to have separate locking primitives that each do one job well than to have a combined one that’s worse at both jobs.
Concept ACK c4b750f510d1913a6af6549b17bd3d89e211c62c
Last commit is (at least for me) non-trivial to verify its refactoring nature. I am wondering if it could be split up more…
3753@@ -3741,7 +3754,8 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
3754 {
3755 unsigned int nTimeSmart = wtx.nTimeReceived;
3756 if (!wtx.hashUnset()) {
3757- if (const CBlockIndex* pindex = LookupBlockIndex(wtx.hashBlock)) {
3758+ int64_t blocktime;
3759+ if (chain().findBlock(wtx.hashBlock, nullptr /* CBlock */, &blocktime)) {
In commit c4b750f510d1913a6:
The named argument is block, not CBlock?
This commit does not change behavior. It just adds new skeleton classes that
don't do anything and aren't instantiated yet.
This commit does not change behavior. All it does is pass new function
parameters.
It is easiest to review this change with:
git log -p -n1 -U0 --word-diff-regex=.
Route calls during node initialization and shutdown that would happen between a
node process and wallet processes through the serializable `Chain::Client`
interface, rather than `WalletInitInterface` which is now simpler and only
deals with early initialization and parameter interaction.
This commit mostly does not change behavior. The only change is that the
"Wallet disabled!" and "No wallet support compiled in!" messages are now logged
earlier during startup.
This commit does not change behavior.
It is easiest to review this change with:
git log -p -n1 -U0
This commit does not change behavior. All it does is pass new function
parameters.
It is easiest to review this change with:
git log -p -n1 -U0 --word-diff-regex=.33@@ -33,6 +34,26 @@
34 #include <utility>
35 #include <vector>
36
37+//! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
-wallet.
490@@ -456,8 +491,32 @@ class WalletImpl : public Wallet
491 CWallet& m_wallet;
492 };
493
494+class WalletClientImpl : public ChainClient
utACK https://github.com/bitcoin/bitcoin/pull/14437/commits/081accb875412f38718f2e40ed7bbdf15e6f4ef8
I’ll do manual testing later today. Summary of reviewed commits:
7e2e62cf7 - Add skeleton chain and client classes
Introduces Chain class which consolidates the node’s interface for use by clients like GUI and wallet. Also introduces ChainClient which is a class that allows the node to manage clients. No methods introduced on either.
8db11dd0b - Pass chain and client variables where needed
Constructs Chain interface (via InitInterfaces) during initialization and passes it to dependent code.
79d579f4e - Remove uses of cs_main in wallet code
Introduces Chain::Lock and uses its acquisition to replace references to cs_main in wallet code. Chain::Lock’s interface is Chain’s, but its use implies that we have a lock on the chainstate (i.e. cs_main).
Eventually this could be replaced with a snapshot of the chainstate to avoid blocking more critical chainstate manipulations (and whatever else requires cs_main) during wallet operations.
081accb87 - Pass chain locked variables where needed
Makes explicit the dependence of various wallet methods on the Chain
interface. Passing around locked_chain implies cs_main acquisition and so
per-function lock annotations are removed. In a later PR, we’ll actually use
locked_chain methods to replace direct access to e.g. chainActive and
mapBlockIndex.
ea961c3d7 - Remove direct node->wallet calls in init.cpp
Replaces wallet-specific initialization with a more general chain_clients
model - allowing for the initialization of non-wallet code which consumes the
chain in a read-only way.
1580@@ -1562,7 +1581,11 @@ bool AppInitMain()
1581 }
1582
1583 // ********************************************************* Step 9: load wallet
1584- if (!g_wallet_init_interface.Open()) return false;
1585+ for (const auto& client : interfaces.chain_clients) {
std::any_of? :-)
Also, I’d prefer if we didn’t leave style feedback at all.
Of course it could use std::any_of, or it could use a while loop, or do while loop, or… It is just a different color up to the author to choose and for everyone else to accept.
@MarcoFalke I should have made clear that my comment was a “nit” :-)
The comparison you’re making isn’t fair: no one would suggest changing this to a while loop, but striving to use STL algorithms were possible is what the C++ Core Guidelines (with editors Bjarne Stroustrup and Herb Sutter) recommends:
ES.1: Prefer the standard library to other libraries and to “handcrafted code”
Code using a library can be much easier to write than code working directly with language features, much shorter, tend to be of a higher level of abstraction, and the library code is presumably already tested. The ISO C++ Standard Library is among the most widely known and best tested libraries. It is available as part of all C++ Implementations.
[…]
0auto sum = accumulate(v, 0.0); // betterbut don’t hand-code a well-known algorithm:
0int max = v.size(); // bad: verbose, purpose unstated 1double sum = 0.0; 2for (int i = 0; i < max; ++i) 3 sum = sum + v[i];
Could use std::any_of? :-)
This I think it would actually have to use std::all_of
re: #14437 (review)
Could use std::any_of? :-)
This PR is already merged so I won’t change this here. But it’s an interesting suggestion. I might prefer all_of over any_of since the bool represents success, and success only happens if all calls succeed.
Note: one drawback to using any/all in a context like this is it makes behavior non-deterministic, because the standard doesn’t guarantee what order the calls will run in or whether calls will be skipped due to short-circuiting: https://stackoverflow.com/questions/47692180/is-stdany-of-required-to-follow-short-circuit-logic
post-merge-utACK https://github.com/bitcoin/bitcoin/pull/14437/commits/ea961c3d7256c66146b4976ab1293db4a628c0de
This is looking good, thanks Russ! 🎉