-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: improve wallet recovery and scanning handling of reorgs #3655
Conversation
There was a problem hiding this 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() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if requested_epoch_time <= mid_header.timestamp.as_u64() { | |
} | |
if requested_epoch_time <= mid_header.timestamp.as_u64() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy begs to differ
There was a problem hiding this 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:
- 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)
- 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.
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. |
ef3a112
to
4dad4c5
Compare
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.
e2a95df
to
78b0fdd
Compare
* 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)
* 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)
* 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)
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 newDevelopment
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:
sync_utxos_by_block
method to theBaseNodeWalletService
RPC service to facilitate a more appropriate UTXO sync strategy for the walletScannedBlocks
table to the wallet DB to keep a history of scanned block headers and the number and amount of outputs recovered from each block.How Has This Been Tested?
Added new Rust integration tests
Added back excluded Cucumber tests