Closed j-berman closed 4 months ago
Nice to see this coming along :)
Probably worth holding off on a deeper review until it's ready, almost there :)
Ready for review :)
Noting there are 2 significant aspects of this scanner that will need to be dealt with. I think it'll be easier to handle in follow-up PR's.
1. The subaddress lookahead.
EnoteFindingContextMockLegacy
takes as input a constant legacy_subaddress_map
to see enotes received to subaddresses.
LegacyBasicEnoteRecord
could instead be view tag matched enotes and their nominal_spendkey
, rather than known-owned view scanned enotes.
LegacyBasicEnoteRecord
's serially in process_chunk_full_legacy
and identifies owned enotes, it then expands the suabddress lookahead as needed.2. Pool handling.
/getblocks.bin
) in order to reduce round trips to the daemon.AsyncScanContextLegacy
could cache pool txs and then that cache can be reused for get_nonledger_chunk
.To reviewers: I suggest going commit-by-commit and working up to the final async scanner.
10/13 of the commits are theoretically independent and can be PR'd separately.
If the design generally looks solid, I can divide it up into smaller PR's to ease review. Additionally we can PR the daemon RPC changes to monerod at any time, so that daemons would be prepared for this scanner well in advance of releasing the Seraphis lib in a functional wallet to end-users.
I think it would make sense to re-structure the Seraphis lib to simply do a single "scanning pass" (scan chain then pool).
I intentionally added a follow-up pass because there is a race condition between chain scanning and pool scanning that can cause mined blocks to contain txs invisible to a wallet even after it claims to have scanned the pool.
Opened a separate issue to discuss since I don't think it needs to affect this PR: https://github.com/UkoeHB/monero/issues/41
Once this is merged, please open as many PRs to monero core as possible to reduce the file overlap between this branch and core.
Submitted PR's for the simple modifications to the core monero repo for the async scanner. If daemons run those changes, they'll be fully compatible with the async scanner in this PR.
Ready for review.
I've observed the following speed-ups benchmarking versus wallet2:
Benchmark results can vary widely based on setup (client/node internet speeds and machines). Source for the scanner used to benchmark.
How it works
Assume the user's wallet must start scanning blocks from height 5000.
max_block_count
blocks, and the chunk is not the tip of the chain, this means there was a "gap" in the chunk request. The scanner launches another parallel RPC request to fill in the gap./getblocks.bin
RPC endpoint.(req.start_height + res.blocks.size())
to(req.start_height + req.max_block_count)
.Some technical highlights
std::function<bool(const cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::request, cryptonote::COMMAND_RPC_GET_BLOCKS_FAST::response)>
.Major TODO's for this PR
Cleanup the formatting, code structure, code names and commit history.Implement backwards compatibility so the scanner will work pointing to nodes running today.Add unit tests.Try to smooth out the scanner's progress.The scanner completes scanning many chunks in intermittent waves and then sits waiting (uncomfortably) until the next wave of chunks; it doesn't scan in a steady stream.max_chunk_size_hint
from 1000 to 20 to fetch chunks 20 blocks at a time. Overall time to scan remains the same (potentially slightly faster) and feedback to the user on scanner progress is significantly smoother.Other TODO's for this PR
Move "mock" implementations to the "production-ready" sections they belong.Complete TODO's in the code.As soon as the scanner encounters an error or encounters the terminal chunk, it should immediately cancel any pending tasks it's waiting on and error out quickly.Remove the blockchain scanner utility from this PR (and all changes to existing wallet functions for this utility). It's there to help see how the scanner works.Misc.
There are a few things from this PR I can PR to the Monero repo today in bite-sized PR's:
wallet2
and intowallet/wallet_errors.h
. I did this so I could reuse wallet2's error logic in this PR, which the higher level wallet API also expects.get_blocks.bin
daemon RPC endpoint:max_block_count
param to the request (not a breaking change)start_height
that is greater than the chain tip and the request fieldfail_on_high_height
is false, then instead of erroring, the server returns empty blocks and the chain height.TODO: make this field optional in the response so it's not a breaking change.(DONE)