Remove GetDepthInMainChain dependency on locked chain interface

ariard commented at 3:53 pm on May 1, 2019: member

Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.

I think it’s ready for a first round of review before to get further.

BlockUntilSyncedToCurrent : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.

- AbandonTransaction : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn’t conflicted anymore so we return 0 as tx is again unconfirmed After #16624, we instead rely on Confirmation.

- AddToWalletIfInvolvingMe: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in mapWallet with a height set to zero so we remove check on block_hash.IsNull Already done in #16624

fanquake added the label Refactoring on May 1, 2019

fanquake added the label Wallet on May 1, 2019

DrahtBot commented at 6:27 pm on May 1, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17369 (Refactor: Move encryption code between KeyMan and Wallet by achow101)
#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
#17060 (Cache 26% more coins: Reduce CCoinsMap::value_type from 96 to 76 bytes by martinus)
#16944 (gui: create PSBT with watch-only wallet by Sjors)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#16895 (External signer multisig support by Sjors)
#16688 (log: Add validation interface logging by jkczyz)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

DrahtBot added the label Needs rebase on May 1, 2019

ariard force-pushed on May 2, 2019

DrahtBot removed the label Needs rebase on May 2, 2019

ariard force-pushed on May 6, 2019

DrahtBot added the label Needs rebase on May 7, 2019

ariard force-pushed on May 13, 2019

DrahtBot removed the label Needs rebase on May 13, 2019

DrahtBot added the label Needs rebase on May 14, 2019

ariard force-pushed on May 29, 2019

DrahtBot removed the label Needs rebase on May 29, 2019

DrahtBot added the label Needs rebase on Jun 18, 2019

fanquake added this to the "Chasing Concept ACK" column in a project

fanquake commented at 1:30 am on June 19, 2019: member

Added to the “Chasing Concept ACK” board. @ryanofsky maybe you could give an initial Concept/Approach ACK/NACK ?

ariard commented at 3:15 am on June 19, 2019: member

Yes would be awesome to get a Concept ACK on the approach followed but if it’s not the best one, I’m eager to rework the PR in depth! Going to rebase/fix tests failure soon

in src/wallet/wallet.h:821 in 0284c72ab0 outdated

814@@ -815,6 +815,11 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
815      */
816     uint256 m_last_block_processed GUARDED_BY(cs_wallet);
817 
818+    /* Height of last block processed is used by wallet to know depth of transactions
819+     * without relying on Chain interface beyond asynchronous updates.
820+     */
821+    int m_last_block_processed_height;

ryanofsky commented at 5:38 pm on June 19, 2019:

In commit “Add m_last_block_processed_height field in CWallet” (0284c72ab07362150fd26c86966ea7c7bc6cfed5)

Would be good to initialize this to -1 here for safety (and also for documentation, to be clear that this can be -1).

Would also be good to add GUARDED_BY(cs_wallet) annotation.

in src/wallet/wallet.h:355 in 8a1975586d outdated

350@@ -351,6 +351,11 @@ class CMerkleTx
351      */
352     int nIndex;
353 
354+    /* Refers to height of block against which tx is merkle branch linked to
355+     * An block_height == 0 means that tx isn't yet linked to any block

ryanofsky commented at 5:59 pm on June 19, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (233ae92f2d51333f158810d85d13cf63a1144e6b)

Can we use -1 instead of 0 for this to be consistent with nIndex? Also would be good to note here that when a transaction is conflicted, hashBlock and block_height refer to the earliest block with a conflicting transaction instead of the block containing the transaction.

jnewbery commented at 6:09 pm on July 23, 2019:

The comment says An block_height == 0 in commit Add m_block_height field in CMerkleTx and then changes to A block_height == -1 in Use CWallet::m_last_block_processed_height in GetDepthInMainChain.

It would be better to not change this in the course of the PR and just set it to A block_height == -1 in the first PR.

in src/wallet/wallet.h:430 in 8a1975586d outdated

388@@ -383,9 +389,10 @@ class CMerkleTx
389         READWRITE(hashBlock);
390         READWRITE(vMerkleBranch);
391         READWRITE(nIndex);
392+        READWRITE(m_block_height);

ryanofsky commented at 6:06 pm on June 19, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (233ae92f2d51333f158810d85d13cf63a1144e6b)

Isn’t this going to break existing serialization of transactions in the wallet? I think it’d be best not to change the serialized representation and just make this a memory-only field that gets filled when the wallet is loaded.

ariard commented at 1:52 am on July 9, 2019:

Hmmm filled at startup using getBlockHeight, given we already have hashBlock ? Do you envision the wallet to always query chain state to succeed its startup ?

in src/wallet/wallet.cpp:971 in 8a1975586d outdated

964@@ -965,12 +965,19 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
965         if (!wtxIn.hashUnset() && wtxIn.hashBlock != wtx.hashBlock)
966         {
967             wtx.hashBlock = wtxIn.hashBlock;
968+            wtx.m_block_height = wtxIn.m_block_height;
969+            fUpdated = true;
970+        }
971+        if (wtxIn.hashBlock.IsNull() && !wtx.hashBlock.IsNull()) {

ryanofsky commented at 6:53 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

This logic is getting hard to follow with three repetitive if(hashBlock…) blocks. Now that this is unconditionally updating hashBlock, I think the new logic is equivalent to the following and can be simplified:

0assert(!wtxIn.isAbandoned()); // Incoming transactions should never have special abandoned block hash
1if (wtxIn.hashBlock != wtx.hashBlock) {
2    wtx.hashBlock = wtxIn.hashBlock;
3    wtx.m_block_height = wtxIn.m_block_height;
4    fUpdated = true;
5}

ariard commented at 1:54 am on July 9, 2019:

Updated AddToWallet, hope it’s clearer

in src/wallet/wallet.cpp:4671 in 8a1975586d outdated

4445 
4446     // set the position of the transaction in the block
4447     nIndex = posInBlock;
4448+
4449+    // set tx height
4450+    m_block_height = block_height;

ryanofsky commented at 7:06 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

It looks like a remaining place where m_block_height is still unset is in importprunedfunds. It would probably be good to change that code to call wtx.SetMerkleBranch instead of setting members directly.

ariard commented at 1:58 am on July 9, 2019:

Oooops, I forgot this one. I’ve added a call to SetMerkleBranch in importprunedfunds but had to change its argument to hask for transaction height too. Seems to me similar to the serialization issue, do we want wallet to ask chain the state of its stored transactions every time, even it should be able to infer it from the blocks already connected ?

in src/wallet/wallet.cpp:1164 in 8a1975586d outdated

1160@@ -1152,6 +1161,8 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui
1161             // If the orig tx was not in block/mempool, none of its spends can be in mempool
1162             assert(!wtx.InMempool());
1163             wtx.nIndex = -1;
1164+            // Conflicting height is inherited from ancestor tx

ryanofsky commented at 7:17 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

There’s no particular reason for keeping this height value, is there? I think it’d be cleaner to always represent abandoned transactions with block hash=ABANDON_HASH, block height=-1, index=-1 than to awkwardly make block hash and block height point different places.

ariard commented at 2:20 am on July 9, 2019:

Okay, I did a second round with the following model in head (state, triplet<hashBlock, nIndex, block_height>) : NEW<{}, -1, -1>, UNCONFIRMED<{}, 0, 0>, CONFIRMED<block_hash, posInBlock, block_height>, ABANDON<ABANDON_HASH, -1, -1>, CONFLICTED<hashBlock, -1, conflicting_height>. Do you have one similar, should we noted it somewhere ?

in src/wallet/wallet.cpp:1099 in 8a1975586d outdated

1096@@ -1090,8 +1097,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
1097             CWalletTx wtx(this, ptx);
1098 
1099             // Get merkle branch if transaction was found in a block

ryanofsky commented at 7:28 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

This comment seems out of date since position information is saved regardless. Would just drop it.

ariard commented at 2:00 am on July 9, 2019:

Should be dropped on 1458ded

in src/wallet/wallet.cpp:1100 in 8a1975586d outdated

1096@@ -1090,8 +1097,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
1097             CWalletTx wtx(this, ptx);
1098 
1099             // Get merkle branch if transaction was found in a block
1100-            if (!block_hash.IsNull())
1101-                wtx.SetMerkleBranch(block_hash, posInBlock);
1102+            // Set block_hash to Null, posInBlock to 0, block_height to 0

ryanofsky commented at 7:31 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

I don’t think this additional text belongs here since it is really commenting on the code calling this function, and not explaining what is happening right here. Would just drop this comment.

ariard commented at 2:00 am on July 9, 2019:

Should be dropped on 1458ded

in src/wallet/wallet.cpp:1283 in 8a1975586d outdated

1279@@ -1267,11 +1280,11 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1280 
1281     m_last_block_processed_height += 1;
1282     for (const CTransactionRef& ptx : vtxConflicted) {
1283-        SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */);
1284+        SyncTransaction(ptx, {} /* block hash */, 0 /* block height */, 0 /* position in block */);

ryanofsky commented at 7:46 pm on June 19, 2019:

In commit “Add m_block_height field in CMerkleTx” (8a1975586d3cc7f98961581859fd146a632bbd34)

Not really related to this PR, but I’m surprised this code is treating vtxConflicted transactions as if they are in the mempool and not marking them as conflicted with nIndex set to -1, and block hash and height pointing to the conflicting block. Maybe add a TODO here that we could explicitly mark these transactions as being conflicted (have GetDepthInMainChain return negative values instead of 0).

ariard commented at 2:23 am on July 9, 2019:

Ok, updated on 6289909, maybe I should refine it in its own commit?

in src/wallet/wallet.cpp:103 in 6b7962a650 outdated

 93@@ -94,10 +94,7 @@ static std::set<CWallet*> g_unloading_wallet_set;
 94 // Custom deleter for shared_ptr<CWallet>.
 95 static void ReleaseWallet(CWallet* wallet)
 96 {
 97-    // Unregister and delete the wallet right after BlockUntilSyncedToCurrentChain
 98-    // so that it's in sync with the current chainstate.
 99     wallet->WalletLogPrintf("Releasing wallet\n");
100-    wallet->BlockUntilSyncedToCurrentChain();

ryanofsky commented at 7:53 pm on June 19, 2019:

In commit “Restrain waitForNotificationsIfNewBlocksConnected check to strict tip to avoid race condition” (6b7962a650def5629eb2f897b6ace79a684c0cf0)

This commit seems to be doing two unrelated things: dropping the BlockUntilSyncedToCurrentChain call here and also changing the BlockUntilSyncedToCurrentChain implementation. Would suggest splitting this into two separate commits if possible.

ariard commented at 2:11 am on July 9, 2019:

Splitted between 8a16c67 and 9c07c16

in src/interfaces/chain.h:235 in 6b7962a650 outdated

240@@ -241,8 +241,7 @@ class Chain
241     virtual std::unique_ptr<Handler> handleNotifications(Notifications& notifications) = 0;
242 
243     //! Wait for pending notifications to be processed unless block hash points to the current
244-    //! chain tip, or to a possible descendant of the current chain tip that isn't currently
245-    //! connected.
246+    //! chain tip.

ryanofsky commented at 7:54 pm on June 19, 2019:

In commit “Restrain waitForNotificationsIfNewBlocksConnected check to strict tip to avoid race condition” (6b7962a650def5629eb2f897b6ace79a684c0cf0)

Would suggest renaming waitForNotificationsIfNewBlocksConnected to waitForNotificationsIfTipChanged to accurately describe the new behavior.

ariard commented at 2:06 am on July 9, 2019:

Done, see 9c07c16

in src/wallet/wallet.cpp:4681 in 233ae92f2d outdated

4447@@ -4448,10 +4448,18 @@ void CMerkleTx::SetMerkleBranch(const uint256& block_hash, int posInBlock, int b
4448 
4449 int CMerkleTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
4450 {
4451+    if (pwallet == nullptr)
4452+        return 0;
4453+
4454     if (hashUnset())
4455         return 0;

ryanofsky commented at 8:08 pm on June 19, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (233ae92f2d51333f158810d85d13cf63a1144e6b)

Can we assert(m_block_height > 0) after this point? I’m afraid of another bug like the importprunedfunds one mentioned earlier leaving m_block_height unset and this function returning bogus values.

ariard commented at 2:15 am on July 9, 2019:

Hmmm, I introduced assert(m_block_height >= -1) as we may GetDepthInMainChain on fresh transactions, their hashBlock being unset, their depth should appear as zero.

in src/wallet/wallet.cpp:4458 in 233ae92f2d outdated

4454     if (hashUnset())
4455         return 0;
4456 
4457-    return locked_chain.getBlockDepth(hashBlock) * (nIndex == -1 ? -1 : 1);
4458+    if (nIndex == -1) {
4459+        // Tx is no more conflicted but it's not more confirmed, so return 0

ryanofsky commented at 8:17 pm on June 19, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (233ae92f2d51333f158810d85d13cf63a1144e6b)

This looks right but is the logic is kind of obscure. Maybe you could add an explanatory comment like “If nIndex is -1 and the block hash is set, it means the transaction is conflicted, and that m_block_height will be set to the height of the earliest conflicting block, so can return 0 is that block has been disconnected.”

I think it’d also be good to replace nested if(A) { if (B) return; } with if (A && B) { return; }

ariard commented at 2:12 am on July 9, 2019:

Commented and updated logic as suggested on 573ca36

in src/wallet/wallet.h:1317 in 0284c72ab0 outdated

1312@@ -1308,6 +1313,10 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
1313 
1314     /** Implement lookup of key origin information through wallet key metadata. */
1315     bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
1316+
1317+    int GetLastBlockHeight() const { return m_last_block_processed_height; };

ryanofsky commented at 8:25 pm on June 19, 2019:

In commit “Add m_last_block_processed_height field in CWallet” (0284c72ab07362150fd26c86966ea7c7bc6cfed5)

Can this assert(m_last_block_processed_height >= 0)? This would catch usage errors if there are calls to this function before the wallet is initialized.

ariard commented at 2:13 am on July 9, 2019:

Check should be there in b1ae297

ariard commented at 3:18 am on July 16, 2019:

Note : I’ve seen one Travis failure on feature_reindex.py due to the assert. I wasn’t able to reproduce it on my local dev env, I think it’s caused by Travis really slow setup, but that’s not great, what’s your take on it ?

ryanofsky commented at 8:49 pm on June 19, 2019: member

Concept ACK. I left a lot of suggestions, but overall this looks very good, and it’s great to get rid of all these cs_main locks.

promag commented at 1:54 pm on June 30, 2019: member

Concept ACK.

ariard force-pushed on Jul 9, 2019

ariard commented at 2:24 am on July 9, 2019: member

Answered back to @ryanofsky comments on 1458ded, I’m on cleaning last Travis failures

DrahtBot removed the label Needs rebase on Jul 9, 2019

DrahtBot added the label Needs rebase on Jul 9, 2019

fanquake removed this from the "Chasing Concept ACK" column in a project

ariard force-pushed on Jul 15, 2019

ariard force-pushed on Jul 16, 2019

DrahtBot removed the label Needs rebase on Jul 16, 2019

ariard force-pushed on Jul 16, 2019

ariard force-pushed on Jul 20, 2019

ariard force-pushed on Jul 22, 2019

in src/wallet/wallet.cpp:4679 in 8d84bd5608 outdated

4622-int CMerkleTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
4623+int CMerkleTx::GetDepthInMainChain() const
4624 {
4625+    if (pwallet == nullptr)
4626+        return 0;
4627+    assert(m_block_height >= -1);

jnewbery commented at 4:05 pm on July 23, 2019:

This line is introduced in a too-early commit (Add m_last_block_processed_height field in CWallet) where the member variable m_block_height doesn’t yet exist. The Add m_last_block_processed_height field in CWallet commit doesn’t build because the variable is not declared.

ariard commented at 6:28 pm on July 24, 2019:

Ooops, bad rebase sorry, should be corrected

in src/wallet/rpcdump.cpp:378 in 8d84bd5608 outdated

374@@ -375,6 +375,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
375                 {
376                     {"rawtransaction", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A raw transaction in hex funding an already-existing address in wallet"},
377                     {"txoutproof", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex output from gettxoutproof that contains the transaction"},
378+                    {"height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The height in which transaction has been confirmed"},

jnewbery commented at 4:24 pm on July 23, 2019:

Is this required? the txoutproof includes a block header, which could be used to look up the height in the chain.

ariard commented at 6:36 pm on July 24, 2019:

Hmmm well we could use getBlockHeight again in this function but I didn’t want because was planning to remove this Chain method after we move ScanForWalletTransactions and its logic on the node side.

Maybe that a bit premature, but IMO this RPC should come with all proofs + height without us having to query the chain..

jnewbery commented at 8:30 pm on July 24, 2019:

I don’t like this change for a couple of reasons:

it’s a breaking change for clients, so they’ll need to reimplement their code
the RPC doesn’t do any checking on the height value passed, so it’s possible to get the wallet into some inconsistent state by passing a bad height value.

in src/wallet/wallet.h:877 in 8d84bd5608 outdated

872@@ -864,6 +873,9 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
873     /** Internal database handle. */
874     std::unique_ptr<WalletDatabase> database;
875 
876+    //! Fetches a key from the keypool
877+    bool GetKeyFromPool(CPubKey& key, bool internal = false);

jnewbery commented at 4:38 pm on July 23, 2019:

No need to move this function declaration.

in src/wallet/test/wallet_tests.cpp:259 in 8d84bd5608 outdated

255@@ -251,16 +256,17 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
256 
257     wtx.hashBlock = ::ChainActive().Tip()->GetBlockHash();
258     wtx.nIndex = 0;
259+    wtx.m_block_height = ::ChainActive().Height();

jnewbery commented at 5:28 pm on July 23, 2019:

Can these three lines be replaced by a call to wtx.SetMerkleBranch()?

in src/wallet/wallet.h:376 in 8d84bd5608 outdated

371@@ -372,6 +372,9 @@ class CMerkleTx
372   /** Constant used in hashBlock to indicate tx has been abandoned */
373     static const uint256 ABANDON_HASH;
374 
375+protected:
376+    const CWallet* pwallet;

jnewbery commented at 5:44 pm on July 23, 2019:

In Move CWallet ptr from CWalletTx to CMerkleTx: is there any point in keeping the CMerkleTx/CWalletTx division after this? CMerkleTx is only used as a base class for CWalletTx. It’s been around since the first git commit, and I expect Satoshi thought CMerkleTx could be useful outside the wallet, but now that it’s adding a pointer to the CWallet, it seems redundant to have it as a separate class.

ariard commented at 6:48 pm on July 24, 2019:

See #16451, thanks for going forwad on it :)

in src/wallet/wallet.cpp:1417 in 8d84bd5608 outdated

1413@@ -1422,12 +1414,13 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1414     // to abandon a transaction and then have it inadvertently cleared by
1415     // the notification that the conflicted transaction was evicted.
1416 
1417+    m_last_block_processed_height += 1;

jnewbery commented at 6:24 pm on July 23, 2019:

This height accounting in the wallet scares me slightly. I think that prior to this change, the wallet tolerates any bug in the node that sends duplicate BlockConnected() or BlockDisconnected() calls over the CValidationInterface. After this change, a duplicate BlockConnected notification could cause the wallet to get into a very bad inconsistent state.

Is it possible to update the CValidationInterface to also pass the block height when sending BlockConnected/BlockDisconnected notifications?

ariard commented at 8:01 pm on July 24, 2019:

You’re right, it’s far better to rely on message content than counting the message in itself. To do, I’ve extended BlockDisconnected, for BlockConnected we have CBlockIndex it was just masked between Notification and CWallet

jnewbery commented at 6:26 pm on July 23, 2019: member

I’ve done a quick code review. The main thing that I’d like to see changed is merging the CMerkleTx and CWalletTx classes now that CMerkleTx is accessing wallet data (see discussion here: http://www.erisian.com.au/bitcoin-core-dev/log-2019-07-23.html#l-322)

jnewbery commented at 6:28 pm on July 23, 2019: member

(excellent work, by the way. Great to see some of these locked_chain instances being cleaned up)

ariard force-pushed on Jul 24, 2019

ariard commented at 8:03 pm on July 24, 2019: member

Addressed @jnewbery comments on a53586c, except #15931 (review) where I’m still thinking on it

ariard force-pushed on Jul 24, 2019

DrahtBot added the label Needs rebase on Jul 30, 2019

laanwj referenced this in commit 8241b51504 on Jul 31, 2019

jnewbery commented at 12:07 pm on July 31, 2019: member

#16451 is merged. You should be able to rebase this and remove one of the commits.

sidhujag referenced this in commit 8e647ab7ee on Aug 1, 2019

ariard force-pushed on Aug 6, 2019

ariard commented at 5:19 am on August 6, 2019: member

Rebased without CMerkleTx relative commit

ariard force-pushed on Aug 6, 2019

DrahtBot removed the label Needs rebase on Aug 6, 2019

in src/wallet/wallet.h:846 in 8fa26b2a68 outdated

842+    /* Height of last block processed is used by wallet to know depth of transactions
843+     * without relying on Chain interface beyond asynchronous updates. For safety, we
844+     * initialize it to -1.
845+     */
846+    int m_last_block_processed_height{-1};
847+    GUARDED_BY(cs_wallet);

jnewbery commented at 3:19 pm on August 6, 2019:

I’m not sure if this macro does anything?

ariard commented at 5:04 am on August 7, 2019:

Thanks, I misused some linter scripts I think

(in fact syntax variable {-1} GUARDED_BY(lock) failed on some compilers compare to variable GUARDED_BY(lock) = -1)

in src/wallet/wallet.h:839 in 8fa26b2a68 outdated

835@@ -831,7 +836,14 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
836     uint256 m_last_block_processed GUARDED_BY(cs_wallet);
837 
838     //! Fetches a key from the keypool
839-    bool GetKeyFromPool(CPubKey &key, bool internal = false);
840+    bool GetKeyFromPool(CPubKey& key, bool internal = false);

jnewbery commented at 3:19 pm on August 6, 2019:

Please don’t fixup style from surrounding code. It makes the diff less clean.

jnewbery commented at 3:19 pm on August 6, 2019: member

just a couple of comments on the first commit

jnewbery commented at 3:29 pm on August 6, 2019: member

I suggest you reverse the order of the first two commits:

have one commit that only updates the signature of BlockDiscconected in the validation interface.
the second commit adds m_last_block_processed_height and uses the pindex in BlockConnected/BlockDisconnected to set the height.

Having one way for setting m_last_block_processed_height in the first commit which is replaced by a different way in the second commit makes this confusing for reviewers.

Sjors commented at 3:39 pm on August 6, 2019: member

Concept ACK on reducing reliance on locked_chain. It seems worth adding m_block_height.

I might be more clear to swap the first two (?) commits, thereby introducing m_last_block_processed_height without the += 1 stuff.

Getting a bunch of warnings on macOS along these lines:

 0In file included from wallet/ismine.cpp:12:
 1./wallet/wallet.h:846:5: warning: declaration does not declare anything [-Wmissing-declarations]
 2    GUARDED_BY(cs_wallet);
 3    ^
 4./threadsafety.h:18:23: note: expanded from macro 'GUARDED_BY'
 5#define GUARDED_BY(x) __attribute__((guarded_by(x)))
 6                      ^
 7In file included from wallet/ismine.cpp:12:
 8./wallet/wallet.h:846:5: warning: attribute guarded_by ignored, because it is not attached to a declaration [-Wignored-attributes]
 9./threadsafety.h:18:48: note: expanded from macro 'GUARDED_BY'
10#define GUARDED_BY(x) __attribute__((guarded_by(x)))
11                                               ^
122 warnings generated.

I’m worried that serializing m_block_height in CWalletTx makes wallets non backwards compatible, but I can’t produce a problem (shameless plug for #12134).

ariard force-pushed on Aug 7, 2019

ariard commented at 5:56 am on August 7, 2019: member

Thanks for review @Sjors,

I inverted the 2 top commits at 71a053c to ease review.

About m_block_height in CWalletTx, caching in block height in-memory let us with 2 options at serialization :

serialize in-memory member too, deserialize it at wallet loading
query getBlockHeight by passing hashBlockat LoadToWallet called by ReadKeyValue

I tried also to implement 2nd option, but seems to me really inefficient due to the necessity to add another cs_main lock in LoadWallet for the length of deserialization to preserve locks order.

It may need release notes but I think it’s move us towards less interdependence between wallet and chain. What’s your thoughts ?

ariard force-pushed on Aug 7, 2019

in src/interfaces/chain.h:225 in 71a053ca85 outdated

220@@ -224,8 +221,8 @@ class Chain
221         virtual ~Notifications() {}
222         virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
223         virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
224-        virtual void BlockConnected(const CBlock& block, const std::vector<CTransactionRef>& tx_conflicted) {}
225-        virtual void BlockDisconnected(const CBlock& block) {}
226+        virtual void BlockConnected(const CBlock& block, const std::vector<CTransactionRef>& tx_conflicted, const CBlockIndex* index) {}
227+        virtual void BlockDisconnected(const CBlock& block, const CBlockIndex* index) {}

ryanofsky commented at 7:39 am on August 7, 2019:

It’s fine to use CBlockIndex pointers in CValidationInterface methods, but Chain::Notification methods should just pass int height parameters. Using CBlockIndex here exposes node internals to the wallet and makes #10102 less efficient because CBlockIndex data would have to be serialized and sent across a socket.

ryanofsky commented at 8:12 am on August 7, 2019: member

About m_block_height in CWalletTx, caching in block height in-memory let us with 2 options at serialization :

serialize in-memory member too, deserialize it at wallet loading

query getBlockHeight by passing hashBlockat LoadToWallet called by ReadKeyValue

I tried also to implement 2nd option, but seems to me really inefficient due to the necessity to add another cs_main lock in LoadWallet for the length of deserialization to preserve locks order.

It may need release notes but I think it’s move us towards less interdependence between wallet and chain. What’s your thoughts ?

Definitely should go with option #2. (Sorry, I suggested this earlier with #15931 (review), but I didn’t follow up). Changing wallet serialization makes this PR more complicated to understand and test, and is something that can complicate wallet design going forward by increasing the number of data formats we have to think about staying compatible with.

If there is evidence that 2nd option makes startup time slower (I’d be skeptical because there should be no contention for cs_main and these are in-memory lookups), then serialization could be changed in a more targeted followup PR. It would also be easier to consider other serialization formats in a followup PR, like storing information about blocks separately without changing the transaction format.

jnewbery commented at 12:55 pm on August 7, 2019: member

Changing wallet serialization makes this PR more complicated to understand and test, and is something that can complicate wallet design going forward by increasing the number of data formats we have to think about staying compatible with.

Note that mapValue can be used to change WalletTx serialization in a backwards-compatible way. It’s a key-value store that anything can be written to/read from. Take a look at the ReadOrderPos()/WriteOrderPos() for an example of a parameter being written/read from the mapValue store. @ryanofsky - how can we be sure that if we add a getBlockHeight() call for every tx in the wallet, we won’t introduce a performance regression for very large wallets?

ryanofsky commented at 1:34 pm on August 7, 2019: member

@ryanofsky - how can we be sure that if we add a getBlockHeight() call for every tx in the wallet, we won’t introduce a performance regression for very large wallets?

I don’t think we can be sure without measuring. I would say if there is a performance regression, I think it would be better to handle in a followup PR that’s only changing the serialization and avoiding getBlockHeight() calls, and not changing serialization along with the other things that already have to change here.

The reason I’m skeptical there would be a performance regression is that we’re talking about adding a single map lookup in a loop which is already doing multiple allocations, map insertions, disk reads, and deserialization, and with the whole loop preceding a rescan. I wouldn’t be shocked to see a difference, but I would be surprised to see a difference, and wouldn’t want to make the wallet format more complicated and inconsistent, just for the possibility that a difference might exist.

Note that mapValue can be used to change WalletTx serialization in a backwards-compatible way.

Changing the serialization in a backward compatible way would probably be good, but would create some risk for the future (especially without #12134) that we write new code that assumes data is present and seems to work on new wallets, but then turns out not to work on old wallets.

ryanofsky commented at 2:00 pm on August 7, 2019: member

To be sure, if there is reason to think changing serialization will soup up the wallet, please don’t let me stand in the way. My reaction isn’t “please don’t do this”, just “why wouldn’t you want to mess with this if you didn’t have to?”

ariard commented at 2:23 pm on August 7, 2019: member

Loading large wallets after initialization will need to lock cs_main to deserialize their txn, which maybe a performance regression. Removing chain lock in the wallet would make it better but still be a hit.

But agree, this PR is already big enough, I will change for 2nd option, and let’s talk about safe serialization in another one, if needed and after more thoughts.

ariard commented at 6:43 pm on August 7, 2019: member

@ryanofsky in fact I just realized we may have to take 1st option, bitcoin-wallet binary load a wallet and can’t get its chain interface..

Do you see a 3rd option beyond the 2 I’ve mentioned ?

ariard force-pushed on Aug 7, 2019

ryanofsky commented at 7:26 pm on August 7, 2019: member

@ryanofsky in fact I just realized we may have to take 1st option, bitcoin-wallet binary load a wallet and can’t get its chain interface..

Do you see a 3rd option beyond the 2 I’ve mentioned ?

Just skip this if interfaces::Chain* pointer is null. Bitcoin-wallet won’t do anything that requires knowing the block height of transactions in the wallet.

Sjors commented at 9:03 am on August 8, 2019: member

Just skip this if interfaces::Chain* pointer is null.

That makes sense, though maybe make that more explicit?

ariard force-pushed on Aug 8, 2019

ariard commented at 5:05 pm on August 8, 2019: member

Yes, I’ve beefed up comments on this, tell me it’s enough (see in CWallet::Verify and CWallet::LoadWallet)

The query-chain-to-know-height approach makes compatibility not an issue, and doesn’t block us to go forward with tx height serialization later . A caveat, right now, it has to lock chain for the whole deserialization period to preserve lock order. This inefficiency should go away when we remove chain locks in wallet (hopefully soon).

Please notify a diff in AddToWallet since last review, where we now also check block height of incoming transaction against its old version. Reason is when we load wallet while reindexing, getBlockHeight is not going to find block, so even if tx block hash doesn’t change, we need to correct its height when we get block notification (BlockConnected) after reindex is done.

Also, I didn’t pollute wallet db code path by passing the chain lock, as this one may be null in case of wallet tool and lock can be taken recursively without issue. Instead, we check again in LoadToWallet with the added method hasChain to know if we can query chain state safely.

ariard force-pushed on Aug 8, 2019

ariard commented at 5:38 pm on August 8, 2019: member

Hmm annotated GetDepthInMainChain with a NO_THREAD_SAFETY_ANALYSIS right now to avoid member access into incomplete type (same as GetConflicts and GetAvailableCredit). If anyone has an idea how to resolve them smoothly, I will take it.

ariard force-pushed on Aug 8, 2019

jnewbery commented at 3:18 pm on August 9, 2019: member

I’ve added a GetDepthInMainChain() call back into SubmitMemoryPoolAndRelay() in #16557. I think we probably want to get that log fix in first and rebase this PR on top of that (with the Remove locked_chain from GetDepthInMainChain and its callers commit updated to remove locked_chain from SubmitMemoryPoolAndRelay())

in src/wallet/wallet.h:700 in 0233a55a71 outdated

844@@ -833,6 +845,12 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
845     //! Fetches a key from the keypool
846     bool GetKeyFromPool(CPubKey &key, bool internal = false);
847 
848+    /* Height of last block processed is used by wallet to know depth of transactions
849+     * without relying on Chain interface beyond asynchronous updates. For safety, we
850+     * initialize it to -1.
851+     */
852+    int m_last_block_processed_height GUARDED_BY(cs_wallet) = -1;

jnewbery commented at 4:02 pm on August 9, 2019:

nit: place this new data member immediately next to m_last_block_processed. The comment for both need to emphasize that this block hash/height is for the node’s tip and doesn’t indicate that the wallet has scanned up to this hash/height.

ariard commented at 5:19 pm on August 12, 2019:

Yes, had a comment about it, it doesn’t imply that the wallet has seen all blocks since genesis up until m_last_block_processed_height

jkczyz commented at 4:06 pm on August 19, 2019:

If I’m reading @jnewbery’s comment correctly, part of the documentation for m_last_block_processed and m_last_block_processed_height can be shared.

ariard commented at 3:06 pm on August 20, 2019:

Updated comments

promag commented at 11:35 pm on November 6, 2019:

5aacc3eff15b9b5bdc951f1e274f00d581f63bce

nit, use default member initialize {-1} instead of = -1.

in src/wallet/wallet.cpp:1118 in 0233a55a71 outdated

1122-            fUpdated = true;
1123-        }
1124-        // If no longer abandoned, update
1125-        if (wtxIn.hashBlock.IsNull() && wtx.isAbandoned())
1126-        {
1127+        assert(!wtxIn.isAbandoned()); // Incoming transactions should never have special abandoned block hash

jnewbery commented at 5:01 pm on August 9, 2019:

I don’t think this new assert is right. A user might ‘abandon’ a transaction and it later be relayed or confirmed in a block.

ariard commented at 5:58 pm on August 12, 2019:

After reading again about how to use abandontransaction, I think you’re right, tx might be not in a block or not in your mempool so you can flag is as abandoned. But after this, it can still being included as part of other peers mempools. Removed it, was suggested here initially #15931 (review)

in src/wallet/wallet.cpp:1184 in 0233a55a71 outdated

1172@@ -1180,9 +1173,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
1173     return true;
1174 }
1175 
1176-void CWallet::LoadToWallet(const CWalletTx& wtxIn)
1177+void CWallet::LoadToWallet(CWalletTx& wtxIn)
1178 {
1179     uint256 hash = wtxIn.GetHash();
1180+    Optional<int> block_height = MakeOptional(false, int());
1181+    if (hasChain()) {

jnewbery commented at 5:07 pm on August 9, 2019:

nit: add comment on why a chain might not be available. I think this is only the case in the wallettool.

ariard commented at 6:09 pm on August 12, 2019:

Okay, this point is already mentioned in commit message, Verify and LoadWallet but added also on this place.

jkczyz commented at 6:23 pm on August 19, 2019:

Instead of initializing an Optional<int> then conditionally assigning it based on the result of hasChain(), could you instead define something like chainHeight() to return an Optional<int>? This would hide the chain logic from LoadToWallet().

If not, consider directly assigning 0 to wtxIn.m_block_height and conditionally updating it based on hasChain(). The addition of Optional<int> here just seems to add indirection for the reader.

ariard commented at 2:53 pm on August 20, 2019:

I don’t want to add new method in Chain interface, but yes 2nd suggested alternative avoid the MakeOptional which is good, updated

in src/wallet/wallet.cpp:1252 in 0233a55a71 outdated

1245@@ -1248,9 +1246,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256
1246 
1247             CWalletTx wtx(this, ptx);
1248 
1249-            // Get merkle branch if transaction was found in a block
1250-            if (!block_hash.IsNull())

jnewbery commented at 5:24 pm on August 9, 2019:

I don’d understand why you’ve removed this conditional (I think it’s because you want to set the tx’s block height to 0 (https://github.com/bitcoin/bitcoin/pull/15931#discussion_r301371748), but I don’t understand why you want to do that).

In general, I think it’d be easier if hashBlock, nIndex and m_block_height were added to a struct along with an tx_state enum which could take UNCONFIRMED, CONFIRMED, CONFLICTED or ABANDONED. I think these magic values in nIndex are confusing and should only be dealt with by the serialization/deserialization code.

EDIT: if you do add that struct, consider using it as an argument for SyncTransaction() and AddToWalletIfInvolvingMe().

ariard commented at 9:36 pm on August 15, 2019:

Gone this way, see https://github.com/bitcoin/bitcoin/pull/16624

in src/wallet/wallet.cpp:4683 in 0233a55a71 outdated

4673+    assert(m_block_height >= -1);
4674     if (hashUnset())
4675         return 0;
4676 
4677-    return locked_chain.getBlockDepth(hashBlock) * (nIndex == -1 ? -1 : 1);
4678+    // If nIndex is -1 and the block hash is set, it means the transaction is conflicted, and that m_block_height

jnewbery commented at 7:03 pm on August 9, 2019:

I think an alternative would be to reset the hashBlock, nIndex and m_block_height for all transactions matching the block hash when a BlockDisconnected is received. That would involve iterating through all of the wallet txs to check their hashBlocks. Re-orgs are supposed to be pretty rare, so I think that’s ok. Thoughts?

ariard commented at 9:41 pm on August 15, 2019:

Yes I’ve thought about while working on the rescan logic. I’ve introduced a Rewind callback in the Chain::Notifications, it should call a wallet method to zeroed out cleanly tx hashBlock, nIndex and m_block_height. I’ve proposed to introduce in this future-PR, this one is already wide enough.

in src/wallet/wallet.cpp:99 in 0233a55a71 outdated

 97@@ -98,10 +98,7 @@ static std::set<CWallet*> g_unloading_wallet_set;
 98 // Custom deleter for shared_ptr<CWallet>.
 99 static void ReleaseWallet(CWallet* wallet)
100 {
101-    // Unregister and delete the wallet right after BlockUntilSyncedToCurrentChain

jnewbery commented at 7:09 pm on August 9, 2019:

I don’t understand why this has to be removed. Can you add some more detail to the commit log?

(I think the current commit log “Scheduler thread servicing chain state is stopped before wallet unloading and can’t be useful” isn’t right. ReleaseWallet() is the delete expression for the CWallet shared_ptr, and so is called for example when the wallet is unloaded during runtime.)

ariard commented at 9:59 pm on August 15, 2019:

I’ve run tests again, can’t remember but I had a test staling some point which was solved by this, I think I may have introduce a height error at some point which would false waitForNotificationsIfTipChanged and later corrected it without realizing it would fix this. Drop this commit.

in src/wallet/wallet.cpp:1337 in 0233a55a71 outdated

1334 {
1335     auto locked_chain = chain().lock();
1336     LOCK(cs_wallet);
1337 
1338-    int conflictconfirms = -locked_chain->getBlockDepth(hashBlock);
1339+    int conflictconfirms = -conflicting_height;

jnewbery commented at 7:38 pm on August 9, 2019:

I think this is a change in behaviour. The was previously (current tip height) - (height of block with conflicting tx). This is now just (height of block with conflicting tx).

ariard commented at 6:31 pm on August 12, 2019:

Thanks for this one, in fact after thinking of it, that would be a bug if you have multiple conflicting txn, you would pick up the one with the highest height instead of the highest depth so not the most conflicting one..

jnewbery commented at 7:45 pm on August 9, 2019: member

I really like this PR. I’ve reviewed all the commits and left some inline comments. One other suggested change:

The Restrain waitForNotificationsIfNewBlocksConnected check to strict tip commit log could be a bit clearer. I suggest something like:

waitForNotificationsIfNewBlocksConnected() would previously return immediately if the best block processed by the wallet was a descendant of the node’s tip. This would mean that during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.

Instead, change waitForNotificationsIfNewBlocksConnected() to waitForNotificationsIfTipChanged() and only return early if the best block processed by the wallet is the node’s tip.

I’d also recommend updating the text in the PR description.

in src/validationinterface.h:117 in 3a04cd6776 outdated

113@@ -114,7 +114,7 @@ class CValidationInterface {
114      *
115      * Called on a background thread.
116      */
117-    virtual void BlockDisconnected(const std::shared_ptr<const CBlock> &block) {}
118+    virtual void BlockDisconnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) {}

promag commented at 10:01 pm on August 15, 2019:

3a04cd6776f7f1ec86cad20de90c9fbb24655536

nit, could drop & from 1st argument, it was discussed elsewhere that smart pointers and iterators should be used as value.

jnewbery commented at 9:54 pm on August 19, 2019:

Please don’t do this. Trying to ‘fix’ surrounding code in a PR makes the diff bigger and more confusing for reviewers.

in src/wallet/wallet.cpp:1454 in 3d32a691d2 outdated

1442@@ -1442,9 +1443,12 @@ void CWallet::BlockDisconnected(const CBlock& block, int height)
1443     auto locked_chain = chain().lock();
1444     LOCK(cs_wallet);
1445 
1446+    m_last_block_processed_height = height - 1;
1447     for (const CTransactionRef& ptx : block.vtx) {
1448         SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */);
1449     }
1450+
1451+    m_last_block_processed = block.hashPrevBlock;

promag commented at 10:05 pm on August 15, 2019:

3d32a691d2df4c73a39a64a5d331c29b5eb94213

Is this a bugfix?

ariard commented at 2:46 pm on August 19, 2019:

Yes it was a bugfix compare to a53586c at least, it became obvious only after switching to query the chain based on block hash at wallet loading!

ariard force-pushed on Aug 15, 2019

in src/wallet/wallet.h:1374 in b75a18f8a3 outdated

1369+    {
1370+        AssertLockHeld(cs_wallet);
1371+        return m_last_block_processed_height;
1372+    };
1373+    /** Set last block processed height, currently only use in unit test */
1374+    void SetLastBlockHeight(int block_height) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

promag commented at 10:18 pm on August 15, 2019:

3d32a691d2df4c73a39a64a5d331c29b5eb94213

Why isn’t this named SetLastBlockProcessedHeight?

Anyway I think the variable should be m_chain_tip_height. Also, I think now m_last_block_processed should be improved, like:

0    uint256 m_chain_tip_hash GUARDED_BY(cs_wallet);
1    int m_chain_tip_height GUARDED_BY(cs_wallet){-1};

and both should be set at once.

ariard commented at 2:31 pm on August 19, 2019:

Agree for the name of method and set at once, but not sure for variables. I think it would be confusing, as intent of this PR is to let the wallet have its own view of what chain tip is after processing block from validation interface. So real chain tip and the wallet one may diverge.

promag commented at 2:40 pm on August 19, 2019:

Yeah these would be from the wallet POV.

in src/wallet/wallet.cpp:1074 in b75a18f8a3 outdated

1424@@ -1425,6 +1425,7 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1425     // to abandon a transaction and then have it inadvertently cleared by
1426     // the notification that the conflicted transaction was evicted.
1427 
1428+    m_last_block_processed_height = height;

jkczyz commented at 4:12 pm on August 19, 2019:

Would it be possible to set m_last_block_processed_height when setting m_last_block_processed below? Or does the intermediary calls rely on the value of either?

Similarly for CWallet::BlockDisconnected.

ariard commented at 3:13 pm on August 20, 2019:

Yes it has to be before, but IIRC not the m_last_block_processed assignment. Moved it near to m_last_block_processed_height.

promag commented at 11:31 pm on November 6, 2019:

5aacc3eff15b9b5bdc951f1e274f00d581f63bce

Yes it has to be before

Could leave a comment explaining why?

Moved it near to m_last_block_processed_height.

Why? If there’s no reason to change then I’d it keep where it was.

in src/wallet/wallet.h:492 in a9b7592ce9 outdated

488@@ -488,6 +489,11 @@ class CWalletTx
489      */
490     int nIndex;
491 
492+    /* Refers to height of block against which tx is merkle branch linked to

jkczyz commented at 6:05 pm on August 19, 2019:

Nits:

Consider rewording for succinctness and clarity: Height of block against which tx is linked via a merkle branch.
End each sentence with a period as they tend to blend when reading the comment.

ariard force-pushed on Aug 20, 2019

ariard commented at 3:23 pm on August 20, 2019: member

Updated at f766fe4, including @promag and @jkczyz feedbacks.

ariard force-pushed on Aug 20, 2019

ariard commented at 7:37 pm on August 20, 2019: member

Rebased at 7427cbf, due to silent build failures not reported by merge conflict detection.

dergigi commented at 2:03 pm on August 21, 2019: none

Tried to compile commit 7427cbf to prepare for the PR review club, and I’m getting an error when running make:

0  CXX      qt/test/test_bitcoin_qt-wallettests.o
1qt/test/wallettests.cpp: In function ‘void {anonymous}::TestGUI()’:
2qt/test/wallettests.cpp:145:17: error: ‘using element_type = class CWallet’ {aka ‘class CWallet’} has no member named ‘SetLastBlockProcessedHeight’; did you mean ‘SetLastBlockProcessed’?
3  145 |         wallet->SetLastBlockProcessedHeight(105, ::ChainActive().Tip()->GetBlockHash());
4      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
5      |                 SetLastBlockProcessed
6make[2]: *** [Makefile:11357: qt/test/test_bitcoin_qt-wallettests.o] Error 1
7make[2]: *** [Makefile:11357: qt/test/test_bitcoin_qt-wallettests.o] Error 1
8make[1]: *** [Makefile:13676: all-recursive] Error 1
9make: *** [Makefile:774: all-recursive] Error 1

Not sure if I’m doing anything wrong. I am able to compile master without any errors.

ariard force-pushed on Aug 21, 2019

ariard commented at 2:17 pm on August 21, 2019: member

@dergigi sorry for that, skipped qt test build when I replied promag comment yesterday, shouldn’t have. Updated at 15100ee with fix

meshcollider referenced this in commit 5e202382a9 on Sep 5, 2019

DrahtBot added the label Needs rebase on Sep 5, 2019

sidhujag referenced this in commit 4c9cc729fc on Sep 10, 2019

ryanofsky commented at 6:13 pm on October 10, 2019: member

I was looking over this again and think it’s a really nice change. It’d be good to rebase now that #16624 is merged. It would also help with #16426 which depends on this PR.

ariard force-pushed on Oct 14, 2019

ariard commented at 6:08 am on October 14, 2019: member

Thanks for the push @ryanofsky, and sorry for the delay for following-up on this PR. Also added nits leftover from #16624

I spent a bit more time thinking how we handle conflicts and found that Confirmation status isn’t correct for children of conflicted txn, if conflicting tx get removed we unconfirm well the parent conflicted tx but not its child. Test here : https://github.com/ariard/bitcoin/tree/2019-10-conflicted-txn, will correct in a later PR to avoid delaying more this one.

DrahtBot removed the label Needs rebase on Oct 14, 2019

DrahtBot added the label Needs rebase on Oct 15, 2019

in src/wallet/wallet.cpp:1182 in 6593366d38 outdated

1182             wtxIn.setUnconfirmed();
1183             wtxIn.m_confirm.hashBlock = uint256();
1184+            wtxIn.m_confirm.block_height = 0;
1185             wtxIn.m_confirm.nIndex = 0;
1186+        } else {
1187+            wtxIn.m_confirm.block_height = (block_height) ? *block_height : 0;

ryanofsky commented at 6:39 pm on October 16, 2019:

In commit “Add block_height field in struct Confirmation” (6593366d385e1c299a675af58ddd2138b3bd01e5)

What’s the reason for the && !block_height check? Is it to handle the case where a transaction was reorged out while online and then reconfirmed while offline? It seems like like nIndex will still incorrectly be 0 in this case, though the block height and hash will remain set. Not sure the 0 matters or needs to be fixed by a later rescan.

Would be good to add a comment about the && !block_height condition or any case where incorrect information might be set now that needs to be fixed by rescan later.

ariard commented at 3:34 pm on October 23, 2019:

Updated the comment, which was saying the inverse of what was done in code, if tx was confirmed (or conflicted by another confirmed tx) and a reorg happens we should reset tx status to unconfirmed. The case where a transaction was reorged out while online and then reconfirmed while offline is covered by the rescan logic of wallet creation IMO. Reason for the && !block_height is to check that we don’t find anymore the block based on the tx hashBlock stored, hint of a reorg.

ryanofsky commented at 5:05 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Makes sense! I think logic would be less confusing it we got rid of the repeated height checks, though. Would suggest

 0Optional<int> block_height = locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock);
 1if (block_height) {
 2    // Update cached block height variable since it not stored in the
 3    // serialized transaction.
 4    wtxIn.m_confirm.block_height = *block_height;
 5} else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) {
 6    // If tx block (or conflicting block) was reorged out of chain
 7    // while the wallet was shut down, change tx status to UNCONFIRMED
 8    // and reset block height, hash, and index. ABANDONED tx don't have
 9    // associated blocks and don't need to be updated.
10    wtxIn.setUnconfirmed();
11    wtxIn.m_confirm.hashBlock = uint256();
12    wtxIn.m_confirm.block_height = 0;
13    wtxIn.m_confirm.nIndex = 0;
14}

in src/wallet/wallet.h:982 in 6593366d38 outdated

977@@ -976,6 +978,8 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
978     /** Interface for accessing chain state. */
979     interfaces::Chain& chain() const { assert(m_chain); return *m_chain; }
980 
981+    /** Interface to assert chain access. */
982+    bool hasChain() const { return m_chain != nullptr; }

ryanofsky commented at 7:01 pm on October 16, 2019:

In commit “Add block_height field in struct Confirmation” (6593366d385e1c299a675af58ddd2138b3bd01e5)

New method seems unused

ariard commented at 3:34 pm on October 23, 2019:

Removed

in src/interfaces/chain.cpp:361 in 25f7a78b95 outdated

352+    void waitForNotificationsIfTipChanged(const uint256& old_tip) override
353     {
354         if (!old_tip.IsNull()) {
355             LOCK(::cs_main);
356             if (old_tip == ::ChainActive().Tip()->GetBlockHash()) return;
357-            CBlockIndex* block = LookupBlockIndex(old_tip);

ryanofsky commented at 7:12 pm on October 16, 2019:

In commit “Restrain waitForNotificationsIfNewBlocksConnected check to strict tip” (25f7a78b952f172457b75b7fa2dc889215e0703b)

Can the commit message say what motivated this change or what it has to do with the other changes in the PR? It mentions changing RPCs to not indicate reorged transactions are confirmed, but this seems like preexisting behavior not related to the PR.

ariard commented at 3:38 pm on October 23, 2019:

Updated commit message, due to the switch to height tracking based on callbacks, if we don’t wait for the whole callbacks queue being drawn because the last block hash processed by the wallet may be a descendant of the current tip, we may miss to process BlockDisconnected and so get tx deptch inconsistencies.

in src/wallet/wallet.cpp:4673 in 901fa3a3a5 outdated

4668@@ -4669,10 +4669,13 @@ void CWalletTx::SetConf(Status status, const uint256& block_hash, int posInBlock
4669 
4670 int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
4671 {
4672+    if (pwallet == nullptr)
4673+        return 0;

ryanofsky commented at 7:23 pm on October 16, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (901fa3a3a5fdac52ec6df69acb744abd99a6cb99)

Can this return be eliminated? It seems risky to silently return incorrect information about a transaction if there was an iinitialization error at an earlier point in the code. It might be better to assert.

ariard commented at 3:38 pm on October 23, 2019:

Switch to asserting.

in src/wallet/wallet.h:648 in 901fa3a3a5 outdated

644+    // annotation "EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)". The annotation
645+    // "NO_THREAD_SAFETY_ANALYSIS" was temporarily added to avoid having to
646+    // resolve the issue of member access into incomplete type CWallet. Note
647+    // that we still have the runtime check "AssertLockHeld(pwallet->cs_wallet)"
648+    // in place.
649+    int GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const NO_THREAD_SAFETY_ANALYSIS;

ryanofsky commented at 7:35 pm on October 16, 2019:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (901fa3a3a5fdac52ec6df69acb744abd99a6cb99)

This TODO is fine and no need to change things here, but I think GetDepthInMainChain and related methods should probably be standalone utility functions or CWallet methods instead of CWalletTx members. I guess other ways to support adding the lock annotation would be to move the CWalletTx class declaration after CWallet, or pass a new CWallet& argument.

ariard commented at 3:39 pm on October 23, 2019:

Ok, I think will let it as it is for now and will try to pass a CWallet& argment in a future PR scoped only on this.

ryanofsky approved

ryanofsky commented at 8:12 pm on October 16, 2019: member

Light code review ACK 4f5d03a4c68e0576a85add4254f4f4470897a0fa. I reviewed the whole thing and think it looks good, but did have some questions below.

re: #15931 (comment)

found that Confirmation status isn’t correct for children of conflicted txn

IIUC this is current behavior, independent of the PR?

ariard force-pushed on Oct 23, 2019

ariard commented at 3:40 pm on October 23, 2019: member

Thanks for review @ryanofsky, rebased and addressed your comment at 089189e

IIUC this is current behavior, independent of the PR?

AFAIK, we don’t have logic to undo conflicts on child txn so it was already even there before #16624.

in src/wallet/test/wallet_tests.cpp:310 in de8fe7d717 outdated

306@@ -307,7 +307,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64
307         wallet.AddToWallet(wtx);
308     }
309     if (block) {
310-        wtx.SetConf(CWalletTx::Status::CONFIRMED, block->GetBlockHash(), 0);
311+        wtx.SetConf(CWalletTx::Status::CONFIRMED, block->GetBlockHash(), 0, 0);

ryanofsky commented at 4:26 pm on October 23, 2019:

In commit “Pass block height in Chain::BlockConnected/Chain::BlockDisconnected” (82c591ea35ac918147ca66444ae608b8385abd7c)

Would it be more correct to pass the block height here instead of 0?

in src/wallet/wallet.h:506 in de8fe7d717 outdated

504+     * where they instead point to block hash and block height of the deepest conflicting tx.
505      */
506     struct Confirmation {
507         Status status = UNCONFIRMED;
508         uint256 hashBlock = uint256();
509+        int block_height = 0;

ryanofsky commented at 4:38 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Perhaps something for a followup PR, but I wonder if some getBlockHeight calls can be removed now and replaced with block_height accesses:

https://github.com/bitcoin/bitcoin/blob/089189ef99effe79bb2b71b9248f8aa419079b9a/src/interfaces/wallet.cpp#L66 https://github.com/bitcoin/bitcoin/blob/089189ef99effe79bb2b71b9248f8aa419079b9a/src/wallet/wallet.cpp#L4049

ariard commented at 7:10 pm on October 24, 2019:

I let it be, need thoughts maybe to retrieve more as same time

in src/wallet/wallet.cpp:1175 in de8fe7d717 outdated

1174-        if (locked_chain && !locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock)) {
1175+    if (locked_chain) {
1176+        Optional<int> block_height = locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock);
1177+        // If tx (or conflicting tx) has been reorged out of chain while wallet being shutdown
1178+        // change tx status to UNCONFIRMED and reset hashBlock/nIndex. ABANDONED tx should have
1179+        // a block_height unset but shouldn't see its status updated.

ryanofsky commented at 4:49 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

I think this comment should explicitly mention what you said about “The case where a transaction was reorged out while online and then reconfirmed while offline is covered by the rescan logic,” or at least mention generally that these transaction statuses aren’t final and will be patched up by a rescan after the load.

in src/wallet/wallet.cpp:1173 in de8fe7d717 outdated

1172-    // change tx status to UNCONFIRMED and reset hashBlock/nIndex.
1173-    if (!wtxIn.m_confirm.hashBlock.IsNull()) {
1174-        if (locked_chain && !locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock)) {
1175+    if (locked_chain) {
1176+        Optional<int> block_height = locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock);
1177+        // If tx (or conflicting tx) has been reorged out of chain while wallet being shutdown

ryanofsky commented at 4:49 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

s/being shutdown/was shutdown/ for clarity

DrahtBot removed the label Needs rebase on Oct 23, 2019

in src/wallet/wallet.cpp:951 in de8fe7d717 outdated

1309@@ -1303,7 +1310,6 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui
1310         if (currentconfirm == 0 && !wtx.isAbandoned()) {
1311             // If the orig tx was not in block/mempool, none of its spends can be in mempool
1312             assert(!wtx.InMempool());
1313-            wtx.m_confirm.nIndex = 0;

ryanofsky commented at 8:57 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Note: this is one of the previous PR cleanups mentioned in the commit description: #16624 (review)

in src/wallet/wallet.cpp:1401 in de8fe7d717 outdated

1397@@ -1391,7 +1398,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status stat
1398 void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) {
1399     auto locked_chain = chain().lock();
1400     LOCK(cs_wallet);
1401-    SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, {} /* block hash */, 0 /* position in block */);
1402+    SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, {} /* block hash */, 0 /* position in block */, 0 /* block height */);

ryanofsky commented at 9:06 pm on October 23, 2019:

In commit “Pass block height in Chain::BlockConnected/Chain::BlockDisconnected” (82c591ea35ac918147ca66444ae608b8385abd7c)

Order of arguments here is wrong. Block height come before position in block.

ariard commented at 7:12 pm on October 24, 2019:

Added commit d0c4ce6 : Replace CWalletTx:SetConf by Confirmation initialization list, should make things less error-prone.

in src/wallet/wallet.h:803 in de8fe7d717 outdated

799@@ -798,10 +800,10 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
800      * Abandoned state should probably be more carefully tracked via different
801      * posInBlock signals or by checking mempool presence when necessary.
802      */
803-    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
804+    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool fUpdate, int block_height) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

ryanofsky commented at 9:14 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Could you move the block_height argument after block_hash to be consistent with SyncTransaction argument order and Confirmation struct order?

Alternately, it possible, it would be really nice to combine the 4 status/hash/height/index arguments into a single CWalletTx::Confirmation struct argument, because having 4 int arguments in a row makes bugs more likely.

ariard commented at 7:12 pm on October 24, 2019:

See d0c4ce6

in src/wallet/wallet.h:633 in de8fe7d717 outdated

629@@ -630,7 +630,7 @@ class CWalletTx
630     // in place.
631     std::set<uint256> GetConflicts() const NO_THREAD_SAFETY_ANALYSIS;
632 
633-    void SetConf(Status status, const uint256& block_hash, int posInBlock);
634+    void SetConf(Status status, const uint256& block_hash, int posInBlock, int block_height);

ryanofsky commented at 9:15 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Could you move the block_height argument after block_hash to be consistent with SyncTransaction argument order and Confirmation struct order?

in src/wallet/wallet.cpp:4659 in de8fe7d717 outdated

4655     m_confirm.nIndex = posInBlock;
4656 }
4657 
4658 int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
4659 {
4660+    assert(m_confirm.block_height >= -1);

ryanofsky commented at 9:24 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

This seems harmless, but it’s unclear why it’s part of this commit. Is this line meant to added to another commit where the block_height is actually used? Also, I don’t see where -1 would be set or allowed for this.

ariard commented at 7:13 pm on October 24, 2019:

Removed, was there to be sure block_height was initialized in a previous version, now it has to be with Confirmation default initialization

in src/wallet/wallet.h:500 in de8fe7d717 outdated

495@@ -496,13 +496,14 @@ class CWalletTx
496         ABANDONED
497     };
498 
499-    /* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed.
500-     * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state
501-     * where they instead point to block hash and index of the deepest conflicting tx.
502+    /* Confirmation includes tx status and a triplet of {block hash/block height/tx index in block} at which tx has been confirmed.
503+     * This triplet is whole 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state

ryanofsky commented at 9:28 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Maybe replace “hasn’t confirmed yet” with “is unconfirmed or abandoned” to be more specific and include mention of the abandoned state.

in src/wallet/wallet.h:409 in de8fe7d717 outdated

546@@ -546,7 +547,6 @@ class CWalletTx
547          * compatibility (pre-commit 9ac63d6).
548          */
549         if (serializedIndex == -1 && m_confirm.hashBlock == ABANDON_HASH) {
550-            m_confirm.hashBlock = uint256();

ryanofsky commented at 9:30 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Note: this is another cleanup from the previous pr.

in src/wallet/wallet.h:1152 in 9161f0bf03 outdated

1410@@ -1405,6 +1411,20 @@ class CWallet final : public FillableSigningProvider, private interfaces::Chain:
1411 
1412     /** Implement lookup of key origin information through wallet key metadata. */
1413     bool GetKeyOrigin(const CKeyID& keyid, KeyOriginInfo& info) const override;
1414+
1415+    /** Get last block processed height */
1416+    int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
1417+    {
1418+        AssertLockHeld(cs_wallet);
1419+        return m_last_block_processed_height;

ryanofsky commented at 9:39 pm on October 23, 2019:

In commit “Add m_last_block_processed_height field in CWallet” (9161f0bf0321e90e0aaeeafc58c5ea6f97ebb1e2):

Would assert(m_last_block_processed_height >= 0) to make sure we’re past initialization and don’t wind up with crazy depth values.

ariard commented at 7:13 pm on October 24, 2019:

Done

in src/wallet/wallet.cpp:1426 in de8fe7d717 outdated

1422@@ -1416,7 +1423,7 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction
1423     m_last_block_processed_height = height;
1424     m_last_block_processed = block_hash;
1425     for (size_t i = 0; i < block.vtx.size(); i++) {
1426-        SyncTransaction(block.vtx[i], CWalletTx::Status::CONFIRMED, block_hash, i);
1427+        SyncTransaction(block.vtx[i], CWalletTx::Status::CONFIRMED, block_hash, m_last_block_processed_height, i);

ryanofsky commented at 9:43 pm on October 23, 2019:

In commit “Add block_height field in struct Confirmation” (de8fe7d717042823f2aa429be2a260904a10b42e)

Can you replace m_last_block_processed_height with height on this line? It’s slightly worrying to depend on the wallet member if it might get updated for some reason, and also better to be more consistent with previous block_hash argument.

ryanofsky commented at 9:48 pm on October 23, 2019: member

Started closer review (will update list below with progress).

82c591ea35ac918147ca66444ae608b8385abd7c Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (1/7)
9161f0bf0321e90e0aaeeafc58c5ea6f97ebb1e2 Add m_last_block_processed_height field in CWallet (2/7)
de8fe7d717042823f2aa429be2a260904a10b42e Add block_height field in struct Confirmation (3/7)
acae8e41fbcec5ee969d511e736e9238970aa087 Restrain waitForNotificationsIfNewBlocksConnected check to strict tip (4/7)
8033be2b111af4c741d5c0ea715381976abd4c15 Use CWallet::m_last_block_processed_height in GetDepthInMainChain (5/7)
360237eaa9e4ad11865c00407a4ef444df841923 Remove locked_chain from GetDepthInMainChain and its callers (6/7)
089189ef99effe79bb2b71b9248f8aa419079b9a Remove getBlockDepth method from Chain::interface (7/7)

DrahtBot added the label Needs rebase on Oct 24, 2019

fanquake added this to the "Blockers" column in a project

ryanofsky approved

ryanofsky commented at 7:05 pm on October 24, 2019: member

Code review ACK 089189ef99effe79bb2b71b9248f8aa419079b9a. I finished reviewing the last few commits, which were all straightforward, and I didn’t see anything I’d change about them. I forgot to mention this yesterday, but please ignore my review suggestions from then if they seem like too much work, or not worth the effort. I think this change should basically be ready to merge in its current form after a rebase and a few re-acks.

ariard force-pushed on Oct 24, 2019

ariard commented at 7:15 pm on October 24, 2019: member

Thanks @ryanofsky , addressed most of your comments, specially #15931 (review)a and adding commit d0c4ce6 to wrap all confirmations values directly in BlockConnected/BlockDisconnected.

Rebased at 75c276e

DrahtBot removed the label Needs rebase on Oct 24, 2019

in src/wallet/rpcdump.cpp:384 in d0c4ce686b outdated

382@@ -383,7 +383,8 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
383         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
384     }
385 
386-    wtx.SetConf(CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), txnIndex);
387+    CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), txnIndex);
388+    wtx.m_confirm = confirm;

ryanofsky commented at 9:06 pm on October 24, 2019:

In commit “Replace CWalletTx::SetConf by Confirmation initialization list” (d0c4ce686ba388b9fefff2b153358e33a042ee80)

Not important, but might want to avoid creating named variable with:

0wtx.m_confirm = CWalletTx::Confirmation{CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), txnIndex};

Similarly with all the other temporary confirm variables below.

promag commented at 11:59 pm on November 6, 2019:

9700fcb47feca9d78e005b8d18b41148c8f6b25f

You can also drop Confirmation constructor and do

0wtx.m_confirm = {CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), int(txnIndex)};

and

0    SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* hash = */ {}, /* index = */ 0);

in src/wallet/wallet.cpp:863 in d0c4ce686b outdated

1192@@ -1193,19 +1193,19 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn)
1193     }
1194 }
1195 
1196-bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool fUpdate)
1197+bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool fUpdate)

ryanofsky commented at 9:08 pm on October 24, 2019:

In commit “Replace CWalletTx::SetConf by Confirmation initialization list” (d0c4ce686ba388b9fefff2b153358e33a042ee80)

Not a big deal, but probably better to pass with reference type const CWalletTx::Confirmation& to avoid copies when this is called. Similarly for other functions taking confirm arguments.

ryanofsky approved

ryanofsky commented at 9:18 pm on October 24, 2019: member

Code review ACK 75c276ea9bcb95f549f872ad365313ec7bd76da3. Changes since last review: rebase, space formatting stuff, requested new assert in GetLastBlockHeight, new commit passing Confirm structs instead of individual properties, and various other requested changes. Looks very good. I left two suggestions for the new commit, but feel free to ignore

laanwj commented at 12:52 pm on October 25, 2019: member

Concept ACK. I like the direction of this.

laanwj added this to the milestone 0.20.0 on Oct 25, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

ariard force-pushed on Oct 30, 2019

ryanofsky approved

ryanofsky commented at 5:37 pm on October 30, 2019: member

Code review ACK b2c68eb3cdc851d4d1a3bb6a8a64397907e22618. No actual changes since last review, just rebase due to adjacent conflicts with #17260

DrahtBot removed the label Needs rebase on Oct 30, 2019

jonatack commented at 5:01 pm on October 31, 2019: member

Code review in progress. Built b2c68eb3cdc851d4d1a3bb6a8a64397907e22618, ran tests, running bitcoind w/logging for a few hours.

DrahtBot added the label Needs rebase on Nov 5, 2019

ryanofsky commented at 5:55 pm on November 5, 2019: member

This has had a decent amount of code review but only one code review ack.

If previous reviewers want to take another look and add ACKs, it would be very helpful. The PR is smaller and simpler than it was originally, and most of the changes (especially in the last few commits) are actually trivial. A lot of improvments can build on this PR to make the wallet more asynchronous, clean up rescanning, etc.

Review history:

ryanofsky review and concept ack 2019-06-19 #15931#pullrequestreview-251859358 promag concept ack 2019-06-30 #15931#pullrequestreview-256075977 jnewbery review 2019-07-23 #15931#pullrequestreview-265517375 jnewbery review 2019-08-06 #15931#pullrequestreview-271423875 sjors review and concept ack 2019-08-06 #15931 (comment) jnewbery review 2019-08-09 #15931#pullrequestreview-273228460 promag review 2019-08-15 #15931#pullrequestreview-275692610 jkczyz review 2019-08-19 #15931#pullrequestreview-276662148 ryanofsky review 2019-10-23 #15931#pullrequestreview-306024592 ryanofsky review ack 2019-10-24 #15931#pullrequestreview-306864462 laanwj concept ack 2019-10-25 #15931 (comment) ryanofsky reack 2019-10-30 #15931#pullrequestreview-309395041

Pass block height in Chain::BlockConnected/Chain::BlockDisconnected

To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.

This new parameter will be use in the following commit to establish
wallet height.

10b4729e33

Add m_last_block_processed_height field in CWallet

At BlockConnected/BlockDisconnected, we rely on height of block
itself to know current height of wallet

5aacc3eff1

in src/wallet/wallet.cpp:1860 in b2c68eb3cd outdated

1859@@ -1840,7 +1860,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const
1860     if (!locked_chain.checkFinalTx(*tx)) {

ryanofsky commented at 6:08 pm on November 5, 2019:

Note for future followup: I believe we could call IsFinalTx instead of checkFinalTx so locked_chain argument no longer needs to be passed to CWalletTx::IsTrusted

DrahtBot removed the label Needs rebase on Nov 5, 2019

ariard force-pushed on Nov 5, 2019

in src/wallet/rpcwallet.cpp:1601 in dd3cb38041 outdated

1596@@ -1598,7 +1597,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1597     for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
1598         CWalletTx tx = pairWtx.second;
1599 
1600-        if (depth == -1 || abs(tx.GetDepthInMainChain(*locked_chain)) < depth) {

ryanofsky commented at 8:32 pm on November 5, 2019:

In commit “Remove locked_chain from GetDepthInMainChain and its callers” (dd3cb3804126abdba8684cb965d2e1dc3766b3fd)

Appears to be a mistake during rebase: lost abs() call

ariard commented at 8:44 pm on November 5, 2019:

Thanks to spot it, was introducted in 436ad43, corrected rebase mistake!

ariard force-pushed on Nov 5, 2019

ryanofsky approved

ryanofsky commented at 9:22 pm on November 5, 2019: member

Code review ACK 8a11ab3ff4cba6456021e46813f530c22ada8ac1. No changes since previous review other than rebase

promag commented at 9:39 pm on November 5, 2019: member

Sure Russell, I’ll review.

in src/wallet/rpcdump.cpp:372 in 058eb3a0ee outdated

368     if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) {
369 
370         auto locked_chain = pwallet->chain().lock();
371-        if (locked_chain->getBlockHeight(merkleBlock.header.GetHash()) == nullopt) {
372+        height = locked_chain->getBlockHeight(merkleBlock.header.GetHash());
373+        if (height == nullopt) {

jkczyz commented at 10:03 pm on November 5, 2019:

This code would benefit if organized using a guard clause.

0if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) != merkleBlock.header.hashMerkleRoot) {
1    throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
2}
3
4auto locked_chain = pwallet->chain().lock();
5Optional<int> height = locked_chain->getBlockHeight(merkleBlock.header.GetHash());
6if (height == nullopt) {
7    ...
8}

This would remove nesting, allow height and to be initialized where it is declared, and move the exception currently in the else clause closer to where the condition has been violated. It would also allow txnIndex to be declared and defined in one place.

in src/wallet/wallet.h:366 in 8a11ab3ff4 outdated

367-        Status status = UNCONFIRMED;
368-        uint256 hashBlock = uint256();
369-        int nIndex = 0;
370+        Status status;
371+        uint256 hashBlock;
372+        int block_height;

jnewbery commented at 10:12 pm on November 5, 2019:

It looks like you add this member one commit too early (Replace CWalletTx::SetConf by Confirmation initialization list instead of Add block_height field in struct Confirmation), which means that all of the Confirmation objects in that commit are setting height incorrectly. Not sure if that would break anything with git bisect. Can you rearranage those commits?

in src/wallet/wallet.h:359 in 8a11ab3ff4 outdated

355@@ -356,14 +356,16 @@ class CWalletTx
356         ABANDONED
357     };
358 
359-    /* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed.
360-     * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state
361-     * where they instead point to block hash and index of the deepest conflicting tx.
362+    /* Confirmation includes tx status and a triplet of {block hash/block height/tx index in block} at which tx has been confirmed.

jnewbery commented at 10:15 pm on November 5, 2019:

nit (not worth fixing here, but good to keep in mind for future PRs): these very long (>120 char) lines make looking at diffs quite difficult. Both this comment block and the Confirmation constructor could be line-wrapped.

in src/wallet/wallet.h:360 in 8a11ab3ff4 outdated

355@@ -356,14 +356,16 @@ class CWalletTx
356         ABANDONED
357     };
358 
359-    /* Confirmation includes tx status and a pair of {block hash/tx index in block} at which tx has been confirmed.
360-     * This pair is both 0 if tx hasn't confirmed yet. Meaning of these fields changes with CONFLICTED state
361-     * where they instead point to block hash and index of the deepest conflicting tx.
362+    /* Confirmation includes tx status and a triplet of {block hash/block height/tx index in block} at which tx has been confirmed.
363+     * This triplet is whole 0 if tx is unconfirmed or abandoned. Meaning of these fields changes with CONFLICTED state

jnewbery commented at 10:16 pm on November 5, 2019:

nit: “This triplet is whole 0” doesn’t sound right. “All three are set to 0” would be more natural.

in src/wallet/wallet.h:368 in 8a11ab3ff4 outdated

369-        int nIndex = 0;
370+        Status status;
371+        uint256 hashBlock;
372+        int block_height;
373+        int nIndex;
374+        Confirmation(Status s = UNCONFIRMED, uint256 h = uint256(), int b = 0, int i = 0) : status(s), hashBlock(h), block_height(b), nIndex(i) {}

jnewbery commented at 10:22 pm on November 5, 2019:

I’m not a big fan of these default arguments, especially since the last two are both ints. It’s too easy to accidentally call the constructor with the wrong number of arguments and set block_height when you meant to set nIndex (as this branch does in an intermediate commit)

ariard commented at 5:55 pm on November 6, 2019:

Rearrange members initialization order and add comments, tell me if you have a better measure.

in src/wallet/wallet.h:368 in 5643cb72ea outdated

365-        uint256 hashBlock = uint256();
366-        int nIndex = 0;
367+        Status status;
368+        uint256 hashBlock;
369+        int block_height;
370+        int nIndex;

jkczyz commented at 10:45 pm on November 5, 2019:

Can these fields be const now?

ariard commented at 5:55 pm on November 6, 2019:

I think we can’t due to setting them with method like setAbandoned?

jkczyz commented at 10:08 pm on November 6, 2019:

Ah, you’re right. Could be better to assign a new object within setAbandoned instead, but feel free to leave as it is. Looks like there are other places where these are directly assigned.

in src/wallet/wallet.h:366 in 5643cb72ea outdated

360@@ -361,9 +361,11 @@ class CWalletTx
361      * where they instead point to block hash and index of the deepest conflicting tx.
362      */
363     struct Confirmation {
364-        Status status = UNCONFIRMED;
365-        uint256 hashBlock = uint256();
366-        int nIndex = 0;
367+        Status status;
368+        uint256 hashBlock;
369+        int block_height;

jkczyz commented at 10:47 pm on November 5, 2019:

The documentation needs to be updated for this new field.

ariard commented at 5:57 pm on November 6, 2019:

Shouldn’t be an issue anymore as block_height has been switched to the right commit.

jnewbery commented at 11:04 pm on November 5, 2019: member

utACK 8a11ab3ff4cba6456021e46813f530c22ada8ac1

I have a couple of nits inline, which can be fixed up later in order to not invalidate review. There are a couple of changes that I would like fixed before merge, since they’re to do with the git history, so can’t be fixed after the fact (and they don’t change the final code, so should be an easy re-ACK for other reviewers)

Add the block_height member in the Add block_height field in struct Confirmation to not potentially break a git bisect and make the history correct.
The wording for commit Restrain waitForNotificationsIfNewBlocksConnected check to strict tip is a little confusing. Can I suggest the following instead:

 0Only return early from BlockUntilSyncedToCurrentChain() if current tip is exact match
 1
 2In the next commit, we start using BlockConnected/BlockDisconnected
 3callbacks to establish tx depth, rather than querying the chain
 4directly.
 5
 6Currently, BlockUntilSyncedToCurrentChain() will return early if the
 7best block processed by the wallet is a descendant of the node's tip.
 8That means that in the case of a re-org, it won't wait for the
 9BlockDisconnected callbacks that have been enqueued during the re-org
10but have not yet been triggered in the wallet.
11
12Change BlockUntilSyncedToCurrentChain() to only return early if the
13wallet's m_last_block_processed matches the tip exactly. This ensures
14that there are no BlockDisconnected or BlockConnected callbacks
15in-flight.

Great work with this PR. Thanks for persisting with it, and thanks to @ryanofsky for chasing reviewers!

jkczyz commented at 11:05 pm on November 5, 2019: contributor

utACK 8a11ab3 modulo a few comments.

Replace CWalletTx::SetConf by Confirmation initialization list 9700fcb47f

ariard force-pushed on Nov 6, 2019

ariard commented at 5:54 pm on November 6, 2019: member

Updated at 36b68de :

move block_height to Add block_height field in struct Confirmation
modify Confirmation constructor and add comments to avoid ambiguity in place where it’s called
rearrange Confirmation constructor comment
fix nit wording on Confirmation comment
add guard clause for importprunedfunds as suggested in its own commit
rewrite commit message for former Restrain waitForNotificationsIfNewBlocksConnected check to strict tip

Thanks @ryanofsky, @jnewbery, @jkczyz for reviews! Re-ACK should be minimal on diff.

EDIT: forgot to update test too, travis should pass now.

Add block_height field in struct Confirmation

At wallet loading, we rely on chain state querying to retrieve
height of txn, to do so we ensure that lock order is respected
between cs_main and cs_wallet.

If wallet loaded is the wallet-tool one, all wallet txn will
show up with a height of zero. It doesn't matter as confirmation
height is not used by wallet-tool.

Reorder arguments and document Confirmation calls to avoid
ambiguity.

Fixes nits left from #16624

5971d3848e

Refactor some importprunedfunds checks with guard clause

Credit to jkczyz

769ff05e48

Only return early from BlockUntilSyncedToCurrentChain if current tip

is exact match

In the next commit, we start using BlockConnected/BlockDisconnected
callbacks to establish tx depth, rather than querying the chain
directly.

Currently, BlockUntilSyncedToCurrentChain will return early if
the best block processed by the wallet is a descendant of the node'tip.
That means that in the case of a re-org, it won't wait for the
BlockDisconnected callbacks that have been enqueued during the re-org
but have not yet been triggered in the wallet.

Change BlockUntilSyncedToCurrentChain to only return early if the
wallet's m_last_block_processed matches the tip exactly. This ensures
that there are no BlockDisconnected or BlockConnected callbacks
in-flight.

f77b1de16f

Use CWallet::m_last_block_processed_height in GetDepthInMainChain

Avoid to lock chain to query state thanks to tracking last block
height in CWallet.

0ff03871ad

Remove locked_chain from GetDepthInMainChain and its callers

We don't remove yet Chain locks as we need to preserve lock
order with CWallet one until swapping at once to avoid
deadlock failures (spotted by --enable-debug)

b66c429c56

Remove getBlockDepth method from Chain::interface

Pass conflicting height in CWallet::MarkConflicted

36b68de5b2

ariard force-pushed on Nov 6, 2019

ryanofsky approved

ryanofsky commented at 6:56 pm on November 6, 2019: member

Code review ACK 36b68de5b2938722911db900ca299f7008780d01. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment

in src/wallet/wallet.h:369 in 36b68de5b2

370-        int nIndex = 0;
371+        Status status;
372+        int block_height;
373+        uint256 hashBlock;
374+        int nIndex;
375+        Confirmation(Status s = UNCONFIRMED, int b = 0, uint256 h = uint256(), int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {}

jnewbery commented at 7:03 pm on November 6, 2019:

very minor nit: using b for height and h for hash is a little confusing. Using a few extra characters and calling these status_in, height_in, hash_in and index_in would make this clearer.

jnewbery commented at 7:26 pm on November 6, 2019: member

utACK 36b68de5b2938722911db900ca299f7008780d01

I’ve left one nit, which you definitely shouldn’t fix unless you need to change something else.

jkczyz commented at 10:09 pm on November 6, 2019: contributor

utACK 769ff05

jnewbery commented at 11:11 pm on November 6, 2019: member

@jkczyz you’ve ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2938722911db900ca299f7008780d01).

in src/wallet/wallet.h:1147 in 5aacc3eff1 outdated

1142+        AssertLockHeld(cs_wallet);
1143+        assert(m_last_block_processed_height >= 0);
1144+        return m_last_block_processed_height;
1145+    };
1146+    /** Set last block processed height, currently only use in unit test */
1147+    void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

promag commented at 11:39 pm on November 6, 2019:

5aacc3eff15b9b5bdc951f1e274f00d581f63bce

Use const uint256& block_hash?

in src/qt/test/wallettests.cpp:142 in 5aacc3eff1 outdated

138@@ -139,10 +139,12 @@ void TestGUI(interfaces::Node& node)
139     wallet->LoadWallet(firstRun);
140     {
141         auto spk_man = wallet->GetLegacyScriptPubKeyMan();
142+        auto locked_chain = wallet->chain().lock();

promag commented at 11:44 pm on November 6, 2019:

5aacc3eff15b9b5bdc951f1e274f00d581f63bce

Why not just lock ::cs_main if you don’t even use the interface?

ariard commented at 4:37 pm on November 7, 2019:

Method chain here is the way to use the interfaces::chain, I think it doesn’t matter that much here to use the Node or Chain one.

in src/zmq/zmqnotificationinterface.cpp:188 in 10b4729e33 outdated

184@@ -185,7 +185,7 @@ void CZMQNotificationInterface::BlockConnected(const std::shared_ptr<const CBloc
185     }
186 }
187 
188-void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock)
189+void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)

promag commented at 11:47 pm on November 6, 2019:

10b4729e33f76092bd8cfa06d1a5e0a066436f76

nit, just pindex? Otherwise use snake case. Same in the header.

jkczyz commented at 0:11 am on November 7, 2019: contributor

@jkczyz you’ve ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de).

utACK 36b68de

Right! The PR interface was showing a different order from the branch.

in src/wallet/wallet.cpp:3943 in 0ff03871ad outdated

3935@@ -3936,9 +3936,11 @@ CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn)
3936 
3937 int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
3938 {
3939+    assert(pwallet != nullptr);
3940+    AssertLockHeld(pwallet->cs_wallet);
3941     if (isUnconfirmed() || isAbandoned()) return 0;
3942 
3943-    return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);
3944+    return (pwallet->GetLastBlockHeight() - m_confirm.block_height + 1) * (isConflicted() ? -1 : 1);

promag commented at 0:19 am on November 7, 2019:

0ff03871add000f8b4d8f82aeb168eed2fc9dc5f

I don’t think commit message is correct:

0Avoid to lock chain to query state thanks to tracking last block
1height in CWallet.

Should say something like?

0Use wallet's last block height instead of the locked chain, which
1will allow removing the chain lock.

ariard commented at 4:38 pm on November 7, 2019:

That’s a better wording, I will use it if I have to rebase.

ryanofsky commented at 10:14 pm on January 27, 2020:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (0ff03871add000f8b4d8f82aeb168eed2fc9dc5f) @ariard It seems like this line is written assuming that GetLastBlockHeight() is always >= m_confirm.block_height, but if I add asserts here:

0assert(m_confirm.block_height >= 0);
1assert(m_confirm.block_height <= pwallet->GetLastBlockHeight());

The second assert fails in wallet_abandonconflict.py and wallet_bumpfee.py tests. Wondering if this expected or seems like a bug to you

in src/wallet/wallet.cpp:989 in 36b68de5b2

985@@ -986,7 +986,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
986     auto locked_chain = chain().lock();
987     LOCK(cs_wallet);
988 
989-    int conflictconfirms = -locked_chain->getBlockDepth(hashBlock);
990+    int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1;

promag commented at 0:23 am on November 7, 2019:

36b68de5b2938722911db900ca299f7008780d01

What’s the problem with -(m_last_block_processed_height - conflicting_height + 1)?

promag commented at 0:25 am on November 7, 2019: member

Code review ACK 36b68de5b2938722911db900ca299f7008780d01.

Build some intermediate commits too. Some comments for your consideration.

ariard commented at 4:43 pm on November 7, 2019: member

Can someome relaunch AppVeyor ? It got a linker error on bech32_tests.obj

jonatack commented at 4:50 pm on November 7, 2019: member

Can someome relaunch AppVeyor ? It got a linker error on bech32_tests.obj

Appveyor is an unrelated pending issue from #17357 affecting all other PRs that iiuc should be fixed by #17384.

meshcollider commented at 10:20 am on November 8, 2019: contributor

Looks good, thanks for keeping this maintained ariard. I agree with most of promag’s comments but they aren’t worth holding up merge for.

utACK 36b68de5b2938722911db900ca299f7008780d01

meshcollider referenced this in commit 99ab3a72c5 on Nov 8, 2019

meshcollider merged this on Nov 8, 2019

meshcollider closed this on Nov 8, 2019

fanquake removed this from the "Blockers" column in a project

MarcoFalke commented at 11:07 pm on November 12, 2019: member

It looks like this might have sped up all WalletBalance micro benchmarks:

https://codespeed.bitcoinperf.com/timeline/?exe=3%2C4%2C2%2C1%2C5&base=1%2B23&ben=micro.clang.WalletBalanceDirty&env=1&revs=10&equid=off&quarts=on&extr=on

shesek commented at 10:56 pm on November 19, 2019: contributor

It appears like @MarcoFalke’s link always displays the last 10 days, which already don’t include the performance gains (which happened on Nov 8). I couldn’t find a way to link to a specific date range in codespeed, so for future reference for people bumping into this thread, here’s a screenshot instead (of the last 50 days).

jasonbcox referenced this in commit f5619aa4a7 on Sep 22, 2020

jasonbcox referenced this in commit 9f81ae2a5b on Sep 22, 2020

jasonbcox referenced this in commit a49534be02 on Sep 22, 2020

jasonbcox referenced this in commit c789d0e822 on Sep 22, 2020

jasonbcox referenced this in commit e2178e2316 on Sep 22, 2020

jasonbcox referenced this in commit c3248ae486 on Sep 22, 2020

jasonbcox referenced this in commit 169f65bc5d on Sep 22, 2020

jasonbcox referenced this in commit e0ba693ad5 on Sep 22, 2020

jasonbcox referenced this in commit d0e0b2ca11 on Sep 22, 2020

in src/wallet/wallet.cpp:3941 in 0ff03871ad outdated

3935@@ -3936,9 +3936,11 @@ CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn)
3936 
3937 int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const
3938 {
3939+    assert(pwallet != nullptr);
3940+    AssertLockHeld(pwallet->cs_wallet);
3941     if (isUnconfirmed() || isAbandoned()) return 0;
3942 
3943-    return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);

ryanofsky commented at 7:30 pm on January 8, 2021:

In commit “Use CWallet::m_last_block_processed_height in GetDepthInMainChain” (0ff0387)

Dropping the getBlockDepth call here introduces a bug because getBlockDepth had a safety check to return 0 if there was a reorg and hashBlock is no longer in the active chain, while the new code will just return a garbage depth. I think this is also the reason adding height asserts didn’t work in some tests: #15931 (review). A few fixes are possible and mentioned bug-reorg-depth. A fix shouldn’t be hard, but a bigger challenge is adding better test coverage to prevent more bugs like this.

Munkybooty referenced this in commit 2bd48bf151 on Nov 17, 2021

Munkybooty referenced this in commit 6cae26ad5e on Nov 17, 2021

Munkybooty referenced this in commit 438010ab74 on Nov 17, 2021

Munkybooty referenced this in commit ae87f054f5 on Nov 18, 2021

Munkybooty referenced this in commit 338e7c21c9 on Nov 24, 2021

Munkybooty referenced this in commit c7e5c321e2 on Nov 30, 2021

Munkybooty referenced this in commit 4d83b241ae on Dec 15, 2021

Munkybooty referenced this in commit 081b78a64a on Dec 17, 2021

Munkybooty referenced this in commit 3d60fb3fdb on Dec 24, 2021

kittywhiskers referenced this in commit 7ccad766e2 on Jan 11, 2022

Munkybooty referenced this in commit ff621867b6 on Jan 19, 2022

Munkybooty referenced this in commit 71513cb59c on Jan 24, 2022

kittywhiskers referenced this in commit 2ce130f930 on Jan 25, 2022

Munkybooty referenced this in commit 02e5436bb8 on Jan 30, 2022

kittywhiskers referenced this in commit a5af23fba1 on Feb 19, 2022

kittywhiskers referenced this in commit 5af3504e80 on Mar 25, 2022

kittywhiskers referenced this in commit b2c5352024 on Mar 30, 2022

kittywhiskers referenced this in commit 35d587a9f6 on Apr 3, 2022

kittywhiskers referenced this in commit 69a3d865d5 on Apr 3, 2022

kittywhiskers referenced this in commit be0a86ad1f on Apr 5, 2022

kittywhiskers referenced this in commit 0930a0260f on Apr 5, 2022

kittywhiskers referenced this in commit 66fe39a5ae on Apr 6, 2022

kittywhiskers referenced this in commit 33995ba245 on Apr 7, 2022

kittywhiskers referenced this in commit e78336eb3a on Apr 7, 2022

kittywhiskers referenced this in commit 2666192a77 on Apr 17, 2022

kittywhiskers referenced this in commit b735422a83 on Apr 19, 2022

PastaPastaPasta referenced this in commit f2704eae34 on Apr 20, 2022

malbit referenced this in commit 72b925f69a on Aug 7, 2022

DrahtBot locked this on Aug 16, 2022

Remove GetDepthInMainChain dependency on locked chain interface #15931

Conflicts