wallet: Remove calls to Chain::Lock methods

ryanofsky commented at 7:51 pm on January 17, 2020: member

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.

ryanofsky renamed this:
~~wallet: Remove calls to Chain::Lock methods in wallet~~
wallet: Remove calls to Chain::Lock methods
on Jan 17, 2020

fanquake added the label Wallet on Jan 17, 2020

jnewbery commented at 10:54 pm on January 17, 2020: member

Concept ACK

DrahtBot commented at 0:58 am on January 18, 2020: 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:

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

in src/wallet/wallet.h:1206 in cfc9373305 outdated

1144@@ -1145,6 +1145,12 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
1145         assert(m_last_block_processed_height >= 0);
1146         return m_last_block_processed_height;
1147     };
1148+    uint256 GetLastBlockHash() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)
1149+    {
1150+        AssertLockHeld(cs_wallet);
1151+        assert(m_last_block_processed_height >= 0);

ariard commented at 7:52 pm on January 21, 2020:

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.

ryanofsky commented at 9:51 pm on January 21, 2020:

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

in src/interfaces/wallet.cpp:334 in cfc9373305 outdated

337-            num_blocks = -1;
338-            block_time = -1;
339-        }
340+        num_blocks = m_wallet->GetLastBlockHeight();
341+        block_time = -1;
342+        m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), nullptr, &block_time);

ariard commented at 8:06 pm on January 21, 2020:

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

ryanofsky commented at 9:51 pm on January 21, 2020:

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

ariard commented at 1:57 am on January 30, 2020:

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 ?

ryanofsky commented at 8:58 pm on January 31, 2020:

re: #17954 (review)

(Relevant commit is cfc9373305eed32cd27eb436b555b06bc470dcbf)

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

in src/wallet/rpcdump.cpp:364 in e399fb49e8 outdated

358@@ -359,8 +359,9 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
359     }
360 
361     auto locked_chain = pwallet->chain().lock();
362-    Optional<int> height = locked_chain->getBlockHeight(merkleBlock.header.GetHash());
363-    if (height == nullopt) {
364+    LOCK(pwallet->cs_wallet);
365+    int height;
366+    if (!pwallet->chain().findAncestor(pwallet->GetLastBlockHash(), merkleBlock.header.GetHash(), &height)) {

ariard commented at 8:33 pm on January 21, 2020:

e399fb4

Previously, getBlockHeight would have return nullopt if merkleBlock have been out of chain. With this change, a height can be returned and ancestry asserted while node and walle tip being unsynchronized, so merkleBlock have been reorged out. IMO that’s fine if rpc caller is aware than processing have been done with best-wallet-knowledge.

ryanofsky commented at 9:52 pm on January 21, 2020:

re: #17954 (review)

e399fb4

Previously, getBlockHeight would have return nullopt if merkleBlock have been out of chain. With this change, a height can be returned and ancestry asserted while node and walle tip being unsynchronized, so merkleBlock have been reorged out. IMO that’s fine if rpc caller is aware than processing have been done with best-wallet-knowledge.

Thanks, will add these details to the commit description. I think “best-wallet-knowledge” can really be the only safe assumption for calling wallet rpcs if we’re going to let the wallet act asynchronously from the node

in src/wallet/rpcdump.cpp:792 in 673e0b6e7c outdated

787@@ -789,9 +788,10 @@ UniValue dumpwallet(const JSONRPCRequest& request)
788     // produce output
789     file << strprintf("# Wallet dump created by Bitcoin %s\n", CLIENT_BUILD);
790     file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime()));
791-    const Optional<int> tip_height = locked_chain->getHeight();
792-    file << strprintf("# * Best block at time of backup was %i (%s),\n", tip_height.get_value_or(-1), tip_height ? locked_chain->getBlockHash(*tip_height).ToString() : "(missing block hash)");
793-    file << strprintf("#   mined on %s\n", tip_height ? FormatISO8601DateTime(locked_chain->getBlockTime(*tip_height)) : "(missing block time)");
794+    file << strprintf("# * Best block at time of backup was %i (%s),\n", pwallet->GetLastBlockHeight(), pwallet->GetLastBlockHash().ToString());
795+    int64_t block_time = 0;
796+    pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &block_time);

ariard commented at 8:36 pm on January 21, 2020:

673e0b6

Another candidate for GetLastBlockTime

in src/wallet/rpcdump.cpp:567 in 673e0b6e7c outdated

563@@ -564,8 +564,7 @@ UniValue importwallet(const JSONRPCRequest& request)
564         if (!file.is_open()) {
565             throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
566         }
567-        Optional<int> tip_height = locked_chain->getHeight();
568-        nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
569+        pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &nTimeBegin);

ariard commented at 8:46 pm on January 21, 2020:

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

ryanofsky commented at 10:30 pm on January 21, 2020:

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?

ariard commented at 2:02 am on January 30, 2020:

re: #17954 (review)

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.

ryanofsky commented at 9:32 pm on January 31, 2020:

re: #17954 (review)

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

in src/wallet/rpcdump.cpp:1382 in 1f4b604717 outdated

1363@@ -1364,20 +1364,13 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1364         EnsureWalletIsUnlocked(pwallet);
1365 
1366         // Verify all timestamps are present before importing any keys.
1367-        const Optional<int> tip_height = locked_chain->getHeight();
1368-        now = tip_height ? locked_chain->getBlockMedianTimePast(*tip_height) : 0;

ariard commented at 8:55 pm on January 21, 2020:

1f4b604

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

jnewbery commented at 8:53 pm on February 11, 2020:

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

ariard approved

ariard commented at 8:57 pm on January 21, 2020: member

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.

ryanofsky force-pushed on Jan 21, 2020

ryanofsky commented at 10:44 pm on January 21, 2020: member

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 1f4b60471730fb5961096fef286c1bd6ec2538b8 -> aeba8afaf5d12fd7b2552616f817712d4fc5062e (pr/unlock.1 -> pr/unlock.2, compare) with some tweaks to earlier commits and a few new commits extending the PR

re: #17954#pullrequestreview-346148268

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 marked this as ready for review on Jan 22, 2020

ryanofsky commented at 10:32 pm on January 22, 2020: member

Added 3 commits aeba8afaf5d12fd7b2552616f817712d4fc5062e -> 60e6595f5cfd266c14c48994b0bb4afb5bf7fcb3 (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.

ryanofsky force-pushed on Jan 28, 2020

ariard commented at 2:06 am on January 30, 2020: member

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.

DrahtBot added the label Needs rebase on Jan 30, 2020

ryanofsky force-pushed on Jan 31, 2020

DrahtBot removed the label Needs rebase on Jan 31, 2020

ryanofsky commented at 8:34 pm on February 5, 2020: member

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

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

ryanofsky force-pushed on Feb 6, 2020

ryanofsky force-pushed on Feb 7, 2020

in src/interfaces/wallet.cpp:376 in d2f92a9f75 outdated

384@@ -389,7 +385,7 @@ class WalletImpl : public Wallet
385             return false;
386         }
387         balances = getBalances();
388-        num_blocks = locked_chain->getHeight().get_value_or(-1);
389+        num_blocks = m_wallet->GetLastBlockHeight();

jnewbery commented at 8:29 pm on February 11, 2020:

I don’t think num_blocks is even used. There’s only one place that this interface function is called and it doesn’t use the result. Can you just remove it?

ryanofsky commented at 9:08 pm on February 12, 2020:

re: #17954 (review)

I don’t think num_blocks is even used. There’s only one place that this interface function is called and it doesn’t use the result. Can you just remove it?

Good catch, and thanks for bringing it up, it is fixed in #18123

ryanofsky referenced this in commit 37d27bc070 on Feb 11, 2020

in src/interfaces/chain.h:167 in d2f92a9f75 outdated

163@@ -145,6 +164,9 @@ class Chain
164     //! the specified block hash are verified.
165     virtual double guessVerificationProgress(const uint256& block_hash) = 0;
166 
167+    //! Return true if data is available for the specified blocks.

jnewbery commented at 10:56 pm on February 11, 2020:

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

ryanofsky commented at 9:08 pm on February 12, 2020:

re: #17954 (review)

‘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

in src/wallet/rpcwallet.cpp:3557 in d2f92a9f75 outdated

