wallet: Remove calls to Chain::Lock methods by ryanofsky · Pull Request #17954 · bitcoin/bitcoin

ryanofsky · 2020-01-17T19:51:04Z

This is a set of changes updating wallet code to make fewer calls to Chain::Lock methods, so the Chain::Lock class will be easier to remove in #16426 with fewer code changes and small changes to behavior.

jnewbery · 2020-01-17T22:54:32Z

Concept ACK

DrahtBot · 2020-01-18T00:58:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Refactor WalletRescanReserver to use wallet reference #18601 (wallet: Refactor WalletRescanReserver to use wallet reference by promag)
[wallet] Track conflicted transactions removed from mempool and fix UI notifications #18600 ([wallet] Track conflicted transactions removed from mempool and fix UI notifications by ariard)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
gui: Bilingual GUI error messages #16224 (gui: Bilingual GUI error messages by hebasto)
Remove CWalletTx merging logic from AddToWallet #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.

ariard

Code review ACK 1f4b604.

What do you think about caching also last block time at connection ? I think that would make some changes here better by avoiding to uselessly lock the node, even if it's through the interface.

ariard · 2020-01-21T19:52:45Z

src/wallet/wallet.h

+    uint256 GetLastBlockHash() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
+    {
+        AssertLockHeld(cs_wallet);
+        assert(m_last_block_processed_height >= 0);


cfc9373

It just occurs to me than this assert and the one in GetLastBlockHeight (than I introduced in 5aacc3e) are unsafe if we don't have a Chain interface from which to query block height at wallet creation (CreateWalletFromFile) but that's something to keep in mind if in the future you can run the wallet without a chain.

cfc9373

It just occurs to me than this assert and the one in GetLastBlockHeight (than I introduced in 5aacc3e) are unsafe if we don't have a Chain interface from which to query block height at wallet creation (CreateWalletFromFile) but that's something to keep in mind if in the future you can run the wallet without a chain.

Yes, I think these cases would only be hit when running wallet code offline with the bitcoin-wallet tool or something similar. But if we add more offline features more code will have to change to be flexible about missing data

ariard · 2020-01-21T20:06:18Z

src/interfaces/wallet.cpp

-        }
+        num_blocks = m_wallet->GetLastBlockHeight();
+        block_time = -1;
+        m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), nullptr, &block_time);


cfc9373

As noted in commit, we may have asynchronicity between node and wallet w.r.t block processing. So returned block time may not be the one of last block height. To avoid this we may cache block time to fetch it when needed with GetLastBlockTime. It would also remove some getBlockTime (but not all last time I looked on).

cfc9373

As noted in commit, we may have asynchronicity between node and wallet w.r.t block processing. So returned block time may not be the one of last block height. To avoid this we may cache block time to fetch it when needed with GetLastBlockTime. It would also remove some getBlockTime (but not all last time I looked on).

Just to be clear, height and time here should be in sync due to cs_wallet being held above. Could still cache the time though. Commit description is saying how the GUI display should be more up to date after this commit, because the transaction data and num blocks value will be in sync, instead of a higher num blocks being returned with older transaction data

Even further than cs_wallet, we are still holding cs_main there through the chain lock. When we're going to remove locked_chain we may have asynchronicity due to to height being based on BlockConnected locked by cs_wallet and findBlock locked by cs_main ?

re: #17954 (comment)

(Relevant commit is cfc9373)

Even further than cs_wallet, we are still holding cs_main there through the chain lock. When we're going to remove locked_chain we may have asynchronicity due to to height being based on BlockConnected locked by cs_wallet and findBlock locked by cs_main ?

The GUI is asynchronous by design. It just needs to display internally consistent information within a transaction, and be able to determine if the information is fresh or out of date. The num_blocks height here returned to gui is used for that freshness check, so the new value set here should be better than the previous value for that. More ideally, though num_blocks will be replaced by a hash, which #17993 starts to do

src/wallet/rpcdump.cpp

ariard · 2020-01-21T20:36:17Z

src/wallet/rpcdump.cpp

-    file << strprintf("#   mined on %s\n", tip_height ? FormatISO8601DateTime(locked_chain->getBlockTime(*tip_height)) : "(missing block time)");
+    file << strprintf("# * Best block at time of backup was %i (%s),\n", pwallet->GetLastBlockHeight(), pwallet->GetLastBlockHash().ToString());
+    int64_t block_time = 0;
+    pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &block_time);


673e0b6

Another candidate for GetLastBlockTime

ariard · 2020-01-21T20:46:33Z

src/wallet/rpcdump.cpp

        }
-        Optional<int> tip_height = locked_chain->getHeight();
-        nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
+        pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &nTimeBegin);


673e0b6

I'm not sure about the commit message, IMO it's less accurate but on the whole make the rescan protection better by starting farther in the past. Anyway, being based on wallet tip or node tip should be safe given the range of TIMESTAMP_WINDOW

673e0b6

I'm not sure about the commit message, IMO it's less accurate but on the whole make the rescan protection better by starting farther in the past. Anyway, being based on wallet tip or node tip should be safe given the range of TIMESTAMP_WINDOW

Hmm, I'm not sure when it would be less accurate. Are you thinking of a case?

re: #17954 (comment)

I think I meaned because we rely now on wallet last block hash instead of main tip and we may be late from one block, so in my opinion we are less accurate from one block but we agree on rescan being safer. Nit interpretation, doesn't matter.

re: #17954 (comment)

I think I meaned because we rely now on wallet last block hash instead of main tip and we may be late from one block, so in my opinion we are less accurate from one block but we agree on rescan being safer. Nit interpretation, doesn't matter.

I think in the case you are talking about the block height/hash/time values in the backup are now more accurate than before because cs_wallet is locked already. So the backup information make will be consistent with the wallet block tip, not the node block tip, in any cases where they are different

ariard · 2020-01-21T20:55:30Z

src/wallet/rpcdump.cpp


        // Verify all timestamps are present before importing any keys.
-        const Optional<int> tip_height = locked_chain->getHeight();
-        now = tip_height ? locked_chain->getBlockMedianTimePast(*tip_height) : 0;


1f4b604

If #17443 gets first + GetLastBlockTime, you may avoid to call findBlock here.

Agree that caching the last block time would make some of these commits easier.

ryanofsky

Thanks for the review! I pushed a few more commits I think finishing up the wallet rpc code. There are some more changes to come in wallet.cpp after this.

Updated 1f4b604 -> aeba8af (pr/unlock.1 -> pr/unlock.2, compare) with some tweaks to earlier commits and a few new commits extending the PR

re: #17954 (review)

What do you think about caching also last block time at connection ? I think that would make some changes here better by avoiding to uselessly lock the node, even if it's through the interface.

I think it's probably a good idea. It would use some more memory though, and it's unclear if it the change would simplify this PR or not overlap much. I think it's probably something to try out separately.

ryanofsky · 2020-01-21T21:51:22Z

src/interfaces/wallet.cpp

-        }
+        num_blocks = m_wallet->GetLastBlockHeight();
+        block_time = -1;
+        m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), nullptr, &block_time);


cfc9373

As noted in commit, we may have asynchronicity between node and wallet w.r.t block processing. So returned block time may not be the one of last block height. To avoid this we may cache block time to fetch it when needed with GetLastBlockTime. It would also remove some getBlockTime (but not all last time I looked on).

Just to be clear, height and time here should be in sync due to cs_wallet being held above. Could still cache the time though. Commit description is saying how the GUI display should be more up to date after this commit, because the transaction data and num blocks value will be in sync, instead of a higher num blocks being returned with older transaction data

ryanofsky · 2020-01-21T21:51:28Z

src/wallet/wallet.h

+    uint256 GetLastBlockHash() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
+    {
+        AssertLockHeld(cs_wallet);
+        assert(m_last_block_processed_height >= 0);


cfc9373

It just occurs to me than this assert and the one in GetLastBlockHeight (than I introduced in 5aacc3e) are unsafe if we don't have a Chain interface from which to query block height at wallet creation (CreateWalletFromFile) but that's something to keep in mind if in the future you can run the wallet without a chain.

Yes, I think these cases would only be hit when running wallet code offline with the bitcoin-wallet tool or something similar. But if we add more offline features more code will have to change to be flexible about missing data

src/wallet/rpcdump.cpp

ryanofsky · 2020-01-21T22:30:03Z

src/wallet/rpcdump.cpp

        }
-        Optional<int> tip_height = locked_chain->getHeight();
-        nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
+        pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &nTimeBegin);


673e0b6

I'm not sure about the commit message, IMO it's less accurate but on the whole make the rescan protection better by starting farther in the past. Anyway, being based on wallet tip or node tip should be safe given the range of TIMESTAMP_WINDOW

Hmm, I'm not sure when it would be less accurate. Are you thinking of a case?

ryanofsky · 2020-01-22T22:32:55Z

Added 3 commits aeba8af -> 60e6595 (pr/unlock.2 -> pr/unlock.3, compare) and removed PR draft status.

I think this is basically done. Combined with #17443 it should remove all calls to interfaces::Chain::Lock methods after wallet loading (#15719 should clean up loading). I'm hoping this PR and #17443 can be merged before #16426 so #16426 can be a little smaller and change wallet behavior less.

ariard · 2020-01-30T02:06:16Z

Thanks Russ, will review new changes soon.

I think it's probably a good idea. It would use some more memory though, and it's unclear if it the change would simplify this PR or not overlap much. I think it's probably something to try out separately.

I've a branch doing it, I can try to rebase it on top of this one and squeeze a last one before #16426.

ryanofsky

Comments below are from last week (they were in pending status because I forgot to submit the github review)

ryanofsky · 2020-01-31T20:58:03Z

src/interfaces/wallet.cpp

-        }
+        num_blocks = m_wallet->GetLastBlockHeight();
+        block_time = -1;
+        m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), nullptr, &block_time);


re: #17954 (comment)

(Relevant commit is cfc9373)

Even further than cs_wallet, we are still holding cs_main there through the chain lock. When we're going to remove locked_chain we may have asynchronicity due to to height being based on BlockConnected locked by cs_wallet and findBlock locked by cs_main ?

The GUI is asynchronous by design. It just needs to display internally consistent information within a transaction, and be able to determine if the information is fresh or out of date. The num_blocks height here returned to gui is used for that freshness check, so the new value set here should be better than the previous value for that. More ideally, though num_blocks will be replaced by a hash, which #17993 starts to do

ryanofsky · 2020-01-31T21:32:45Z

src/wallet/rpcdump.cpp

        }
-        Optional<int> tip_height = locked_chain->getHeight();
-        nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
+        pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &nTimeBegin);


re: #17954 (comment)

I think I meaned because we rely now on wallet last block hash instead of main tip and we may be late from one block, so in my opinion we are less accurate from one block but we agree on rescan being safer. Nit interpretation, doesn't matter.

I think in the case you are talking about the block height/hash/time values in the backup are now more accurate than before because cs_wallet is locked already. So the backup information make will be consistent with the wallet block tip, not the node block tip, in any cases where they are different

Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling. This bug could cause balances to appear out of date, and was first introduced bitcoin@a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117. Before that commit, there wasn't a problem because cs_main was held during the poll update. Currently, the problem should be rare. But if 8937d99 from bitcoin#17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification. Thanks to John Newbery <[email protected]> for catching this while reviewing bitcoin#17954.

jnewbery

I've reviewed as far as 759e494 wallet: Avoid use of Chain::Lock in rescanblockchain and just have nits so far. I'll try to complete review tomorrow.

src/interfaces/wallet.cpp

jnewbery · 2020-02-11T20:53:53Z

src/wallet/rpcdump.cpp


        // Verify all timestamps are present before importing any keys.
-        const Optional<int> tip_height = locked_chain->getHeight();
-        now = tip_height ? locked_chain->getBlockMedianTimePast(*tip_height) : 0;


Agree that caching the last block time would make some of these commits easier.

jnewbery · 2020-02-11T22:56:35Z

src/interfaces/chain.h

    //! the specified block hash are verified.
    virtual double guessVerificationProgress(const uint256& block_hash) = 0;

+    //! Return true if data is available for the specified blocks.


'specified blocks' is a bit vague. Can you be more precise about what block_hash min_height and max_height mean?

re: #17954 (comment)

'specified blocks' is a bit vague. Can you be more precise about what block_hash min_height and max_height mean?

Added description, also made min_height not Optional since nullopt was equivalent to 0

jnewbery · 2020-02-11T23:03:36Z

src/wallet/rpcwallet.cpp

        if (!request.params[0].isNull()) {
            start_height = request.params[0].get_int();
-            if (start_height < 0 || !tip_height || start_height > *tip_height) {
+            if (start_height < 0 || tip_height < 0 || start_height > tip_height) {


GetLastBlockHeight() can't return a tip_height that's < 0, so I think you can just remove || tip_height < 0

re: #17954 (comment)

GetLastBlockHeight() can't return a tip_height that's < 0, so I think you can just remove || tip_height < 0

Thanks updated

jnewbery · 2020-02-11T23:05:16Z

src/wallet/rpcwallet.cpp

    {
        auto locked_chain = pwallet->chain().lock();
-        Optional<int> tip_height = locked_chain->getHeight();
+        LOCK(pwallet->cs_wallet);


Do you need to hold the wallet lock for this entire block? Does it make sense to call:

WITH_LOCK(pwallet->cs_wallet, pwallet->GetLastBlockHeight());

re: #17954 (comment)

Do you need to hold the wallet lock for this entire block? Does it make sense to call:

WITH_LOCK(pwallet->cs_wallet, pwallet->GetLastBlockHeight());

The lock is also needed for the GetLastBlockHash call in the findAncestorByHeight line below. This could do something cleverer to reduce locking, and I'm happy to make changes if there are suggestions, but moving the lock seemed like simplest change that would work.

jnewbery · 2020-02-11T23:06:43Z

src/wallet/rpcwallet.cpp

-            if (stop_height) {
-                stop_block = locked_chain->getBlockHash(*stop_height);
-            }
+        if (tip_height >= 0) {


Again, I think this is always true, so you can remove this conditional.

re: #17954 (comment)

Again, I think this is always true, so you can remove this conditional.

Thanks, removed

bf36a3c gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky) Pull request description: Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling. This bug could cause balances to appear out of date, and was first introduced bitcoin@a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117. Before that commit, there wasn't a problem because cs_main was held during the poll update. Currently, the problem should be rare. But if 8937d99 from bitcoin#17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification. MarcoFalke also points out that a0704a8 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI. Thanks to John Newbery for finding this bug this while reviewing bitcoin#17954. ACKs for top commit: Empact: utACK bitcoin@bf36a3c jonasschnelli: utACK bf36a3c Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4

…k data Summary: FoundBlock class allows interfaces::Chain::findBlock to return more block information without having lots of optional output parameters. FoundBlock class is also used by other chain methods in upcoming commits. There is mostly no change in behavior. Only exception is CWallet::RescanFromTime now throwing NonFatalCheckError instead of std::logic_error. This is part [1/12] of Core [[bitcoin/bitcoin#17954 | PR17954]] : bitcoin/bitcoin@bf30cd4 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7864

Summary: This is a step toward removing the Chain::Lock class and reducing cs_main locking. It also helps ensure the GUI display stays up to date in the case where the node chain height runs ahead of wallet last block processed height. This is part [2/12] of Core [[bitcoin/bitcoin#17954 | PR17954]] : bitcoin/bitcoin@f6da44c Depends on D7864 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7865

ryanofsky changed the title ~~wallet: Remove calls to Chain::Lock methods in wallet~~ wallet: Remove calls to Chain::Lock methods Jan 17, 2020

fanquake added the Wallet label Jan 17, 2020

ryanofsky mentioned this pull request Jan 17, 2020

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

ariard approved these changes Jan 21, 2020

View reviewed changes

ryanofsky force-pushed the pr/unlock branch from 1f4b604 to aeba8af Compare January 21, 2020 22:30

ryanofsky commented Jan 21, 2020

View reviewed changes

ryanofsky marked this pull request as ready for review January 22, 2020 22:22

jnewbery mentioned this pull request Jan 23, 2020

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

Closed

ryanofsky mentioned this pull request Jan 24, 2020

Wallet passive startup #15719

Closed

ryanofsky force-pushed the pr/unlock branch from 60e6595 to 0a44d40 Compare January 28, 2020 23:25

DrahtBot added the Needs rebase label Jan 30, 2020

ryanofsky force-pushed the pr/unlock branch from 0a44d40 to d1dd916 Compare January 31, 2020 21:53

DrahtBot removed the Needs rebase label Jan 31, 2020

ryanofsky commented Feb 5, 2020

View reviewed changes

ryanofsky force-pushed the pr/unlock branch 3 times, most recently from 0d788ae to d2f92a9 Compare February 7, 2020 18:56

This was referenced Feb 11, 2020

wallet: undo conflicts properly in case of blocks disconnection #17543

Closed

Make all tests pass UBSan without using any UBSan suppressions #17208

Closed

ryanofsky mentioned this pull request Feb 11, 2020

gui: Fix race in WalletModel::pollBalanceChanged #18123

Merged

DrahtBot mentioned this pull request Feb 11, 2020

gui: Bilingual GUI error messages #16224

Merged

jnewbery reviewed Feb 11, 2020

View reviewed changes

This was referenced Feb 11, 2020

wallet: Make scan / abort status private to CWallet #14942

Closed

Remove CWalletTx merging logic from AddToWallet #9381

Merged

hebasto mentioned this pull request Nov 15, 2020

Silence false positive GCC warning in wallet/rpcwallet.cpp #20386

Merged

Conversation

ryanofsky commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jan 17, 2020

Uh oh!

DrahtBot commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 Jan 30, 2020 • edited by ryanofsky Loading Uh oh! There was an error while loading. Please reload this page.

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!

ryanofsky left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 22, 2020

Uh oh!

ariard commented Jan 30, 2020

Uh oh!

ryanofsky left a comment

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!

jnewbery 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!

ryanofsky commented Jan 17, 2020 •

edited

Loading

DrahtBot commented Jan 18, 2020 •

edited

Loading

ariard left a comment •

edited

Loading

ariard Jan 30, 2020 •

edited by ryanofsky

Loading