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

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

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 (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

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.

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

673e0b6

Another candidate for GetLastBlockTime

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

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

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.

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?

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

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

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

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

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

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 (review)

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.

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

re: #17954 (review)

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

Thanks, removed

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 ?

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.

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

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

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

e276b68

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

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.

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

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)

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

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

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

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 ?

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.

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 ?

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

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.

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.

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

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.

I think it's confusing to return something unrelated to the common ancestor here...

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

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.

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

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.

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

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

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.

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.

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

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.

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.

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)

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:

chain.findBlock(hash, FoundBlock.height(out_height).time(out_time));                                // current
chain.findBlock().hash(hash).getHeight(out_height).getTime(out_time);                               // new

chain.findAncestorByHash(block, ancestor, FoundBlock.height(out_height).time(out_time));            // current
chain.findBlock().hash(ancestor).descendant(block).getHeight(out_height).getTime(out_time);         // new

chain.findAncestorByHeight(block, height, FoundBlock.height(out_height).time(out_time));            // current
chain.findBlock().height(height).descendant(block).getHeight(out_height).getTime(out_time);         // new

chain.findCommonAncestor(block1, block2, FoundBlock.height(out_height).time(out_time));             // current 
chain.findBlock().descendant(block1).descendant(block2).getHeight(out_height).getTime(out_time);    // new

chain.findNextBlock(hash, height, FoundBlock.height(out_height).time(out_time));                    // current
chain.findBlock().hash(hash).height(height).next(), FoundBlock.height(out_height).time(out_time));  // new

chain.findFirstBlockWithTimeAndHeight(time, height, FoundBlock.height(out_height).time(out_time));  // current
chain.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

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

9da0e41

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

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

re: #17954 (review)

08211e6

Updated

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"

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

bfa71f8

You may keep the assert against block_height parameter?

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

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

bfa71f8

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

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

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.

g++ compiler -Wmaybe-uninitialized warning:

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

Could be

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

?

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.

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?

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.

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

re: #17954 (review)

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

Good catch, simplified this code

nit:

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

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.

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

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,

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

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

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

re: #17954 (review)

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

Thanks, removed

tip_hash and tip_height could be const:

    CMutableTransaction txNew;
    FeeCalculation feeCalc;
    CAmount nFeeNeeded;
    int nBytes;
    {
        std::set<CInputCoin> setCoins;
        auto locked_chain = chain().lock();
        LOCK(cs_wallet);
        const uint256 tip_hash = GetLastBlockHash();
        const int tip_height = GetLastBlockHeight();

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

in commit e0b02c8cb3

        const FoundBlock& ancestor_out={}) = 0;

re: #17954 (review)

in commit e0b02c8

Updated

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:

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!

08211e640f

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

08211e640f

        const FoundBlock& block2_out={}) = 0;

Same for other args

re: #17954 (review)

08211e6

Same for other args

Updated

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

re: #17954 (review)

Updated