3555+        int tip_height = pwallet->GetLastBlockHeight();
3556 
3557         if (!request.params[0].isNull()) {
3558             start_height = request.params[0].get_int();
3559-            if (start_height < 0 || !tip_height || start_height > *tip_height) {
3560+            if (start_height < 0 || tip_height < 0 || start_height > tip_height) {

jnewbery commented at 11:03 pm on February 11, 2020:

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

ryanofsky commented at 9:09 pm on February 12, 2020:

re: #17954 (review)

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

Thanks updated

in src/wallet/rpcwallet.cpp:3543 in d2f92a9f75 outdated

3549+    Optional<int> stop_height;
3550+    uint256 start_block;
3551     {
3552         auto locked_chain = pwallet->chain().lock();
3553-        Optional<int> tip_height = locked_chain->getHeight();
3554+        LOCK(pwallet->cs_wallet);

jnewbery commented at 11:05 pm on February 11, 2020:

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

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

ryanofsky commented at 9:08 pm on February 12, 2020:

re: #17954 (review)

Do you need to hold the wallet lock for this entire block? Does it make sense to call:
0WITH_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.

in src/wallet/rpcwallet.cpp:3577 in d2f92a9f75 outdated

3583-            // so rescan continues to the tip (even if the tip advances during
3584-            // rescan).
3585-            if (stop_height) {
3586-                stop_block = locked_chain->getBlockHash(*stop_height);
3587-            }
3588+        if (tip_height >= 0) {

jnewbery commented at 11:06 pm on February 11, 2020:

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

ryanofsky commented at 9:10 pm on February 12, 2020:

re: #17954 (review)

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

Thanks, removed

jnewbery commented at 11:09 pm on February 11, 2020: member

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

luke-jr referenced this in commit 330a0986e8 on Feb 12, 2020

ryanofsky referenced this in commit bf36a3ccc2 on Feb 12, 2020

ryanofsky added the label Review club on Feb 12, 2020

luke-jr referenced this in commit 5a7833aec0 on Feb 13, 2020

jonasschnelli referenced this in commit b6a16fa44e on Feb 13, 2020

ryanofsky force-pushed on Feb 13, 2020

ryanofsky commented at 3:36 pm on February 13, 2020: member

Updated d2f92a9f759d41b150fa702c8f83d1b05b11c943 -> 1125fd92bd3ee6d44ab9f8118ac68c0f621c5a94 (pr/unlock.8 -> pr/unlock.9, compare) with some hasBlocks improvements and other suggested changes

in src/interfaces/chain.h:148 in e276b68214 outdated

143         int* height = 0) = 0;
144 
145+    //! Find most recent common ancestor between two blocks and optionally
146+    //! return its hash and/or height. Also return height of first block. Return
147+    //! nullopt if either block is unknown or there is no common ancestor.
148+    virtual Optional<int> findCommonAncestor(const uint256& block_hash1,

ariard commented at 6:32 pm on February 13, 2020:

e276b68

“If both blocks are on the same chain ancestor_height is the height of the oldest between them. Also return height of first block which may be the same than ancestor height.”

But honestly would prefer parameterize findFork instead of yet-another-single use method like passing wallet tip hash to findFork (and if null, then use default chain tip).

By the way, is findFork still used after this change ?

ryanofsky commented at 11:16 pm on February 13, 2020:

re: #17954 (review)

e276b68

“If both blocks are on the same chain ancestor_height is the height of the oldest between them. Also return height of first block which may be the same than ancestor height.”

But honestly would prefer parameterize findFork instead of yet-another-single use method like passing wallet tip hash to findFork (and if null, then use default chain tip).

By the way, is findFork still used after this change ?

findFork only used on startup after this change and is removed later in 3f1b867a096ac24073c59ecd2c660e07cfc2be50 from #15719.

I think findCommonAncestor is a more robust and more general version of findFork that works on any two blocks always returning the same value regardless of the current tip, avoiding race conditions that would otherwise happen when the tip is changing.

findCommonAncestor returns multiple values, so which of those values comes back in the return type, and which come back through output parameters is an aesthetic choice that isn’t too important to me. Probably if we were using c++17 I would have this return a tuple. If you think it’s bad to return block1 height, though, I could add a new int* block1_height output parameter, and change the return type from Optional<int> to bool.

ryanofsky commented at 1:21 pm on February 14, 2020:

Here’s a change that would make all the find block methods return block information same way 6f74c0a042b001283e1d7dd8a8ad8b46c75328e5 (branch), if it helps

EDIT: Newer version 25c1ae48204215622bc9fd3a8bc9677f15c32674

in src/wallet/rpcwallet.cpp:1641 in e276b68214 outdated

1637@@ -1638,8 +1638,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1638         --*altheight;
1639     }
1640 
1641-    int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
1642-    uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
1643+    uint256 lastblock = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms);

ariard commented at 7:24 pm on February 13, 2020:

e276b68

If I understand issue linked in the commit message, let’s say you call listsinceblock(genesis_hash, 100) with current_tip == 1100 (shouldn’t matter referring to chain_tip or wallet_tip). Target_confirm = 1100 + 1 - 100 = 1001. Lastblock = blockhash(1001)

Now while calling again listsinceblock(lastblock_hash, 100) with current_tip = 1100 depth = 1100 + 1 - 1001 = 100 So only transactions with depth < 100 are returned and not the ones with 100-conf as expected by target_confirmations (i.e transactions for block 1101, the “100th” from the main chain).

Is this the behavior you’re fixing by returning now the ancestor hash? Seems to me documentation is already marching code “So you would generally use a target_confirmations of say 6, you will be continually re-notified of transactions until they’ve reached 6 confirmations plus any new ones” but not sure if this what we really want..

ryanofsky commented at 11:16 pm on February 13, 2020:

re: #17954 (review)

e276b68

If I understand issue linked in the commit message, let’s say you call listsinceblock(genesis_hash, 100) with current_tip == 1100 (shouldn’t matter referring to chain_tip or wallet_tip). Target_confirm = 1100 + 1 - 100 = 1001. Lastblock = blockhash(1001)

Now while calling again listsinceblock(lastblock_hash, 100) with current_tip = 1100 depth = 1100 + 1 - 1001 = 100 So only transactions with depth < 100 are returned and not the ones with 100-conf as expected by target_confirmations (i.e transactions for block 1101, the “100th” from the main chain).

Is this the behavior you’re fixing by returning now the ancestor hash? Seems to me documentation is already marching code “So you would generally use a target_confirmations of say 6, you will be continually re-notified of transactions until they’ve reached 6 confirmations plus any new ones” but not sure if this what we really want..

I think I need to reread your example more closely to give a better response, but the case which this commit should fix is specifically the case where wallet_tip != chain_tip. So if the wallet is behind and wallet_tip=1100 while chain_tip=1150, I want the first listsinceblock call to return lastblock=blockhash(1001) instead of blockhash(1051) so transactions aren’t missed in the second call

in src/wallet/rpcwallet.cpp:1605 in e276b68214 outdated

1605@@ -1605,8 +1606,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1606 
1607     bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
1608 
1609-    const Optional<int> tip_height = locked_chain->getHeight();
1610-    int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
1611+    int depth = height ? pwallet->GetLastBlockHeight() + 1 - *height : -1;

ariard commented at 7:26 pm on February 13, 2020:

e276b68

Height of genesis block is 0 ? If so depth is -1 which I think isn’t the behavior expected (already there before ?)

ryanofsky commented at 11:16 pm on February 13, 2020:

re: #17954 (review)

e276b68

Height of genesis block is 0 ? If so depth is -1 which I think isn’t the behavior expected (already there before ?)

height is an Optional<int> so height ? is just checking if the optional value is set. If height is 0 the condition will evaluate to true and the correct depth should be set.

ariard commented at 11:20 pm on February 14, 2020:

Oh right, it’s an Optional, forget about it, forgive my C++ noobiness

in src/interfaces/chain.h:137 in e276b68214 outdated

132@@ -133,12 +133,23 @@ class Chain
133         int64_t* max_time = nullptr,
134         int64_t* mtp_time = nullptr) = 0;
135 
136+    //! Find ancestor of block at specified height and return its hash.
137+    virtual uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) = 0;

ariard commented at 7:35 pm on February 13, 2020:

e276b68

Why not modify getBlockHash a bit to do a LookupBlockIndex instead of querying ChainActive() ? Every block in ChainActive needs a BlockIndex so second should be a superset and it shouldn’t be an issue.

If caller care about block being in the active chain, it should call findFork just after.

(Long-term, IMO wallet shouldn’t have to deal with fork and just have a linear view of the chain, only when BlockDisconnected is called, state would be rewind. It’s should be caller responsibility to enforce tips consistency between it’s different components)

ryanofsky commented at 11:16 pm on February 13, 2020:

re: #17954 (review)

e276b68

Why not modify getBlockHash a bit to do a LookupBlockIndex instead of querying ChainActive() ? Every block in ChainActive needs a BlockIndex so second should be a superset and it shouldn’t be an issue.

If caller care about block being in the active chain, it should call findFork just after.

(Long-term, IMO wallet shouldn’t have to deal with fork and just have a linear view of the chain, only when BlockDisconnected is called, state would be rewind. It’s should be caller responsibility to enforce tips consistency between it’s different components)

I’m removing getBlockHash in 3f1b867a096ac24073c59ecd2c660e07cfc2be50 from #15719.

I think of findAncestorByHeight as a more robust replacement for getBlockHash that returns the same thing reliably regardless of the chain tip. findAncestorByHeight is used in a few places. It’s possible these could all go away in the future with your rescan branch, and by replacing listsinceblock and GetKeyBirthTimes code. The ugliest code is the rescan code. I’m not too worried about the other places, and I think none of the places involve wallet code that would be useful to run offline

in src/interfaces/chain.h:141 in 6067b74431 outdated

132@@ -133,6 +133,13 @@ class Chain
133         int64_t* max_time = nullptr,
134         int64_t* mtp_time = nullptr) = 0;
135 
136+    //! Return hash of first block in the chain with timestamp >= the given time
137+    //! and height >= than the given height, or nullopt if there is no block
138+    //! with a high enough timestamp and height. Also return the block height as
139+    //! an optional output parameter (to avoid the cost of a second lookup in
140+    //! case this information is needed.)
141+    virtual Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) = 0;

ariard commented at 9:17 pm on February 13, 2020:

6067b74

I think you can move the existing findFirstBlockWithTimeAndHeight method of Chain::Lock and just avoid adding a new one, it still returns both block height & hash

ryanofsky commented at 11:15 pm on February 13, 2020:

re: #17954 (review)

6067b74

I think you can move the existing findFirstBlockWithTimeAndHeight method of Chain::Lock and just avoid adding a new one, it still returns both block height & hash

I’m removing the other findFirstBlockWithTimeAndHeight call later in 3f1b867a096ac24073c59ecd2c660e07cfc2be50 in #15719. I didn’t remove it here, because I wanted to keep this PR a little smaller and more limited in scope. I also didn’t want to add an extra change for reviewers in code just that’s going to be deleted later.

But I think #16426 could make the change you’re suggesting. I’m pretty sure we’re going to merge #16426 before #15719 so it would make sense to have there

in src/interfaces/chain.cpp:265 in 6067b74431 outdated

258@@ -259,6 +259,16 @@ class ChainImpl : public Chain
259         }
260         return true;
261     }
262+    Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) override
263+    {
264+        LOCK(::cs_main);
265+        CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(min_time, min_height);

ariard commented at 9:40 pm on February 13, 2020:

6067b74

Just to be sure but is FindEarliestAtLeast working as intended ? By passing min_height=0 std::lower_bound is returning an iterator to the first element for which the comparison object return false, thus with the current comparison being pBlock->GetBlockTimeMax() < blockparams.first || pBlock->nHeight < blockparams.second it would return just after the genesis block ?

ryanofsky commented at 11:15 pm on February 13, 2020:

re: #17954 (review)

6067b74

Just to be sure but is FindEarliestAtLeast working as intended ? By passing min_height=0 std::lower_bound is returning an iterator to the first element for which the comparison object return false, thus with the current comparison being pBlock->GetBlockTimeMax() < blockparams.first || pBlock->nHeight < blockparams.second it would return just after the genesis block ?

It seems right because pBlock->nHeight < 0 will be false for the genesis block so the lambda should be false, and lower_bound should stop there, returning the genesis block. This comes from #15670, by the way.

ariard commented at 11:28 pm on February 14, 2020:

Hmmm if I understand RescanFromTime expected behavior is to find earliest block with both nTime and height superior at the ones passed not either so sounds like I broke it ?

ryanofsky commented at 11:51 pm on February 14, 2020:

re: #17954 (review)

#15670 seems right to me, at least at first glance. a || b can only be false if both a and b are false

in src/wallet/rpcwallet.cpp:3577 in 9aa4b6bb6f outdated

3584-            // rescan).
3585-            if (stop_height) {
3586-                stop_block = locked_chain->getBlockHash(*stop_height);
3587-            }
3588-        }
3589+        start_block = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height);

ariard commented at 10:15 pm on February 13, 2020:

9aa4b6b

I find findAncestorByHeight unclear, here we may have start_height and GetLastBlockHash not pointing to same block. Behavior follows method documentation but why bother asking for the hash, query in ChainActive with the provided height ?

Honestly here I would prefer to stick with getBlockHash, behavior is more straightforward.

ryanofsky commented at 11:16 pm on February 13, 2020:

re: #17954 (review)

9aa4b6b

I find findAncestorByHeight unclear, here we may have start_height and GetLastBlockHash not pointing to same block. Behavior follows method documentation but why bother asking for the hash, query in ChainActive with the provided height ?

Honestly here I would prefer to stick with getBlockHash, behavior is more straightforward.

Maybe findAncestorByHeight needs a better name, but it is supposed to be a direct replacement for getBlockHash that turns a block height into a block hash. The only difference is that getBlockHash will return different values depending on the current tip, while findAncestorByHeight is more stable and always returns the same values regardless of the tip.

in src/wallet/wallet.cpp:1652 in 9aa4b6bb6f outdated

1648@@ -1649,7 +1649,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1649         }
1650         block_height = locked_chain->getBlockHeight(block_hash);
1651         progress_begin = chain().guessVerificationProgress(block_hash);
1652-        progress_end = chain().guessVerificationProgress(stop_block.IsNull() ? tip_hash : stop_block);
1653+        progress_end = chain().guessVerificationProgress(max_height ? chain().findAncestorByHeight(tip_hash, *max_height) : tip_hash);

ariard commented at 10:19 pm on February 13, 2020:

9aa4b6b

Same here, why ScanForWalletTransactions function to then add a call to get previously furnished information ? I would prefer to keep removed getBlockHash calls in rescanblockchain

ryanofsky commented at 11:19 pm on February 13, 2020:

9aa4b6b

Same here, why ScanForWalletTransactions function to then add a call to get previously furnished information ? I would prefer to keep removed getBlockHash calls in rescanblockchain

I don’t think that would be an improvement, or know what advantages you see there.

The problem with getBlockHash calls is that their behavior varies depending on the current node tip. Wallet code is simpler and easier to reason about it only has to deal the last block processed and not have to reconcile last processed information with the node tip. This is why rescanblockchain function gets shorter and simpler as a result of this change (and longer and more complicated in the current #16426)

This commit just tweaks 3 lines of code in ScanForWalletTransactions, and don’t seem too significant. The next commit e1381908537267a937bbd3b83eb00f2fa562928e simplifies ScanForWalletTransactions a little more, though.

ariard commented at 10:26 pm on February 13, 2020: member

Reviewed until 9aa4b6b

I think this approach is better than mine in #16426 by documenting better behavior changes. However I would prefer to avoid introducing to much new method with less-understood behaviors, is the design goal of this patchset still to avoid any reorg-unsafe-method after chain lock is removed ?

ryanofsky commented at 0:57 am on February 14, 2020: member

Thank you Antoine for detailed review! I tried to answer all your comments below

sidhujag referenced this in commit 9b2d026754 on Feb 18, 2020

ryanofsky commented at 4:04 pm on February 19, 2020: member

Review club notes at https://bitcoincore.reviews/17954.html. Meeting in 2 hours if I’m time zoning correctly

jonatack commented at 5:54 pm on February 19, 2020: member

Concept ACK. Built/ran tests, code review in progress.

in src/interfaces/chain.h:146 in e276b68214 outdated

141     virtual bool findAncestorByHash(const uint256& block_hash,
142         const uint256& ancestor_hash,
143         int* height = 0) = 0;
144 
145+    //! Find most recent common ancestor between two blocks and optionally
146+    //! return its hash and/or height. Also return height of first block. Return

luke-jr commented at 6:48 pm on February 19, 2020:

I think it’s confusing to return something unrelated to the common ancestor here…

ryanofsky commented at 7:53 pm on February 19, 2020:

re: #17954 (review)

I think it’s confusing to return something unrelated to the common ancestor here…

Agreed, will backport 25c1ae48204215622bc9fd3a8bc9677f15c32674 (branch) as soon as I get a chance

in src/interfaces/chain.h:143 in c26215addf outdated

135@@ -136,6 +136,12 @@ class Chain
136         int64_t* time = nullptr,
137         int64_t* max_time = nullptr) = 0;
138 
139+    //! Return whether block descends from a specified ancestor, and
140+    //! optionally return height of the ancestor.
141+    virtual bool findAncestorByHash(const uint256& block_hash,
142+        const uint256& ancestor_hash,
143+        int* height = 0) = 0;

fjahr commented at 10:15 am on February 21, 2020:

nit: Maybe using std::numeric_limits<int>::max() would have been a tiny bit nicer because it would have allowed passing in a default initialized height.

ryanofsky commented at 6:14 pm on February 24, 2020:

re: #17954 (review)

nit: Maybe using std::numeric_limits<int>::max() would have been a tiny bit nicer because it would have allowed passing in a default initialized height.

Should be resolved. Height was just a pointer because it was an output parameter. But now the FoundBlock class is used to return information instead of a height pointer

in src/wallet/rpcdump.cpp:365 in c26215addf outdated

358@@ -359,8 +359,9 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
359     }
360 
361     auto locked_chain = pwallet->chain().lock();
362-    Optional<int> height = locked_chain->getBlockHeight(merkleBlock.header.GetHash());
363-    if (height == nullopt) {
364+    LOCK(pwallet->cs_wallet);
365+    int height;

fjahr commented at 2:32 pm on February 21, 2020:

I think this means height will not be set within findAncestorByHash() and stay 0. Since it seems to not be needed it can probably be removed. Then passing an explicit 0 into the Confirmation constructor makes it more explicit that this value is not used/needed.

ryanofsky commented at 6:14 pm on February 24, 2020:

re: #17954 (review)

I think this means height will not be set within findAncestorByHash() and stay 0. Since it seems to not be needed it can probably be removed. Then passing an explicit 0 into the Confirmation constructor makes it more explicit that this value is not used/needed.

height is passed as output argument to findAncestorByHash below, and initialized by that call. The height value does get used and shouldn’t actually be 0

in src/interfaces/chain.cpp:262 in e276b68214 outdated

258@@ -259,6 +259,15 @@ class ChainImpl : public Chain
259         }
260         return true;
261     }
262+    uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) override

fjahr commented at 3:01 pm on February 21, 2020:

nit: The other functions around here follow a different style, returning a bool. I would have preferred to keep this consistent.

ryanofsky commented at 6:13 pm on February 24, 2020:

re: #17954 (review)

nit: The other functions around here follow a different style, returning a bool. I would have preferred to keep this consistent.

Yes, this is better. It returns a bool now.

in src/interfaces/chain.cpp:286 in 9aa4b6bb6f outdated

294@@ -309,6 +295,17 @@ class ChainImpl : public Chain
295         LOCK(cs_main);
296         return GuessVerificationProgress(Params().TxData(), LookupBlockIndex(block_hash));
297     }
298+    bool hasBlocks(const uint256& block_hash, int min_height, Optional<int> max_height) override

fjahr commented at 3:46 pm on February 21, 2020:

nit: Style-wise I find the use of Optional here a bit weird because there are other, more common ways to make an argument optional.

ryanofsky commented at 6:14 pm on February 24, 2020:

re: #17954 (review)

nit: Style-wise I find the use of Optional here a bit weird because there are other, more common ways to make an argument optional.

Probably most common way we denote optional heights is to use -1 as a magic unset height value. But I think using Optional and nullopt is nicer here because it is more explicit and also because the rest of the interfaces/chain code is currently using Optional instead of -1.

Since this is a maximum height and the last function parameter, another approach would be to use a std::numeric_limits<int>::max() default argument value. But IMO, while default argument values are great for optional outputs, for optional inputs they can be confusing and lead to bugs, especially when there are multiple arguments of the same type and the compiler can’t check when a value is passed as the wrong argument

fjahr approved

fjahr commented at 4:40 pm on February 21, 2020: member

Code review ACK 1125fd92bd3ee6d44ab9f8118ac68c0f621c5a94 (also built and ran tests)

Had some nits, this could get merged but I would be happy if you could address my comments on height if you still make changes.

ryanofsky force-pushed on Feb 24, 2020

ryanofsky commented at 10:39 pm on February 24, 2020: member

Thanks for reviews and discussion!

Updated 1125fd92bd3ee6d44ab9f8118ac68c0f621c5a94 -> ace85fbe3516ca02d4bbc6e5e0d677f2cf155933 (pr/unlock.9 -> pr/unlock.10, compare) adding FoundBlock class so find block methods can return information in a uniform way

re: #17954#pullrequestreview-362525933

Had some nits, this could get merged but I would be happy if you could address my comments on height if you still make changes.

You had a few comments about heights, and think I resolved or addressed them all, but let me know!

fanquake referenced this in commit 48fef5ebae on Feb 28, 2020

in src/interfaces/chain.h:45 in 3e64b9e0d5 outdated

40+    FoundBlock& maxTime(int64_t& max_time) { m_max_time = &max_time; return *this; }
41+    FoundBlock& mtpTime(int64_t& mtp_time) { m_mtp_time = &mtp_time; return *this; }
42+    //! Read block data from disk. If the block exists but doesn't have data
43+    //! (for example due to pruning), the CBlock variable will be set to null.
44+    FoundBlock& data(CBlock& data) { m_data = &data; return *this; }
45+    //! Require block, and check that it exists with CHECK_NONFATAL.

ariard commented at 6:26 pm on February 28, 2020:

3e64b9e

I really like this new helper class, just what do you think here of enforching check with a boolean flag to FillBlock and upper level method instead of a attribute setup by FoundBlock constructor caller. E.g in WalletTxToJSON, findBlock is called and FoundBlock constructed with a check requirement, which then calls LookupBlockIndex and FillBlock, and only in this last function the check is going to be enforced.

ryanofsky commented at 3:29 pm on March 2, 2020:

re: #17954 (review)

3e64b9e

what do you think here of enforching check with a boolean flag to FillBlock and upper level method instead of a attribute setup by FoundBlock constructor caller.

I didn’t really think about it, but looking again, the .require() method is pretty pointless. It’s easier and clearer to just use CHECK_NONFATAL at the call sites directly. I updated the PR to do this and drop require(), but let me know if you think more changes still make sense.

in src/interfaces/chain.h:159 in 153f74900e outdated

152@@ -153,6 +153,12 @@ class Chain
153     //! or contents.
154     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
155 
156+    //! Return whether block descends from a specified ancestor, and
157+    //! optionally return block information.
158+    virtual bool findAncestorByHash(const uint256& block_hash,

ariard commented at 6:48 pm on February 28, 2020:

153f749

Also why not adding a FoundBlock& ancestor(uint256& hash) { .. } and let FillBlock check if ancestor exists ? (once you understand FoundBlock helper class, that’s easier to reason on than adding one-use method IMO)

ryanofsky commented at 3:29 pm on March 2, 2020:

re: #17954 (review)

153f749

Also why not adding a FoundBlock& ancestor(uint256& hash) { .. } and let FillBlock check if ancestor exists ? (once you understand FoundBlock helper class, that’s easier to reason on than adding one-use method IMO)

FoundBlock is currently more of a struct with accessor methods than a real class with methods that execute code. If it became a real class wrapping a BlockIndex*, wallet code would no longer be able to construct it locally, instead it would have to make IPC calls to the node to create and destroy it. Also the new methods would have to be virtual and forward from the wallet to node process.

The interface would also seem less better organized. I think it’s nice for findBlock findFirstBlockWithTimeAndHeight findNextBlock findAncestorByHeight findAncestorByHash findCommonAncestor to all be methods of the same class and all work the same way, than to be in different classes and follow different conventions.

I do think a followup could unify these methods, something like:

 0chain.findBlock(hash, FoundBlock.height(out_height).time(out_time));                                // current
 1chain.findBlock().hash(hash).getHeight(out_height).getTime(out_time);                               // new
 2
 3chain.findAncestorByHash(block, ancestor, FoundBlock.height(out_height).time(out_time));            // current
 4chain.findBlock().hash(ancestor).descendant(block).getHeight(out_height).getTime(out_time);         // new
 5
 6chain.findAncestorByHeight(block, height, FoundBlock.height(out_height).time(out_time));            // current
 7chain.findBlock().height(height).descendant(block).getHeight(out_height).getTime(out_time);         // new
 8
 9chain.findCommonAncestor(block1, block2, FoundBlock.height(out_height).time(out_time));             // current 
10chain.findBlock().descendant(block1).descendant(block2).getHeight(out_height).getTime(out_time);    // new
11
12chain.findNextBlock(hash, height, FoundBlock.height(out_height).time(out_time));                    // current
13chain.findBlock().hash(hash).height(height).next(), FoundBlock.height(out_height).time(out_time));  // new
14
15chain.findFirstBlockWithTimeAndHeight(time, height, FoundBlock.height(out_height).time(out_time));  // current
16chain.findBlock().minTime(time).minHeight(height).getHeight(out_height).getTime(out_time));         // new

But this is kind of baroque and I didn’t want to attempt something like that here, even though it could be a followup

ariard commented at 6:43 pm on March 5, 2020:

Okay I get your point with struct-with-accessor-methods vs real-class-with-methods-that-execute-code wrt with IPC/memory separation. My assumption here was we should clean up completely these methods by storing more inside the wallet (like any block header tied to a transaction which matters for us), but that something we should discuss in future PRs/issues. I’m fine with Chain API right now, let’s move forward

in src/interfaces/chain.h:155 in 9da0e4121b outdated

148@@ -149,12 +149,24 @@ class Chain
149     //! or contents.
150     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
151 
152+    //! Find ancestor of block at specified height and optionally return
153+    //! ancestor information.
154+    virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor={}) = 0;

ariard commented at 7:18 pm on February 28, 2020:

9da0e41

Same here, I think you can make findAncestorByHeight and findCommonAncestor as FoundBlock methods (at least I’ve tried for findAncestorByHeight it works well)

ryanofsky commented at 3:29 pm on March 2, 2020:

re: #17954 (review)

9da0e41

Same here, I think you can make findAncestorByHeight and findCommonAncestor as FoundBlock methods (at least I’ve tried for findAncestorByHeight it works well)

It’s not clear when you would want a function to be a member of the chain class vs the block class. Having all functions side by side seems like the simplest starting point

ryanofsky commented at 3:16 am on April 13, 2020:

re: #17954 (review)

08211e6

Updated

in src/wallet/wallet.cpp:1647 in 9701379d37 outdated

1653-        if (max_height) chain().findAncestorByHeight(tip_hash, *max_height, FoundBlock().hash(end_hash));
1654-        progress_begin = chain().guessVerificationProgress(block_hash);
1655-        progress_end = chain().guessVerificationProgress(end_hash);
1656-    }
1657+    uint256 tip_hash = WITH_LOCK(cs_wallet, return GetLastBlockHash());
1658+    uint256 end_hash = tip_hash;

ariard commented at 7:59 pm on February 28, 2020:

9701379

Was a bit confused at first, would comment code, here “If a max_height is provided, do a rescan from start_block to it. Otherwise use wallet tip hash as an ending point”

ryanofsky commented at 3:33 pm on March 2, 2020:

re: #17954 (review)

9701379

Was a bit confused at first, would comment code, here “If a max_height is provided, do a rescan from start_block to it. Otherwise use wallet tip hash as an ending point”

Thanks, added a similar comment

in src/wallet/wallet.cpp:2683 in bfa71f8561 outdated

2532@@ -2532,7 +2533,6 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, interface
2533         // unique "nLockTime fingerprint", set nLockTime to a constant.
2534         locktime = 0;
2535     }
2536-    assert(locktime <= height);

ariard commented at 8:20 pm on February 28, 2020:

bfa71f8

You may keep the assert against block_height parameter?

ryanofsky commented at 3:33 pm on March 2, 2020:

re: #17954 (review)

bfa71f8

You may keep the assert against block_height parameter?

If you think it helps, I can add this back, but I did remove it intentionally. It seemed pointless to assert locktime is less than the height just after setting it to the height, something like

0a = b + c;
1assert(a == b + c);

in src/wallet/wallet.cpp:2602 in bfa71f8561 outdated

2599+    {
2600+        LOCK(cs_wallet);
2601+        tip_hash = GetLastBlockHash();
2602+        tip_height = GetLastBlockHeight();
2603+    }
2604+    txNew.nLockTime = GetLocktimeForNewTransaction(chain(), tip_hash, tip_height);

ariard commented at 8:34 pm on February 28, 2020:

bfa71f8

We take another wallet lock just few lines behind, I think you can move the call to GetLocktimeForNewTransaction there, shouldn’t change anything.

ryanofsky commented at 3:33 pm on March 2, 2020:

re: #17954 (review)

bfa71f8

We take another wallet lock just few lines behind, I think you can move the call to GetLocktimeForNewTransaction there, shouldn’t change anything.

Thanks, moved under the existing wallet lock

in src/interfaces/chain.cpp:245 in 06460d3c2d outdated

259@@ -260,6 +260,11 @@ class ChainImpl : public Chain
260         WAIT_LOCK(cs_main, lock);
261         return FillBlock(LookupBlockIndex(hash), block, lock);
262     }
263+    bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block) override
264+    {
265+        WAIT_LOCK(cs_main, lock);
266+        return FillBlock(ChainActive().FindEarliestAtLeast(min_time, min_height), block, lock);

ariard commented at 8:59 pm on February 28, 2020:

Re: #17954 (review)

After reading again semantics of std::lower_bound comp I think you’re right, while block timestamp is inferior at min_time, iterator is going to keep moving forward, whatever min_height in this case.

ariard commented at 9:07 pm on February 28, 2020: member

Code review ACK ace85fb minus agreeing on what the behavior changes is #17954 (review) (by switching for findAncestorByHeight I still think you now return a previous block as lastblock)

Other suggestions are leaning using more the new FoundBlock helper class instread of adding new methods. I would prefer but I’m fine with how it is right now.

ryanofsky force-pushed on Mar 2, 2020

ryanofsky commented at 8:49 pm on March 2, 2020: member

Updated ace85fbe3516ca02d4bbc6e5e0d677f2cf155933 -> 4c7ae7319e5796e4aa7011d26b2dfd6bca4ebe45 (pr/unlock.10 -> pr/unlock.11, compare)

ariard approved

ariard commented at 6:47 pm on March 5, 2020: member

Code Review ACK 4c7ae73. Changes since last time are more comments, removing require method from FoundBlock, avoiding short-lived wallet locking.

fjahr commented at 2:42 pm on March 11, 2020: member

ACK 4c7ae7319e5796e4aa7011d26b2dfd6bca4ebe45

Reviewed code, built and ran tests locally. It took me a moment to get used to the FoundBlock abstraction but I agree that it’s an improvement.

hebasto commented at 4:35 pm on March 11, 2020: member

Concept ACK.

in src/wallet/rpcwallet.cpp:3539 in 4c7ae7319e outdated

3546@@ -3546,22 +3547,23 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
3547     }
3548 
3549     int start_height = 0;
3550-    uint256 start_block, stop_block;
3551+    Optional<int> stop_height;

hebasto commented at 3:01 pm on March 20, 2020:

g++ compiler -Wmaybe-uninitialized warning:

0wallet/rpcwallet.cpp: In function ‘UniValue rescanblockchain(const JSONRPCRequest&)’:
1wallet/rpcwallet.cpp:3550:19: warning: ‘*((void*)& stop_height +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
2     Optional<int> stop_height;
3                   ^~~~~~~~~~~

Could be

0#include <optional.h>
1    Optional<int> stop_height = MakeOptional(false, int());

?

ryanofsky commented at 6:10 pm on March 20, 2020:

re: #17954 (review)

g++ compiler -Wmaybe-uninitialized warning:

This is a known false positive with no side effects from an old compiler. https://www.boost.org/doc/libs/1_72_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html

I don’t think making code less readable to silence these is a good tradeoff. But if silencing them is actually important, we should at least have an automated check, like a linter or an old gcc running on travis and failing so there doesn’t have to be a manual reporting, update, and review cycle each time a new instance turns up.

hebasto commented at 8:08 pm on March 20, 2020:

I don’t think making code less readable to silence these is a good tradeoff.

Let me add some context: #14711#pullrequestreview-193702611, #15292, #18052

But if silencing them is actually important, we should at least have an automated check, like a linter or an old gcc running on travis and failing so there doesn’t have to be a manual reporting, update, and review cycle each time a new instance turns up.

Do you mean adding of the -Werror=maybe-uninitialized option to a compiler on Travis?

ryanofsky commented at 8:45 pm on March 20, 2020:

re: #17954 (review)

Do you mean adding of the -Werror=maybe-uninitialized option to a compiler on Travis?

I don’t want to make code less readable and I don’t want to spend time on an going basis in every PR that uses Optional to have a back and forth discussion and extra review cycles just because an old compiler prints a harmless, nonsensical warning.

If you disagree and care about these false positive warnings, adding an automated check that catches them on travis should save all of us effort as we continue to use Optional variables more places. Maybe that automated check would be a linter, maybe it would be a changed operating system setting or -Werror flag on travis. Again I don’t really want these checks, but they would probably work and save some time if we have to spend time this way.

in src/qt/test/wallettests.cpp:156 in 4c7ae7319e outdated

155@@ -156,7 +156,7 @@ void TestGUI(interfaces::Node& node)
156 
157         WalletRescanReserver reserver(wallet.get());
158         reserver.reserve();
159-        CWallet::ScanResult result = wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);
160+        CWallet::ScanResult result = wallet->ScanForWalletTransactions(Params().GetConsensus().hashGenesisBlock, 0 /* block height */, {} /* max height */, reserver, true /* fUpdate */);

hebasto commented at 4:05 pm on March 20, 2020:

Why auto locked_chain = wallet->chain().lock(); is still needed?

ryanofsky commented at 6:08 pm on March 20, 2020:

re: #17954 (review)

Why auto locked_chain = wallet->chain().lock(); is still needed?

Good catch, simplified this code

ryanofsky force-pushed on Mar 20, 2020

ryanofsky commented at 6:39 pm on March 20, 2020: member

Thanks for review!

Updated 4c7ae7319e5796e4aa7011d26b2dfd6bca4ebe45 -> 0539e740fb4962d453564adb28e383c77594512d (pr/unlock.11 -> pr/unlock.12, compare) with suggested test simplification

hebasto commented at 7:20 pm on March 20, 2020: member

@ryanofsky It seem CI fails due to the conflict between 80468a97cba27faa9297b86eb901221701d8b13b and #18234.

in src/interfaces/chain.cpp:260 in 7e870974e6 outdated

260+    {
261+        WAIT_LOCK(cs_main, lock);
262+        if (const CBlockIndex* block = LookupBlockIndex(block_hash)) {
263+            if (const CBlockIndex* ancestor = block->GetAncestor(ancestor_height)) {
264+                return FillBlock(ancestor, ancestor_out, lock);
265+            }

hebasto commented at 8:33 pm on March 20, 2020:

nit:

0            const CBlockIndex* ancestor = block->GetAncestor(ancestor_height);
1            return FillBlock(ancestor, ancestor_out, lock);

ryanofsky commented at 3:58 pm on March 23, 2020:

re: #17954 (review)

nit:

I don’t see any advantage in this, it is just making the function less consistent internally. It would help to state what perceived advantages are with suggestions like this.

ryanofsky commented at 9:00 pm on March 20, 2020: member

re: #17954 (comment)

@ryanofsky It seem CI fails due to the conflict between 80468a9 and #18234.

Thanks! Rebased 0539e740fb4962d453564adb28e383c77594512d -> 40a8796ad1a4e6032c466c38b8887e8f1e1022a4 (pr/unlock.12 -> pr/unlock.13, compare) to fix silent merge conflict with #18234

ryanofsky force-pushed on Mar 20, 2020

in src/interfaces/chain.cpp:285 in f6c729977a outdated

280+    {
281+        LOCK(::cs_main);
282+        if (CBlockIndex* block = LookupBlockIndex(block_hash)) {
283+            if (max_height && block->nHeight >= *max_height) block = block->GetAncestor(*max_height);
284+            for (; block->nStatus & BLOCK_HAVE_DATA; block = block->pprev) {
285+                if (block->nHeight < min_height || !block->pprev) return true;

hebasto commented at 9:13 pm on March 20, 2020:

Why the second check !block->pprev is needed?

ryanofsky commented at 3:59 pm on March 23, 2020:

re: #17954 (review)

Why the second check !block->pprev is needed?

Semantics of what hasBlocks should return when blocks don’t exist is arbitrary, but I wrote it to consistently return false if any blocks that exist in the specified range are missing data, and true otherwise. There are test cases to ensure this works consistently for min_height and max_height

< earlier on this line should have been <= though, so I fixed this and added some more test cases for edge conditions,

in src/interfaces/chain.h:151 in 3989c851c3 outdated

147@@ -148,6 +148,11 @@ class Chain
148     //! information.
149     virtual bool findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, const FoundBlock& block={}) = 0;
150 
151+    //! Find next block if block is part of current chain. Also flag if
152+    //! there was a reorg and the specified block hash is no longer in the
153+    //! current chain, and optionally return block information.
154+    virtual bool findNextBlock(const uint256& block_hash, int block_height, const FoundBlock& next={}, bool* reorg=nullptr) = 0;

hebasto commented at 9:26 pm on March 20, 2020:

nit: parameter names in the function declaration differ from ones in the function definition:

FoundBlock& next vs FoundBlock& block_out
bool* reorg vs bool* reorg_out

ryanofsky commented at 4:00 pm on March 23, 2020:

re: #17954 (review)

nit: parameter names in the function declaration differ from ones in the function definition:

FoundBlock& next vs FoundBlock& block_out

bool* reorg vs bool* reorg_out

Thanks, switched to names from declaration

in src/wallet/wallet.cpp:1621 in 3989c851c3 outdated

1618@@ -1619,6 +1619,8 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1619  * @param[in] stop_block  Scan ending block. If block is not on the active
1620  *                        chain, the scan will continue until it reaches the
1621  *                        chain tip.

hebasto commented at 9:43 pm on March 20, 2020:

Is this comment still relevant? And the mention of stop_block in @pre comment?

ryanofsky commented at 3:59 pm on March 23, 2020:

re: #17954 (review)

Is this comment still relevant? And the mention of stop_block in @pre comment?

Thanks, removed

in src/wallet/wallet.cpp:2751 in 56dd499eae outdated

2749     {
2750         std::set<CInputCoin> setCoins;
2751         auto locked_chain = chain().lock();
2752         LOCK(cs_wallet);
2753+        tip_hash = GetLastBlockHash();
2754+        tip_height = GetLastBlockHeight();

hebasto commented at 10:04 pm on March 20, 2020:

tip_hash and tip_height could be const:

0    CMutableTransaction txNew;
1    FeeCalculation feeCalc;
2    CAmount nFeeNeeded;
3    int nBytes;
4    {
5        std::set<CInputCoin> setCoins;
6        auto locked_chain = chain().lock();
7        LOCK(cs_wallet);
8        const uint256 tip_hash = GetLastBlockHash();
9        const int tip_height = GetLastBlockHeight();

ryanofsky commented at 4:00 pm on March 23, 2020:

re: #17954 (review)

tip_hash and tip_height could be const:

Thanks, removed these variables that were left over from an earlier version of this commit

hebasto approved

hebasto commented at 10:33 pm on March 20, 2020: member

ACK 40a8796ad1a4e6032c466c38b8887e8f1e1022a4, modulo nits, tested on Linux Mint 19.3.

ariard commented at 3:03 am on March 22, 2020: member

Code Review ACK 40a8796, changes since last ACK is removing a useless lock tacking in qt test.

(stale Travis?)

DrahtBot added the label Needs rebase on Mar 23, 2020

ryanofsky force-pushed on Mar 24, 2020

ryanofsky commented at 3:25 pm on March 24, 2020: member

Thanks for review and reack!

Rebased 40a8796ad1a4e6032c466c38b8887e8f1e1022a4 -> 19e1db77cbc08f451a3508bb113f2f7cf5a13616 (pr/unlock.13 -> pr/unlock.14, compare) due to conflict with #18278, also fixed a hasBlocks edge case and added tests, and made some suggested minor code cleanups

DrahtBot removed the label Needs rebase on Mar 24, 2020

ariard commented at 3:39 am on March 25, 2020: member

Code Review ACK 19e1db7

Changes since last time are better documentation, hasBlock fix for the lower bound, findNextBlock internal simplification.

I agree with @hebasto, block->pprev is quite confusing, would be happy to reack a hasBlock documentation change to lay out this.

ryanofsky force-pushed on Mar 25, 2020

ryanofsky commented at 1:01 pm on March 25, 2020: member

Thanks for review!

Updated 19e1db77cbc08f451a3508bb113f2f7cf5a13616 -> cdea18ae2dad4a198df65d0043e10c45a22994e3 (pr/unlock.14 -> pr/unlock.15, compare) adding suggested comment

re: #17954 (comment)

I agree with @hebasto, block->pprev is quite confusing, would be happy to reack a hasBlock documentation change to lay out this.

Added comments, but I am surprised this null check is surprising, range cases are exhaustively checked in the unit test and this just makes results stable and not crashy.

ariard commented at 6:05 pm on March 25, 2020: member

Code Review ACK cdea18a

What confusing here isn’t the null check but the decision to return true in this case, because you may have a void block is the given ranged and still return true.

hebasto commented at 2:04 pm on March 28, 2020: member

re-ACK cdea18ae2dad4a198df65d0043e10c45a22994e3

promag commented at 0:11 am on March 30, 2020: member

Core review ACK cdea18ae2dad4a198df65d0043e10c45a22994e3.

DrahtBot added the label Needs rebase on Mar 31, 2020

refactor: Add interfaces::FoundBlock class to selectively return block data

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.

bf30cd4922

wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances

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.

f6da44ccce

wallet refactor: Avoid use of Chain::Lock in qt wallettests

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change doesn't affect behavior.

ade5f87971

wallet: Avoid use of Chain::Lock in importprunedfunds

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case the "Block not found in chain" error
will be stricter and not allow importing data from a blocks between the wallet
last processed tip and the current node tip.

c1694ce6bb

wallet: Avoid use of Chain::Lock in importwallet and dumpwallet

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it will use more accurate backup and
rescan timestamps.

25a9fcf9e5

wallet: Avoid use of Chain::Lock in importmulti

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case it may use a more accurate rescan
time.

bc96a9bfc6

wallet: Avoid use of Chain::Lock in listsinceblock

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
https://github.com/bitcoin/bitcoin/issues/14338#issuecomment-426706574

f7ba881bc6

wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change has no effect on behavior.

3cb85ac594

wallet: Avoid use of Chain::Lock in rescanblockchain

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height

1be8ff280c

wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change affects behavior in a few small ways.

- If there's no max_height specified, percentage progress is measured ending at
  wallet last processed block instead of node tip

- More consistent error reporting: Early check to see if start_block is on the
  active chain is removed, so start_block is always read and the triggers an
  error if it's unavailable

c0d07dc4cb

wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it may set a different lock time.

e958ff9ab5

wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes

This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, where it will treat the last block processed as the
current tip.

48973402d8

ryanofsky force-pushed on Mar 31, 2020

ryanofsky commented at 3:35 pm on March 31, 2020: member

Rebased cdea18ae2dad4a198df65d0043e10c45a22994e3 -> 710b077f9c0d57a40073ceb9e5568ce37d8cdfe2 (pr/unlock.15 -> pr/unlock.16, compare) due to conflict with #18160

DrahtBot removed the label Needs rebase on Mar 31, 2020

hebasto commented at 1:41 pm on April 2, 2020: member

Reviewed recent rebase:

in interfaces/wallet.cpp #18160 moved if (!force && num_blocks == cached_num_blocks) return false; up, but in f6da44ccce4cfff53433e665305a6fe0a01364e4 it moved back. Could this break behavior introduced by #18160?

ryanofsky commented at 1:55 pm on April 2, 2020: member

Reviewed recent rebase:

in interfaces/wallet.cpp #18160 moved if (!force && num_blocks == cached_num_blocks) return false; up, but in f6da44c it moved back. Could this break behavior introduced by #18160?

No, the point of #18160 is to skip recomputing balances when not forced by a transaction update or tip change. Commit f6da44ccce4cfff53433e665305a6fe0a01364e4 is making the tip change detection a little more accurate. An extra lock is required to do this, but GetBalances is still not called. The extra lock is also removed in #16426.

hebasto commented at 2:01 pm on April 2, 2020: member

re-ACK 710b077f9c0d57a40073ceb9e5568ce37d8cdfe2

ariard commented at 1:36 am on April 3, 2020: member

Code Review ACK 710b077

in src/interfaces/chain.h:157 in e0b02c8cb3 outdated

149@@ -150,6 +150,12 @@ class Chain
150     //! or contents.
151     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
152 
153+    //! Return whether block descends from a specified ancestor, and
154+    //! optionally return block information.
155+    virtual bool findAncestorByHash(const uint256& block_hash,
156+        const uint256& ancestor_hash,
157+        const FoundBlock& block={}) = 0;

MarcoFalke commented at 6:34 pm on April 10, 2020:

in commit e0b02c8cb3

0        const FoundBlock& ancestor_out={}) = 0;

ryanofsky commented at 9:50 pm on April 10, 2020:

re: #17954 (review)

in commit e0b02c8

Updated

in src/wallet/rpcdump.cpp:796 in d83fd92520 outdated

794-    file << strprintf("# * Best block at time of backup was %i (%s),\n", tip_height.get_value_or(-1), tip_height ? locked_chain->getBlockHash(*tip_height).ToString() : "(missing block hash)");
795-    file << strprintf("#   mined on %s\n", tip_height ? FormatISO8601DateTime(locked_chain->getBlockTime(*tip_height)) : "(missing block time)");
796+    file << strprintf("# * Best block at time of backup was %i (%s),\n", pwallet->GetLastBlockHeight(), pwallet->GetLastBlockHash().ToString());
797+    int64_t block_time = 0;
798+    CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(block_time)));
799+    file << strprintf("#   mined on %s\n", block_time);

MarcoFalke commented at 6:46 pm on April 10, 2020:

in commit d83fd92520

This is no longer human readable. Idk why the tests don’t fail we used to have at least one parser in the python functional test suite :shrug:

ryanofsky commented at 9:43 pm on April 10, 2020:

re: #17954 (review)

in commit d83fd92

This is no longer human readable. Idk why the tests don’t fail we used to have at least one parser in the python functional test suite

Thanks, confirmed fix with your test from #18597!

in src/interfaces/chain.h:151 in 08211e640f outdated

145@@ -146,12 +146,24 @@ class Chain
146     //! or contents.
147     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
148 
149+    //! Find ancestor of block at specified height and optionally return
150+    //! ancestor information.
151+    virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor={}) = 0;

MarcoFalke commented at 6:56 pm on April 10, 2020:

08211e640f

0    virtual bool findAncestorByHeight(const uint256& block_hash, int ancestor_height, const FoundBlock& ancestor_out={}) = 0;

in src/interfaces/chain.h:165 in 08211e640f outdated

160+    //! return block information.
161+    virtual bool findCommonAncestor(const uint256& block_hash1,
162+        const uint256& block_hash2,
163+        const FoundBlock& ancestor={},
164+        const FoundBlock& block1={},
165+        const FoundBlock& block2={}) = 0;

MarcoFalke commented at 6:57 pm on April 10, 2020:

08211e640f

0        const FoundBlock& block2_out={}) = 0;

Same for other args

ryanofsky commented at 9:50 pm on April 10, 2020:

re: #17954 (review)

08211e6

Same for other args

Updated

in src/wallet/rpcwallet.cpp:1586 in 08211e640f outdated

1580@@ -1581,8 +1581,9 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
1581     uint256 blockId;
1582     if (!request.params[0].isNull() && !request.params[0].get_str().empty()) {
1583         blockId = ParseHashV(request.params[0], "blockhash");
1584-        height = locked_chain->findFork(blockId, &altheight);
1585-        if (!height) {
1586+        height.emplace();
1587+        altheight.emplace();
1588+        if (!pwallet->chain().findCommonAncestor(blockId, pwallet->GetLastBlockHash(), FoundBlock().height(*height), FoundBlock().height(*altheight))) {

MarcoFalke commented at 7:22 pm on April 10, 2020:

0        if (!pwallet->chain().findCommonAncestor(blockId, pwallet->GetLastBlockHash(), /* ancestor_out */ FoundBlock().height(*height), /* blockId out */ FoundBlock().height(*altheight))) {

ryanofsky commented at 9:50 pm on April 10, 2020:

re: #17954 (review)

Updated

ryanofsky commented at 8:33 pm on April 10, 2020: member

Current review status:

MarcoFalke commented at 8:53 pm on April 10, 2020: member

Current review status:

Acks don’t help if the changes introduce a bug :red_circle: :dagger:

ACK 710b077f9c, except the introduced bug 🤺

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 710b077f9c, except the introduced bugs 🤺
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUitNQv/QXsc/TXCoGbszrOL5HQSrTpPCe9uPGwCUAwd6MCY8ZDmj7JcMRdBeLJ2
 8iuCPgSMmF1d/GxJbMxTr1J4gVp+dVKkMgneTnPauwgoh537JHEmplmROC8vIixot
 9RxnW6na6vXfj0aEvSoFbhLBvopXdz5HZVUCjS74/o0YZL2tTvlOlAujHjfJqoFh/
10n3XkrCSNtcXyIioJUhgnnH1wFaYPPm8LVgqrgclm03R4kW4hW+Zq6ibLMtZdP+9L
11gLWVlMGfDu+C6XGp8Nw/Znd9TgKEIMLY0Yf2FUGk5jMMTlqtoAcjoMVXHYo59QVX
12U/YAFl15jnTjoGQsZEuB8Ln1KrvkrkOITS8tPVxjFCGQ8niNY+n1FX+hyA4LDitm
13jpUry9hlMswzBP5EFS6SNZ09bho8mHQP/+rnsJ5s3gF2tj+cZN790H8asy+Ml6sG
14rtOT/PLGVV7NezB4YSwcW1uJiKDellWOQrA99b/kjwUHkYr01BIILL6XpBMNELfy
154Qv4y0/D
16=u2+w
17-----END PGP SIGNATURE-----

Timestamp of file with hash d8d68feef8d9815d83b13f49567f9148ee13f92ef1f16677d1e9c1c98efcf014 -

MarcoFalke approved

MarcoFalke commented at 8:55 pm on April 10, 2020: member

Going to merge, so that the ACKs don’t go down the drain

MarcoFalke commented at 8:57 pm on April 10, 2020: member

actually there are only two acks for the current state of the pr, so I won’t merge for now

ryanofsky force-pushed on Apr 13, 2020

ryanofsky commented at 3:19 am on April 13, 2020: member

Updated 710b077f9c0d57a40073ceb9e5568ce37d8cdfe2 -> 48973402d8bccb673eaeb68b7aa86faa39d3cb8a (pr/unlock.16 -> pr/unlock.17, compare) with suggested tweaks and dumpwallet comment fix covered by new test in #18597

MarcoFalke referenced this in commit 4d26312c17 on Apr 13, 2020

MarcoFalke commented at 2:01 pm on April 13, 2020: member

re-ACK 48973402d8, only change is fixing bug 📀

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 48973402d8, only change is fixing bug 📀
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg7CAwAgLetCNTTZ3j5/d54CEHvjQSQCdU0xAxJZfq2LvcRgphElGFiWjqAnNYM
 8QCt734Dk5fMXjqj6PtDz7Py0GO0wiinIXbR+bXdVvPY9v251H/zeXkSkDcC+yLr3
 92J40jb1Ec/uYMBay7nmXjOmmNJwq4FATKNEBZonLVAqL7z4K7dbJVqyWksVnaR8O
10CEUIsEFLjYQ5oTGzuNDL4mLh59lzi/GpSgnManIyLTCn/eEfcDeyTusTIXGnoOG+
116aCYUjLkFTRlkkmUvD3Ife1gGaM5V5/sJ7KsJbrfz5xrrQfJEUN5NXi2m5CL9vJn
120deZmTC3fpbbplW2VJ8gwT7yOwZ4EJPNlvctWFbDapFDov9hSZHb3IlL73XY8d+K
132zpraFjfiiOA/svrngKAd4Jzuj4ZjMHNNNKzLYr0S/0DNmJTfYVYVtGJ+9l/Itl6
14uPbFH1au0xdhAgS36e4FtXx1W3CHx60+thQTHe8z1eQo1Jq0R0Y4qhywF2cDfeHC
15pR94jjdH
16=est8
17-----END PGP SIGNATURE-----

Timestamp of file with hash 059943689472c2b5db5fc3c5f99636fdd7a6eb8ce233f24227b7d283f6c3d208 -

fjahr commented at 3:59 pm on April 13, 2020: member

re-ACK 48973402d8bccb673eaeb68b7aa86faa39d3cb8a, reviewed rebase and changes since last review, built and ran tests locally

ariard commented at 7:42 am on April 14, 2020: member

Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in findCommonAncestor

MarcoFalke merged this on Apr 14, 2020

MarcoFalke closed this on Apr 14, 2020

sidhujag referenced this in commit c4eb63fbfe on Apr 14, 2020

practicalswift commented at 1:54 pm on April 15, 2020: contributor

@ryanofsky

Post-merge review comment:

0    bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override
1    {
2        WAIT_LOCK(cs_main, lock);
3        const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
4        const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
5        const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
6        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
7    }

Is the use of & instead of && intentional on the return line?

ryanofsky commented at 2:04 pm on April 15, 2020: member

@ryanofsky

Post-merge review comment:

0    bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override
1    {
2        WAIT_LOCK(cs_main, lock);
3        const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
4        const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
5        const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
6        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
7    }

Is the use of & instead of && intentional on the return line?

Yes, because && could short circuit and leave block1_out or block2_out data unfilled

practicalswift commented at 2:16 pm on April 15, 2020: contributor

@ryanofsky

Yes, I was thinking something along the lines of:

 0    bool findCommonAncestor(const uint256& block_hash1, const uint256& block_hash2, const FoundBlock& ancestor_out, const FoundBlock& block1_out, const FoundBlock& block2_out) override
 1    {
 2        WAIT_LOCK(cs_main, lock);
 3        const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
 4        const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
 5        const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
 6        const bool fill1 = FillBlock(ancestor, ancestor_out, lock);
 7        const bool fill2 = FillBlock(block1, block1_out, lock);
 8        const bool fill3 = FillBlock(block2, block2_out, lock);
 9        return fill1 && fill2 && fill3;
10    }

Perhaps a comment could clarify that this is intentional and not a typo?

The current formulation fires off static analyser warnings for all static analysers checking the AutoSAR rules (“Guidelines for the use of the C++14 language in critical and safety-related systems”) or MISRA C++.

AutoSAR (“Guidelines for the use of the C++14 language in critical and safety-related systems”):

Rule M4-5-1 (required, implementation, automated): Expressions with type bool shall not be used as operands to built-in operators other than the assignment operator =, the logical operators &&, ||, !, the equality operators == and ! =, the unary & operator, and the conditional operator.

ryanofsky commented at 2:19 pm on April 15, 2020: member

Feel free to submit a PR. I’d also encourage writing documentation on how to run these static analysers with bitcoin, or maybe run them automatically on travis

MarcoFalke commented at 2:28 pm on April 15, 2020: member

Oh, I didn’t see this during review. I read the method as nothing is filled when the ancestor is not found. I.e. I read the & as &&. Is there any caller that depends on this edge case?

Absent any data, I’d slightly prefer the && version.

ryanofsky commented at 2:34 pm on April 15, 2020: member

Pretty sure nothing requires information about other blocks to be filled when a block isn’t found in this case. It just seemed nicer for the API to return information than not return it, and trivial to implement with no additional code, so I implemented it.

practicalswift commented at 2:41 pm on April 15, 2020: contributor

Which one is preferred of these three? :)

Contender 1.

 0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 1index c8311b298..cd825f870 100644
 2--- a/src/interfaces/chain.cpp
 3+++ b/src/interfaces/chain.cpp
 4@@ -275,7 +275,7 @@ public:
 5         const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
 6         const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
 7         const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
 8-        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
 9+        return FillBlock(ancestor, ancestor_out, lock) && FillBlock(block1, block1_out, lock) && FillBlock(block2, block2_out, lock);
10     }
11     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
12     double guessVerificationProgress(const uint256& block_hash) override

Contender 2.

 0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 1index c8311b298..ed2142884 100644
 2--- a/src/interfaces/chain.cpp
 3+++ b/src/interfaces/chain.cpp
 4@@ -275,7 +275,10 @@ public:
 5         const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
 6         const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
 7         const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
 8-        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
 9+        const bool ancestor_filled = FillBlock(ancestor, ancestor_out, lock);
10+        const bool block1_filled = FillBlock(block1, block1_out, lock);
11+        const bool block2_filled = FillBlock(block2, block2_out, lock);
12+        return ancestor_filled && block1_filled && block2_filled;
13     }
14     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
15     double guessVerificationProgress(const uint256& block_hash) override

Contender 3.

0(empty diff)

MarcoFalke commented at 2:42 pm on April 15, 2020: member

My vote:

51%
49%
0%

ryanofsky commented at 2:43 pm on April 15, 2020: member

I prefer the current code but again please open a separate PR, I don’t think it matters very much

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

HashUnlimited referenced this in commit 74a4bb78c3 on Apr 17, 2020

MarcoFalke commented at 1:13 pm on April 19, 2020: member

In commit f6da44ccce :

The following comment no longer applies and should be fixed up:

0            // Get required locks upfront. This avoids the GUI from getting
1            // stuck if the core is holding the locks for a longer time - for
2            // example, during a wallet rescan.
3            //

MarcoFalke commented at 1:14 pm on April 19, 2020: member

(somehow GitHub didn’t submit my review comment in time ^)

sidhujag referenced this in commit 36bfea40fa on Apr 19, 2020

luke-jr referenced this in commit 06047efe9a on Jun 9, 2020

xdustinface referenced this in commit a57eb27967 on Aug 28, 2020

xdustinface referenced this in commit 6d8153a97a on Aug 28, 2020

xdustinface referenced this in commit 2d8ab5f3b6 on Aug 28, 2020

xdustinface referenced this in commit de1a769c08 on Aug 28, 2020

xdustinface referenced this in commit 1683341563 on Aug 28, 2020

xdustinface referenced this in commit 6c206aa0ba on Aug 28, 2020

xdustinface referenced this in commit 9b70e32d96 on Aug 28, 2020

xdustinface referenced this in commit d0973bb940 on Aug 28, 2020

xdustinface referenced this in commit 951e1302f7 on Aug 28, 2020

xdustinface referenced this in commit dcc3562ca1 on Aug 29, 2020

xdustinface referenced this in commit cceb223aaf on Aug 29, 2020

xdustinface referenced this in commit 273ef3aa32 on Aug 29, 2020

xdustinface referenced this in commit a8d8860069 on Aug 29, 2020

xdustinface referenced this in commit eb72a1275b on Aug 30, 2020

xdustinface referenced this in commit b2ff7243c3 on Aug 30, 2020

xdustinface referenced this in commit 93d6d65a34 on Aug 30, 2020

xdustinface referenced this in commit 590e456bff on Aug 30, 2020

xdustinface referenced this in commit 8f22f0ff7a on Aug 30, 2020

xdustinface referenced this in commit 747c0be07f on Aug 30, 2020

xdustinface referenced this in commit 620e140d9b on Aug 30, 2020

xdustinface referenced this in commit a553b26b13 on Aug 31, 2020

xdustinface referenced this in commit f10d0f8127 on Sep 27, 2020

xdustinface referenced this in commit fee99a16c5 on Sep 29, 2020

xdustinface referenced this in commit 57dbdf304a on Sep 29, 2020

xdustinface referenced this in commit 13bdfff80e on Oct 2, 2020

xdustinface referenced this in commit 7cb8b6bc9d on Oct 2, 2020

xdustinface referenced this in commit 852628ec4e on Oct 5, 2020

xdustinface referenced this in commit ff3104dba3 on Oct 5, 2020

deadalnix referenced this in commit c63adee0dd on Oct 11, 2020

jasonbcox referenced this in commit 1223cd4064 on Oct 11, 2020

jasonbcox referenced this in commit 06409d3c34 on Oct 11, 2020

jasonbcox referenced this in commit 950aa996a0 on Oct 11, 2020

jasonbcox referenced this in commit 987380a9ed on Oct 11, 2020

jasonbcox referenced this in commit c133d6383a on Oct 11, 2020

jasonbcox referenced this in commit 85c542618a on Oct 11, 2020

jasonbcox referenced this in commit cdad5a5b72 on Oct 11, 2020

jasonbcox referenced this in commit 695fa2f2d6 on Oct 11, 2020

jasonbcox referenced this in commit f4fe76343a on Oct 11, 2020

jasonbcox referenced this in commit 08a6658f7f on Oct 11, 2020

jasonbcox referenced this in commit 94adb91eb0 on Oct 11, 2020

xdustinface referenced this in commit 7c6cecaf29 on Oct 14, 2020

van-orton referenced this in commit fb32debaa8 on Oct 30, 2020

sidhujag referenced this in commit ce29ce3b18 on Nov 10, 2020

Fabcien referenced this in commit 527cee36c4 on Jan 2, 2021

Fabcien referenced this in commit e8fb01c940 on Jan 15, 2021

backpacker69 referenced this in commit 0ed77d88d4 on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

wallet: Remove calls to Chain::Lock methods #17954

Conflicts