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

feat: improve wallet recovery and scanning handling of reorgs #3655

Merged

Conversation

philipr-za
Copy link
Contributor

Description

@mikethetike Merging this into the weatherwax branch because I would like to see it tested on the Mobile apps sooner rather than later and not sure what the plan is for migrating them to the new Development

This PR updates the wallet UtxoScanner to better detect and handle reorgs. Previously only the last scanned block was check to see if it had been re-orged out and if it had then the whole chain was scanned from scratch. Reorgs of a few blocks are common so to reduce the amount of work done after a reorg this PR adds maintaining history of previously scanned block headers so that the depth of the reorg can be quickly determined and the rescan only be done for affected blocks.

This PR adds a number of updates:

  • Add the sync_utxos_by_block method to the BaseNodeWalletService RPC service to facilitate a more appropriate UTXO sync strategy for the wallet
  • Add the ScannedBlocks table to the wallet DB to keep a history of scanned block headers and the number and amount of outputs recovered from each block.
  • Update the UTXO scanner to use this stored history to detect reorgs and only sync the required UTXOs
  • Built out a test harness to test the scanner thoroughly in Rust integration tests.

How Has This Been Tested?

Added new Rust integration tests
Added back excluded Cucumber tests

SWvheerden
SWvheerden previously approved these changes Dec 13, 2021
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return Ok(Response::new(before_mid_header.height));
} else if mid_height == right_height {
return Ok(Response::new(right_height));
} else if requested_epoch_time <= mid_header.timestamp.as_u64() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if requested_epoch_time <= mid_header.timestamp.as_u64() {
}
if requested_epoch_time <= mid_header.timestamp.as_u64() {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be made into a separate if. It does not have to be a single chained if-else statement.
We can even split it up into three if's but two is probably better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clippy begs to differ

sdbondi
sdbondi previously approved these changes Dec 16, 2021
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Good idea to add the call to the base node RPC service so that we connect to a single rpc service in the wallet. There were some bugs fixed in #3663 that were causing a problem with the indexes if include_pruned_utxos == false, or start index != a block boundary so index based syncing works fine after the fix (apologies, I had only tested with pruned sync and empty wallet which are not effected by the bugs). Though I like the simplification of the handler by using block boundary indexes in the wallet.

The batching of many utxos in a single message, with large blocks, is anecdotally worse (unless there is sensible evidence to the contrary, we must measure to be sure we understand this) because:

  1. the longer it takes for a whole message to arrive (say because of its size) the longer the client side has to wait before beginning processing (a note on this, I suspect buffering can be easily optimised in the RPC client implementation for long streams so that any chunking on the client side is unnecessary/slower)
  2. large responses are sent as chunk internally as necessary, so this diminishes any per-message RPC header byte saving (RPC header is almost always 5-10 bytes)

In this case, performance will be similar for small blocks and much worse for large blocks as observed in block sync. There is probably a happy middle ground for message size vs number of messages (block boundary indexes cannot be used). Though I would prefer the use of the same (bug free) code for both handlers, I think this can go in and we can make some observations.

@philipr-za
Copy link
Contributor Author

The batching of many utxos in a single message, with large blocks, is anecdotally worse (unless there is sensible evidence to the contrary, we must measure to be sure we understand this) because:

Yes, I did consider this could be an issue. I went with the streaming by block boundary because it is a much simpler conceptual model but it wouldn't be hard to split a large block into multiple smaller paged messages. However, I decided it wasn't worth the effort at this stage because the pending on the filter byte will pretty much solve this potential issue I think. So I decided it will be something to look at once the filter byte has been implemented.

@philipr-za philipr-za dismissed stale reviews from sdbondi and SWvheerden via 51c6168 December 21, 2021 06:56
@philipr-za philipr-za force-pushed the philip-utxo-scanning branch 2 times, most recently from ef3a112 to 4dad4c5 Compare December 21, 2021 07:49
This PR updates the wallet UtxoScanner to better detect and handle reorgs. Previously only the last scanned block was check to see if it had been re-orged out and if it had then the whole chain was scanned from scratch. Reorgs of a few blocks are common so to reduce the amount of work done after a reorg this PR adds maintaining history of previously scanned block headers so that the depth of the reorg can be quickly determined and the rescan only be done for affected blocks.

This PR adds a number of updates:
- Add the `sync_utxos_by_block` method to the `BaseNodeWalletService` RPC service to facilitate a more appropriate UTXO sync strategy for the wallet
- Add the `ScannedBlocks` table to the wallet DB to keep a history of scanned block headers and the number and amount of outputs recovered from each block.
- Update the UTXO scanner to use this stored history to detect reorgs and only sync the required UTXOs
- Built out a test harness to test the scanner thoroughly in Rust integration tests.
@aviator-app aviator-app bot merged commit fe9033b into tari-project:weatherwax Dec 21, 2021
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 3, 2022
* weatherwax:
  feat: add search by commitment to explorer (tari-project#3668)
  feat: tari launchpad (tari-project#3671)
  feat: base_node switching for console_wallet when status is offline (tari-project#3639)
  feat: improve wallet recovery and scanning handling of reorgs (tari-project#3655)
  feat: add GRPC call to search for utxo via commitment hex (tari-project#3666)
  feat: custom_base_node in config (tari-project#3651)
  fix: return correct index for include_pruned_utxos = false (tari-project#3663)
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 7, 2022
* development:
  chore: remove moving lock.mdb (tari-project#3674)
  chore: merge weatherwax
  feat!: provide a compact form of TransactionInput (tari-project#3460)
  v0.22.1.1
  v0.22.1
  ci: add build step (tari-project#3678)
  fix: edge cases causing bans during header/block sync (tari-project#3661)
  fix: end stale outbound queue immediately on disconnect, retry outbound messages (tari-project#3664)
  feat: add search by commitment to explorer (tari-project#3668)
  feat: tari launchpad (tari-project#3671)
  feat: base_node switching for console_wallet when status is offline (tari-project#3639)
  feat: improve wallet recovery and scanning handling of reorgs (tari-project#3655)
  feat: add GRPC call to search for utxo via commitment hex (tari-project#3666)
  feat: custom_base_node in config (tari-project#3651)
  fix: return correct index for include_pruned_utxos = false (tari-project#3663)
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 10, 2022
* development:
  feat: dibbler new genesis block with faucet utxos (tari-project#3688)
  ci: fix clippy warning on generated proto module (tari-project#3690)
  test: fix metadata signature cucumber (tari-project#3687)
  refactor!: clean up #testnet reset TODOs (tari-project#3682)
  feat(comms)!: add signature to peer identity to allow third party identity updates (tari-project#3629)
  chore: remove moving lock.mdb (tari-project#3674)
  chore: merge weatherwax
  v0.22.1.1
  v0.22.1
  ci: add build step (tari-project#3678)
  fix: edge cases causing bans during header/block sync (tari-project#3661)
  fix: end stale outbound queue immediately on disconnect, retry outbound messages (tari-project#3664)
  feat: add search by commitment to explorer (tari-project#3668)
  feat: tari launchpad (tari-project#3671)
  feat: base_node switching for console_wallet when status is offline (tari-project#3639)
  feat: improve wallet recovery and scanning handling of reorgs (tari-project#3655)
  feat: add GRPC call to search for utxo via commitment hex (tari-project#3666)
  feat: custom_base_node in config (tari-project#3651)
  fix: return correct index for include_pruned_utxos = false (tari-project#3663)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants