Open j-berman opened 5 months ago
(obviously the state of the pool isn't fixed to a block hash, but since txs can't be both in the pool and the chain, then this should work)
Alternative that's probably simpler: scan pool first then chain. If any of the user's pool txs are identified in the chain, it can be inferred it's no longer in the pool. This is basically the same as wallet2 today.
Alternative that's probably simpler: scan pool first then chain. If any of the user's pool txs are identified in the chain, it can be inferred it's no longer in the pool. This is basically the same as wallet2 today.
This doesn't work for long scan jobs because the pool view will be quite stale by the end.
Seems acceptable because the pool just gets scanned on the next refresh loop
I'd rather the scanner user control the features it uses, than for the scanner itself to be weakened. Can you accomplish the desired optimization with a config, or by simply ignoring the initial or follow-up scan?
The primary function in question:
bool refresh_enote_store(const scanning::ScanMachineConfig &scan_machine_config,
scanning::ScanContextNonLedger &nonledger_scan_context_inout,
scanning::ScanContextLedger &ledger_scan_context_inout,
scanning::ChunkConsumer &chunk_consumer_inout)
{
// 1. perform a full scan
if (!refresh_enote_store_ledger(scan_machine_config, ledger_scan_context_inout, chunk_consumer_inout))
return false;
// 2. perform an unconfirmed scan
if (!refresh_enote_store_nonledger(SpEnoteOriginStatus::UNCONFIRMED,
SpEnoteSpentStatus::SPENT_UNCONFIRMED,
nonledger_scan_context_inout,
chunk_consumer_inout))
return false;
// 3. perform a follow-up full scan
// rationale:
// - blocks may have been added between the initial on-chain pass and the unconfirmed pass, and those blocks may
// contain txs not seen by the unconfirmed pass (i.e. sneaky txs)
// - we want scan results to be chronologically contiguous (it is better for the unconfirmed scan results to be stale
// than the on-chain scan results)
if (!refresh_enote_store_ledger(scan_machine_config, ledger_scan_context_inout, chunk_consumer_inout))
return false;
return true;
}
How I'm thinking about options also based on some discussions we've had in the past:
refresh_enote_store
is rewritten to do a 2-step refresh_enote_store_ledger
, then refresh_enote_store_nonledger
(or include a config option to skip the step 3 follow-up chain pass)/getblocks.bin
grabs a lock on the chain and pool for duration of serving the response to ensure the pool state corresponds to the top block hash returned by the endpointnonledger_scan_context_inout
and ledger_scan_context_inout
would both be children of a single parent classrefresh_enote_store_ledger
, the ledger_scan_context_inout
is able to fetch pool txs and will keep those pool txs cached in the parent classrefresh_enote_store_nonledger
, those pool txs will be read from the parent class cache in nonledger_scan_context_inout.get_nonledger_chunk
and consumed
begin_scanning_from_index
also wipes the parent's tx pool cachePros
refresh_enote_store
which completes a full scan of all state known to the daemon at a specific point in time.Cons
/getblocks.bin
. It adds more complexity and risk to the daemon-side changes, and will add to review time.refresh_enote_store_ledger
in refresh_enote_store
nonledger_scan_context_inout
and ledger_scan_context_inout
would again both be children of a single parent class.refresh_enote_store_nonledger
, the nonledger_scan_context_inout
fetches pool txs and blocks via /getblocks.bin
, caching blocks in the parent class.refresh_enote_store_ledger
, blocks are read from the parent class cache in ledger_scan_context_inout.get_onchain_chunk
and consumed. If there are more blocks to scan, it'll do so.Pros
Cons
Chunk chunk = scan_context.get_next_chunk()
if chunk == nonledger chunk
handle nonledger chunk
else if chunk == ledger chunk
handle ledger chunk (ensure contiguity to prior ledger chunk and consume)
else
must be the terminal chunk, reached scanner end state
Edited to add clearer terminal chunk handling.
Route 2 is probably simplest to implement imo
Ok after reviewing the seraphis_lib
implementation I remember why I wrote it like this. You don't need to use refresh_enote_store
. Notice that it's in the file scan_process_basic.h/.cpp
. The idea is you can write any high-level function you want to orchestrate scanning using the scan-machine API. So I'd just do option 2 with a new function.
Continuing from point 2 in this comment in the async scanner: https://github.com/UkoeHB/monero/pull/23#issuecomment-2036086371
Ideally wallets minimize round trips to the daemon when loading. This was the idea behind returning pool txs in the
/getblocks.bin
endpoint: https://github.com/monero-project/monero/pull/8076The Seraphis lib scanner currently does 1) scan chain -> 2) scan pool -> 3) scan chain in a scan pass, which would require a minimum of 3 round trips to the daemon. @UkoeHB mentioned:
Proposed solution to this problem: ensure that when the
/getblocks.bin
endpoint returns pool txs, those pool txs correspond to the state of the pool at thetop_block_hash
also returned by the endpoint (to be clear, this would be a change to the daemon RPC side). This way the scanner can be certain that once it has scanned up to chain tip, it knows the state of the pool at that time.