Remove GetDepthInMainChain dependency on locked chain interface by ariard · Pull Request #15931 · bitcoin/bitcoin

ariard · 2019-05-01T15:53:57Z

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

DrahtBot · 2019-05-01T18:27:35Z

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.

fanquake · 2019-06-19T01:30:44Z

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

ariard · 2019-06-19T03:15:39Z

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

ryanofsky

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.

src/wallet/wallet.h

ryanofsky · 2019-06-19T17:59:05Z

src/wallet/wallet.h

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.

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.

ryanofsky · 2019-06-19T18:06:42Z

src/wallet/wallet.h

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.

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 ?

ryanofsky · 2019-06-19T18:53:20Z

src/wallet/wallet.cpp

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:

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

Updated AddToWallet, hope it's clearer

ryanofsky · 2019-06-19T19:06:25Z

src/wallet/wallet.cpp

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.

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 ?

src/wallet/wallet.cpp

src/interfaces/chain.h

ryanofsky · 2019-06-19T20:08:34Z

src/wallet/wallet.cpp

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.

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.

src/wallet/wallet.cpp

src/wallet/wallet.h

promag

Concept ACK.

ariard · 2019-07-09T02:24:55Z

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

ryanofsky · 2019-11-05T20:32:52Z

src/wallet/rpcwallet.cpp

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

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

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

ryanofsky

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

promag · 2019-11-05T21:39:44Z

Sure Russell, I'll review.

jnewbery

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:

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's 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.

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

src/wallet/wallet.h

jnewbery · 2019-11-05T22:22:12Z

src/wallet/wallet.h

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)

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

jkczyz

utACK 8a11ab3 modulo a few comments.

src/wallet/rpcdump.cpp

jkczyz · 2019-11-05T22:45:47Z

src/wallet/wallet.h

Can these fields be const now?

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

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.

jkczyz · 2019-11-05T22:47:22Z

src/wallet/wallet.h

The documentation needs to be updated for this new field.

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

ariard · 2019-11-06T17:54:25Z

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.

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 bitcoin#16624

Credit to jkczyz

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.

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

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)

Pass conflicting height in CWallet::MarkConflicted

ryanofsky

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

jnewbery

utACK 36b68de

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

jnewbery · 2019-11-06T19:03:47Z

src/wallet/wallet.h

+        int block_height;
+        uint256 hashBlock;
+        int nIndex;
+        Confirmation(Status s = UNCONFIRMED, int b = 0, uint256 h = uint256(), int i = 0) : status(s), block_height(b), hashBlock(h), nIndex(i) {}


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.

jkczyz · 2019-11-06T22:09:18Z

utACK 769ff05

jnewbery · 2019-11-06T23:11:07Z

@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).

jkczyz · 2019-11-07T00:11:17Z

@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.

promag

Code review ACK 36b68de.

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

promag · 2019-11-06T23:31:00Z

src/wallet/wallet.cpp

5aacc3e

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.

promag · 2019-11-06T23:35:07Z

src/wallet/wallet.h

5aacc3e

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

promag · 2019-11-06T23:39:16Z

src/wallet/wallet.h

+        return m_last_block_processed_height;
+    };
+    /** Set last block processed height, currently only use in unit test */
+    void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)


5aacc3e

Use const uint256& block_hash?

promag · 2019-11-06T23:44:38Z

src/qt/test/wallettests.cpp

    wallet->LoadWallet(firstRun);
    {
        auto spk_man = wallet->GetLegacyScriptPubKeyMan();
+        auto locked_chain = wallet->chain().lock();


5aacc3e

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

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.

promag · 2019-11-06T23:47:24Z

src/zmq/zmqnotificationinterface.cpp

 }

-void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock)
+void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)


10b4729

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

promag · 2019-11-06T23:59:56Z

src/wallet/rpcdump.cpp

9700fcb

You can also drop Confirmation constructor and do

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

and

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

promag · 2019-11-07T00:19:14Z

src/wallet/wallet.cpp

    if (isUnconfirmed() || isAbandoned()) return 0;

-    return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);
+    return (pwallet->GetLastBlockHeight() - m_confirm.block_height + 1) * (isConflicted() ? -1 : 1);


0ff0387

I don't think commit message is correct:

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

Should say something like?

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

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

promag · 2019-11-07T00:23:13Z

src/wallet/wallet.cpp

    LOCK(cs_wallet);

-    int conflictconfirms = -locked_chain->getBlockDepth(hashBlock);
+    int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1;


36b68de

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

ariard · 2019-11-07T16:43:31Z

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

jonatack · 2019-11-07T16:50:42Z

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 · 2019-11-08T10:20:22Z

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 36b68de

maflcko · 2019-11-12T23:07:25Z

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 · 2019-11-19T22:56:37Z

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).

ryanofsky · 2020-01-27T22:14:22Z

src/wallet/wallet.cpp

    if (isUnconfirmed() || isAbandoned()) return 0;

-    return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);
+    return (pwallet->GetLastBlockHeight() - m_confirm.block_height + 1) * (isConflicted() ? -1 : 1);


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

@ariard It seems like this line is written assuming that GetLastBlockHeight() is always >= m_confirm.block_height, but if I add asserts here:

assert(m_confirm.block_height >= 0); assert(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

ryanofsky · 2021-01-08T19:30:07Z

src/wallet/wallet.cpp

+    AssertLockHeld(pwallet->cs_wallet);
    if (isUnconfirmed() || isAbandoned()) return 0;

-    return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1);


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 (comment). 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.

fanquake added Refactoring Wallet labels May 1, 2019

DrahtBot added the Needs rebase label May 1, 2019

ariard force-pushed the 2019-04-remove-external-locking branch from 5653f46 to a0ef689 Compare May 2, 2019 19:30

DrahtBot removed the Needs rebase label May 2, 2019

ariard force-pushed the 2019-04-remove-external-locking branch 3 times, most recently from adf914a to 773da90 Compare May 6, 2019 18:53

DrahtBot added the Needs rebase label May 7, 2019

ariard force-pushed the 2019-04-remove-external-locking branch from 773da90 to 8b66249 Compare May 13, 2019 13:52

DrahtBot added Needs rebase and removed Needs rebase labels May 13, 2019

ariard force-pushed the 2019-04-remove-external-locking branch from 8b66249 to e284e42 Compare May 29, 2019 23:19

DrahtBot removed the Needs rebase label May 29, 2019

DrahtBot added the Needs rebase label Jun 18, 2019

ryanofsky reviewed Jun 19, 2019

View reviewed changes

promag reviewed Jun 30, 2019

View reviewed changes

ariard force-pushed the 2019-04-remove-external-locking branch 2 times, most recently from cc9bd04 to 1458ded Compare July 9, 2019 01:46

jnewbery mentioned this pull request Jul 9, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

DrahtBot added Needs rebase and removed Needs rebase labels Jul 9, 2019

ariard force-pushed the 2019-04-remove-external-locking branch 4 times, most recently from 1697ee4 to 04b4683 Compare July 16, 2019 02:22

ryanofsky reviewed Nov 5, 2019

View reviewed changes

ariard force-pushed the 2019-04-remove-external-locking branch from f1d66ce to 8a11ab3 Compare November 5, 2019 20:41

ryanofsky approved these changes Nov 5, 2019

View reviewed changes

jnewbery reviewed Nov 5, 2019

View reviewed changes

jkczyz reviewed Nov 5, 2019

View reviewed changes

Replace CWalletTx::SetConf by Confirmation initialization list

9700fcb

Antoine Riard added 6 commits November 6, 2019 13:29

Refactor some importprunedfunds checks with guard clause

769ff05

Credit to jkczyz

Use CWallet::m_last_block_processed_height in GetDepthInMainChain

0ff0387

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

Remove locked_chain from GetDepthInMainChain and its callers

b66c429

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)

Remove getBlockDepth method from Chain::interface

36b68de

Pass conflicting height in CWallet::MarkConflicted

ryanofsky approved these changes Nov 6, 2019

View reviewed changes

jnewbery reviewed Nov 6, 2019

View reviewed changes

promag reviewed Nov 7, 2019

View reviewed changes

ryanofsky reviewed Jan 27, 2020

View reviewed changes

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Jan 7, 2021

Minor wallet issue with conflicted txs #7315

Closed

ryanofsky reviewed Jan 8, 2021

View reviewed changes

kwvg mentioned this pull request Jan 11, 2022

merge bitcoin#15931, #16839, #17192, #17407, #18037, #17997, partial #15639, #17989: deglobalisation and mining rpc backports dashpay/dash#4357

Closed

Conversation

ariard commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

fanquake commented Jun 19, 2019

Uh oh!

ariard commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 5, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ariard commented May 1, 2019 •

edited

Loading

DrahtBot commented May 1, 2019 •

edited

Loading

ariard commented Jun 19, 2019 •

edited

Loading

ariard commented Nov 6, 2019 •

edited

Loading

promag left a comment •

edited

Loading