Repository navigation
Gate backend-specific full_scan options - #367
emma31-dev wants to merge 1 commit into
Conversation
2fc5eb8 to
922dd64
Compare
emma31-dev
changed the title
|
I tested this locally against #340 (regtest, Core v32.1.0, funds only at external index 30, RPC wallet):
Rejecting the flag in RPC-only builds is a nice improvement over silently ignoring it. In a multi-backend build, though, the flag is still accepted and ignored when the wallet uses RPC, because the gate is compile-time while the backend is picked at runtime from the wallet config. A runtime check on the client type would cover that case. A few smaller things:
Full reproduction details are in my comment on #340. |
|
Thanks for bringing this up I'll make the change soon |
14a82b5 to
f2c493e
Compare
Olorunshogo
left a comment
Olorunshogo
left a comment
There was a problem hiding this comment.
Tested b929aa1 against master (regtest, Core v32.1.0, Esplora electrs, built with rpc,esplora,electrum).
The runtime check works: an RPC wallet now rejects --scan-stop-gap and --parallel-request with a clear error instead of silently ignoring them, and Electrum rejects --parallel-request. Esplora and Electrum scans behave the same as on master, including --scan-stop-gap 40 recovering funds at index 30. Clippy is clean with rpc,esplora,electrum and with rpc only.
Left two inline comments: the STOP_GAP env var now breaks RPC scans, and the default of 20 no longer shows in --help.
Smaller: the PR description still says the flag may be available for RPC wallets in multi-backend builds, which the runtime check now prevents, and the branch has a merge commit.
Happy to ACK once the two inline points are addressed.
| #[clap(long, default_value = "5")] | ||
| pub parallel_request: usize, | ||
| #[cfg(any(feature = "electrum", feature = "esplora"))] | ||
| #[arg(env = "STOP_GAP", long = "scan-stop-gap")] |
There was a problem hiding this comment.
Since this still reads STOP_GAP from the environment, an RPC full_scan fails when STOP_GAP is set, even if --scan-stop-gap isn't passed. I reproduced it: with STOP_GAP=50 exported, an RPC wallet errors with "--scan-stop-gap is only supported by the electrum and esplora backends", while the same scan works on master.
There was a problem hiding this comment.
I don't think I can gate the env parsing. What do you suggest I do here?
There was a problem hiding this comment.
Maybe I'll include it in the error message
| client.populate_tx_cache(wallet.tx_graph().full_txs().map(|tx_node| tx_node.tx)); | ||
| let update = client.full_scan(request, self.stop_gap, *batch_size, false)?; | ||
| let update = | ||
| client.full_scan(request, self.stop_gap.unwrap_or(20), *batch_size, false)?; |
There was a problem hiding this comment.
With default_value = "20" gone, full_scan --help no longer shows [default: 20], and 20 is now repeated here and in the Esplora branch. A shared constant like DEFAULT_STOP_GAP, mentioned in the doc comment, would keep the default visible and in one place.
There was a problem hiding this comment.
I was thinking of this yesterday thanks for bringing this up
|
@Olorunshogo Thanks for the review. I'll fix the minor issues. |
797c619 to
0e2f54b
Compare
|
@vadim-anfv I would appreciate a review on this when you have the time. |
The RPC and CBF backends never use `stop_gap`, so accepting the flag silently ignored it. Gate the option to the Electrum and Esplora backends and reject it at runtime for RPC and CBF wallets in multi-backend builds, where the backend is chosen from the wallet config rather than at compile time. Read `STOP_GAP` in the handler rather than through clap so that a globally exported value no longer turns an RPC or CBF scan into an error when the flag was never passed, while supported backends keep the environment behaviour. Keep the default of 20 in one place (`DEFAULT_STOP_GAP`) and document it, with the env var, in `--help`. Related to bitcoindevkit#340.
0e2f54b to
b6e54bf
Compare
Description
Make
full_scanoptions reflect backend support: gate--scan-stop-gapto Electrum/Esplora builds and make Esplora's--parallel-requestan optional override of the wallet-configured request count. Related to #340 .Notes to the reviewers
The RPC backend still scans blocks through
Emitterand does not usestop_gapas a lookahead. This change removes the option from RPC-only builds but does not make RPC discover funds beyond its current lookahead. In builds with multiple backend features enabled, the compile-time flag may still be available when the wallet is configured for RPC.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: