Repository navigation
feat(rpc): scan ranged and multipath public descriptors - #1676
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR extends scantxoutset to accept public wildcard and multipath descriptors for pkh, wpkh, sh(wpkh), and key-only tr forms. The overall design is well-structured: a two-phase approach (work budget check before any derivation) is correct and the scan-cancel integration is thorough.
Critical Finding
One logic bug was identified in window_tx_count_between at line 485 of crates/rpc/src/handlers/chain.rs — the second call to log_prefix_count passes end_id instead of start_id as the branch_tip argument. This means the start count's prefix validation walks the end node's ancestors instead of the start node's ancestors, silently dropping window_tx_count to None (field omitted from response) for any historical getchaintxstats window that falls back to the log path. Note: this line is not in the PR diff — it is a pre-existing bug, but the PR's new code in window_stats continues to call window_tx_count_between for historical selections introduced by this change's range-handling path, so it is directly relevant.
No Other Blocking Issues Found
- The two-phase descriptor budget check (count before derive) correctly prevents hardened-path descriptors from consuming CPU before being rejected.
- The early-return on cancellation in
parse_scan_scripts(replacing the oldbreak) is correct. - Network validation and duplicate-path rejection for multipath specifiers are properly enforced.
- The
scan_descriptorfixed-descriptor range handling (silently ignoring a caller-supplied range for non-wildcards) matches the documented behavior in the PR description. - The
ScanDescriptor::script_count()arithmetic usessaturating_multo prevent overflow on the budget check.
The pre-existing window_tx_count_between bug should be fixed before or alongside this merge to avoid silently incorrect transaction counts for historical scan windows.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Follow-up to Amazon Q's review: the suggested
I reran |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Turn on auto-fix | Re-trigger cubic
- BIP 380/389 merged-scope rows stay Implemented; the scantxoutset descriptor-scan portions move to new Planned rows that link this PR, per the bip-adoption policy that open-PR capabilities must not be marked Implemented. - external-api.md: cite the scantxoutset unit tests and e2e cases as the API-35 evidence alongside the CLI proof. - Update ctutils to 0.4.6 for RUSTSEC-2026-0385. (fmt and cargo deny verified; test lanes skipped — docs and lockfile only.)
|
Pushed
|
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…-descriptors # Conflicts: # Cargo.lock # fuzz/Cargo.lock
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
gosunuts
left a comment
gosunuts
left a comment
There was a problem hiding this comment.
Reviewed adaac15. No actionable defect found in the changed descriptor scan path. Aggregate expansion work is admitted before key derivation and before deduplication; cancellation and reservation cleanup remain intact, as do the existing coherent UTXO/applied-tip read boundary and first-descriptor attribution. On WSL/Linux with Rust 1.99.0, all 15 scantxoutset unit tests passed. Built the daemon from this exact checkout with fjall and ran the scantxoutset process suite against the pinned Core 31.1: all three tests passed. The documented 1,024-expansion limit and supported-descriptor deviations remain; this is not full Core parity or mainnet-performance qualification.
Main brings #1684 opt-in Tor/I2P overlay, #1685 public-listener PeerRemote design, plus #1676 descriptor scan and #1677 compact-scan bound. Branch brings state-payload plumbing (BlockPayload wire, BodyInterest admission, index bundle validation). Resolutions: - crates/node/src/config.rs: keep both validations; branch validate_index_bundle plus main p2p.overlay.validate. - crates/p2p/src/listener.rs send_block: main PeerRemote parameter on branch BlockPayload body; single-conversion entry. - crates/p2p/src/listener.rs handle_compact_outcome: keep branch Option follow-up return (single enqueue_owned_body_fetch publisher per branch 5dee503) with main PeerRemote parameter. Main header-carried body_fetch_owned mark not restored: branch sync/headers.rs and inbound.rs intentionally supersede it with one atomic enqueue; restoring would reintroduce the racy double-mark 5dee503 deleted. - aggregate_budget_tests.rs: main PeerEndpoint address with branch is_none assertion matching the Option return. - crates/rpc/tests/core_compat.rs: main ipv4/ipv6/onion/i2p networks assertion (branch had dropped the stale len-3 check).
Merges origin/main fa187f9 (#1676 ranged/multipath descriptor scan, #1677 compact-block scan near applied tip) into the mining-pool stack. Prior main-merge resolution (462402d) preserved verbatim: PeerRemote listener identity, branch BlockPayload/Option ownership contract, both config validators, PeerEndpoint budget-test addr, 4-network compat pins, AddrMan canonical v1, mempool union. This merge auto-resolved with no conflicts; #1676 RPC scan and #1677 sync-scope changes apply cleanly onto the resolved tree.
Merge origin/main at fa187f9 (scan ranged and multipath public descriptors #1676; compact-block scan scoping #1677) into the index-snapshot lane. Clean automerge; prior main-merge resolution (2609ba4: overlay validators, PeerRemote/BlockPayload listener hybrid, PeerEndpoint tests, four-network RPC pin) preserved untouched.
Second main merge (fa187f9: #1676 ranged/multipath descriptor scan, #1677 compact-blocks-near-tip). Prior merge resolution (a74ef07) preserved untouched: union config validators, main PeerRemote addressing with payload/Option listener lineage, main compat assertions, branch-canonical AddrMan v1. This merge auto-resolved with no conflicts: scantxoutset handler additions, sync compact-near-tip gating, registry/docs updates.
Merge origin/main (46de452, #1684 Tor/I2P overlay + #1676/#1677 scans) into the Lightning lane. Listener (crates/p2p/src/listener.rs, aggregate_budget_tests.rs): main's #1685 addressing wins — PeerRemote/PeerEndpoint throughout, no ListenerRole or rpc-public-bind restored. The compact-outcome path keeps the branch's enqueue-owned-body-fetch design (Option return, single linearizable enqueue site) because the merged tree carries the branch's 3-arg send_headers/InboundHeaders and enqueue-only BlockSync API; main's bool/record variant has no matching API left post-merge. Tests updated to PeerEndpoint with the is_none assertion. All other auto-merged regions keep both intents: branch Lightning/ZMQ/admission work plus main's overlay, scan-bound, and RPC compat changes.
Merge origin/main (fa187f9: #1676 ranged/multipath descriptor scans, #1677 compact scan bound) into the Lightning lane. Auto-merged with no conflicts. Keeps both intents: main's #1677 COMPACT_RECONSTRUCTION_MAX_AHEAD scan bound in BlockSync alongside the branch-owned enqueue-owned-body-fetch protocol; main's PeerRemote/PeerEndpoint addressing untouched. Branch Lightning/ZMQ/admission work plus main's scantxoutset RPC surface (handlers/chain, handlers/util, registry, e2e) combined.
Follow-up: merge origin/main fa187f9 (#1676 scan ranged and multipath public descriptors; #1677 compact blocks only near the applied tip) into 5491964. No conflicts; 12 files auto-merged (Cargo.lock, crates/p2p/src/sync.rs + head_sync tests, rpc chain/util handlers + registry, e2e scantxoutset, docs, fuzz/Cargo.lock). The listener ruling from 5491964 stands (main PeerRemote/addressing design; branch-owned BlockPayload/enqueue protocol): neither listener file is touched by this merge.
Merges origin/main (fa187f9: #1676 ranged/multipath scantxoutset scans plus #1677 compact-scan bound near the applied tip) into the 03-rpc lane (observer interest, ZMQ body interest, retained RPC allocations, submitpackage). scantxoutset conflicts (chain.rs, util.rs, registry.rs, bip-support.md, external-api.md, rpc-reference.md): branch and main convergently implemented bounded wildcard/multipath expansion, so main's #1676 two-phase implementation wins (count all expansions including duplicates before any public-key derivation, then expand with dedup and the 1 MiB script cap). Adopted main's ScanDescriptor/scan_descriptor/expand/scan_key_script, parse flow, error strings, registry wording, API-35 docs, and proof-test references, all verified present in the merged tree. Deleted the branch 2-arg scan_descriptor shim (name collision with main's 3-arg form) and retargeted its one retained caller plus the branch fixed-form test to scan_descriptor_range, preserving their assertions. rpc-reference.md splits the hunk: main's scantxoutset row with the branch submitpackage rows, which main did not touch and which remain implemented (registry plus handlers/tx/submit_package.rs).
Closes #1600.
scantxoutsetpreviously rejected public wildcard and multipath descriptors even thoughderiveaddressescould expand them. It now accepts rangedpkh,wpkh,sh(wpkh)and key-onlytrinputs and feeds their scripts into the existing authoritative UTXO scan.Behavior and limits
[begin,end]ranges are inclusive. Omitted/null ranges on wildcards use[0,1000]per path. Fixed descriptors still validate and ignore a supplied range.-5with native diagnostic text. API-35, the registry and generated RPC reference retainDeviation; this is not whole-method Core parity.Synchronization and ownership
This changes read-only RPC input preparation, not an authoritative sync, validation or commit path. Descriptor parsing and derivation finish before the existing chain-transition exclusion is acquired. There is no new scan/index owner, persistent state or storage-format change.
The relevant obligations are
SYNC-03(bounded optional RPC work) andSYNC-05(coherent committed consumer views). The smallest affected proofs are the scan budget/cancellation tests,scantxoutset_waits_for_one_consistent_utxo_and_tip_transition,scan_projection_uses_retained_branch_after_tip_changes, and the existing real-HTTPscantxoutset_lifecycletest. Broader convergence and foreground-performance claims are outside this change.Validation
Linux x86_64, Rust 1.99.0, source
d200d03e07478dd6d0279fe837f418c5c50308e9based on8fb1f30b23fe6731f7e1e26322f4ba33154ef874:Commands and artifact identities
Cargo commands used
--lockedand, after dependency provisioning,--offline. Build settings:CARGO_BUILD_JOBS=4,CARGO_PROFILE_DEV_DEBUG=0,CARGO_PROFILE_TEST_DEBUG=0; isolatedCARGO_TARGET_DIR=/mnt/code/git/bitcoin-rs/target/issue-1600.986e63b3c8770f08d0059820ad3dd085d1ab9e1bea23946c243f858a06888a08(repository-pinned Core 31.1).30c89dfe2278c75d828859735e9b1d866235cef3839ab5a92533c6e99b3de817.c1ad7e974ece71b19e0a3c63b543576a5f94fee967051950578eb40843a842a3.target/process-harness/e2e/.Validation is scoped to Linux/regtest. Full-workspace, other-platform and mainnet-performance campaigns were not run locally.
The PR appears safe to merge; no actionable defects were found.Summary
Adds bounded public wildcard and multipath support to
scantxoutset.scantxoutsetscans bounded wildcard and multipath public descriptors.ctutilsto version 0.4.6.No actionable issues found. The full PR diff was reviewed because the previous and current heads have different merge bases with the target commit. No previous review threads were supplied. Tests were not run during this review.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Reserve scan] --> B[Parse descriptors and ranges] B --> C{Total expansions at most 1024?} C -->|No| D[Return error and release reservation] C -->|Yes| E[Expand with cancellation checks] E --> F[Keep distinct scripts and first attribution] F --> G[Scan under chain-transition exclusion] G --> H[Build response from retained tip] H --> I[Release reservation]Reviews (5) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile