Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: master
from

Conversation

@ryanofsky
Copy link
Contributor

ryanofsky commented Jan 17, 2020

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 ryanofsky changed the title wallet: Remove calls to Chain::Lock methods in wallet wallet: Remove calls to Chain::Lock methods Jan 17, 2020
@fanquake fanquake added the Wallet label Jan 17, 2020
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 17, 2020

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 18, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #18160 (gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged by promag)
  • #17543 (wallet: undo conflicts properly in case of blocks disconnection by ariard)
  • #16224 (gui: Bilingual GUI error messages by hebasto)
  • #14942 (wallet: Make scan / abort status private to CWallet by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ariard
ariard approved these changes Jan 21, 2020
Copy link
Contributor

ariard left a comment

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.

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 21, 2020

Contributor

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

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

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 21, 2020

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 30, 2020

Contributor

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 ?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2020

Author Contributor

re: #17954 (comment)

(Relevant commit is cfc9373)

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

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

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 21, 2020

Contributor

673e0b6

Another candidate for GetLastBlockTime

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 21, 2020

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

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?

This comment has been minimized.

Copy link
@ariard

ariard Jan 30, 2020

Contributor

re: #17954 (comment)

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2020

Author Contributor

re: #17954 (comment)

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

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

@@ -1364,20 +1364,13 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
EnsureWalletIsUnlocked(pwallet);

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

This comment has been minimized.

Copy link
@ariard

ariard Jan 21, 2020

Contributor

1f4b604

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/unlock branch from 1f4b604 to aeba8af Jan 21, 2020
Copy link
Contributor Author

ryanofsky left a comment

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

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

re: #17954 (review)

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

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

cfc9373

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

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

src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
@@ -564,8 +564,7 @@ UniValue importwallet(const JSONRPCRequest& request)
if (!file.is_open()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
}
Optional<int> tip_height = locked_chain->getHeight();
nTimeBegin = tip_height ? locked_chain->getBlockTime(*tip_height) : 0;
pwallet->chain().findBlock(pwallet->GetLastBlockHash(), nullptr, &nTimeBegin);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 21, 2020

Author Contributor

673e0b6

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

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

@ryanofsky ryanofsky marked this pull request as ready for review Jan 22, 2020
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Jan 22, 2020

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

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

@jnewbery jnewbery mentioned this pull request Jan 23, 2020
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/unlock branch from 60e6595 to 0a44d40 Jan 28, 2020
@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Jan 30, 2020

Thanks Russ, will review new changes soon.

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

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

ryanofsky added 11 commits Jan 16, 2020
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

It also helps ensure the GUI display stays up to date in the case where the
node chain height runs ahead of wallet last block processed height.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change doesn't affect behavior.
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.
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.
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.
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
#14338 (comment)
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.

This change has no effect on behavior.
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
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
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.
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.
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/unlock branch from 0a44d40 to d1dd916 Jan 31, 2020
@DrahtBot DrahtBot removed the Needs rebase label Jan 31, 2020
Copy link
Contributor Author

ryanofsky left a comment

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2020

Author Contributor

re: #17954 (comment)

(Relevant commit is cfc9373)

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

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2020

Author Contributor

re: #17954 (comment)

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

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

@laanwj laanwj added this to Blockers in High-priority for review Feb 6, 2020
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/unlock branch 3 times, most recently from 0d788ae to d2f92a9 Feb 6, 2020
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 11, 2020
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
bitcoin@a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117.
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99 from bitcoin#17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

Thanks to John Newbery <john@johnnewbery.com> for catching this while reviewing
bitcoin#17954.
Copy link
Member

jnewbery left a comment

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

@@ -1364,20 +1364,13 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
EnsureWalletIsUnlocked(pwallet);

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

@@ -145,6 +164,9 @@ class Chain
//! the specified block hash are verified.
virtual double guessVerificationProgress(const uint256& block_hash) = 0;

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

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


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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

Thanks updated

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

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

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 11, 2020

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

Thanks, removed

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 12, 2020
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
bitcoin@a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117.
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99 from bitcoin#17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

Thanks to John Newbery <john@johnnewbery.com> for catching this while reviewing
bitcoin#17954.

Github-Pull: bitcoin#18123
Rebased-From: 37d27bc
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 12, 2020
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
bitcoin@a0704a8#r378452145
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99 from bitcoin#17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

MarcoFalke <falke.marco@gmail.com> also points out that a0704a8 could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.

Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing bitcoin#17954.
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 13, 2020
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
bitcoin@a0704a8#r378452145
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99 from bitcoin#17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

MarcoFalke <falke.marco@gmail.com> also points out that a0704a8 could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.

Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing bitcoin#17954.

Github-Pull: bitcoin#18123
Rebased-From: bf36a3c
jonasschnelli added a commit that referenced this pull request Feb 13, 2020
bf36a3c gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky)

Pull request description:

  Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling.

  This bug could cause balances to appear out of date, and was first introduced a0704a8#diff-2e3836af182cfb375329c3463ffd91f8L117. Before that commit, there wasn't a problem because cs_main was held during the poll update.

  Currently, the problem should be rare. But if 8937d99 from #17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification.

  MarcoFalke also points out that a0704a8 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI.

  Thanks to John Newbery for finding this bug this while reviewing #17954.

ACKs for top commit:
  Empact:
    utACK bf36a3c
  jonasschnelli:
    utACK bf36a3c

Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4
@ryanofsky ryanofsky force-pushed the ryanofsky:pr/unlock branch from d2f92a9 to 1125fd9 Feb 13, 2020
Copy link
Contributor Author

ryanofsky left a comment

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

@@ -145,6 +164,9 @@ class Chain
//! the specified block hash are verified.
virtual double guessVerificationProgress(const uint256& block_hash) = 0;

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

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

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


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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

Thanks updated

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 13, 2020

Author Contributor

re: #17954 (comment)

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

Thanks, removed

Copy link
Contributor

ariard left a comment

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 ?

//! Find most recent common ancestor between two blocks and optionally
//! return its hash and/or height. Also return height of first block. Return
//! nullopt if either block is unknown or there is no common ancestor.
virtual Optional<int> findCommonAncestor(const uint256& block_hash1,

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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 ?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

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

EDIT: Newer version 25c1ae4

@@ -1638,8 +1638,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
--*altheight;
}

int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
uint256 lastblock = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms);

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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

@@ -1605,8 +1606,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)

bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());

const Optional<int> tip_height = locked_chain->getHeight();
int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
int depth = height ? pwallet->GetLastBlockHeight() + 1 - *height : -1;

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

e276b68

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

This comment has been minimized.

Copy link
@ariard

ariard Feb 14, 2020

Contributor

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

@@ -133,12 +133,23 @@ class Chain
int64_t* max_time = nullptr,
int64_t* mtp_time = nullptr) = 0;

//! Find ancestor of block at specified height and return its hash.
virtual uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) = 0;

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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 3f1b867 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

//! with a high enough timestamp and height. Also return the block height as
//! an optional output parameter (to avoid the cost of a second lookup in
//! case this information is needed.)
virtual Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) = 0;

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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 3f1b867 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

Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) override
{
LOCK(::cs_main);
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(min_time, min_height);

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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 ?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

This comment has been minimized.

Copy link
@ariard

ariard Feb 14, 2020

Contributor

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 ?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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

stop_block = locked_chain->getBlockHash(*stop_height);
}
}
start_block = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height);

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

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

This comment has been minimized.

Copy link
@ariard

ariard Feb 13, 2020

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

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 e138190 simplifies ScanForWalletTransactions a little more, though.

Copy link
Contributor Author

ryanofsky left a comment

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

Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) override
{
LOCK(::cs_main);
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(min_time, min_height);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

//! with a high enough timestamp and height. Also return the block height as
//! an optional output parameter (to avoid the cost of a second lookup in
//! case this information is needed.)
virtual Optional<uint256> findFirstBlockWithTimeAndHeight(int64_t min_time, int min_height, int* height = nullptr) = 0;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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 3f1b867 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

@@ -133,12 +133,23 @@ class Chain
int64_t* max_time = nullptr,
int64_t* mtp_time = nullptr) = 0;

//! Find ancestor of block at specified height and return its hash.
virtual uint256 findAncestorByHeight(const uint256& block_hash, int ancestor_height) = 0;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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 3f1b867 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

//! Find most recent common ancestor between two blocks and optionally
//! return its hash and/or height. Also return height of first block. Return
//! nullopt if either block is unknown or there is no common ancestor.
virtual Optional<int> findCommonAncestor(const uint256& block_hash1,

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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

@@ -1605,8 +1606,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)

bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());

const Optional<int> tip_height = locked_chain->getHeight();
int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
int depth = height ? pwallet->GetLastBlockHeight() + 1 - *height : -1;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

@@ -1638,8 +1638,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
--*altheight;
}

int last_height = tip_height ? *tip_height + 1 - target_confirms : -1;
uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256();
uint256 lastblock = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), pwallet->GetLastBlockHeight() + 1 - target_confirms);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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

stop_block = locked_chain->getBlockHash(*stop_height);
}
}
start_block = pwallet->chain().findAncestorByHeight(pwallet->GetLastBlockHash(), start_height);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

re: #17954 (comment)

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.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 14, 2020

Author Contributor

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 e138190 simplifies ScanForWalletTransactions a little more, though.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Feb 18, 2020
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.

This bug could cause balances to appear out of date, and was first introduced
bitcoin@a0704a8#r378452145
Before that commit, there wasn't a problem because cs_main was held during the
poll update.

Currently, the problem should be rare. But if
8937d99 from bitcoin#17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.

MarcoFalke <falke.marco@gmail.com> also points out that a0704a8 could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.

Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing bitcoin#17954.
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Feb 19, 2020

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

Copy link
Member

jonatack left a comment

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

//! Return whether block descends from a specified ancestor, and
//! optionally return height of the ancestor.
virtual bool findAncestorByHash(const uint256& block_hash,
const uint256& ancestor_hash,
int* height = 0) = 0;

//! Find most recent common ancestor between two blocks and optionally
//! return its hash and/or height. Also return height of first block. Return

This comment has been minimized.

Copy link
@luke-jr

luke-jr Feb 19, 2020

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 19, 2020

Author Contributor

re: #17954 (comment)

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

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

@fjahr
fjahr approved these changes Feb 21, 2020
Copy link
Contributor

fjahr left a comment

Code review ACK 1125fd9 (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.

//! optionally return height of the ancestor.
virtual bool findAncestorByHash(const uint256& block_hash,
const uint256& ancestor_hash,
int* height = 0) = 0;

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

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.

Optional<int> height = locked_chain->getBlockHeight(merkleBlock.header.GetHash());
if (height == nullopt) {
LOCK(pwallet->cs_wallet);
int height;

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

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.

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

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

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

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

This comment has been minimized.

Copy link
@fjahr

fjahr Feb 21, 2020

Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.