fix(bitcoincore): correct address derivation, paginate listtransactions, collision-free wallet names - #29
Merged
Conversation
… swallowing errors deriveaddresses was always called with a range argument, which Core rejects for un-ranged descriptors like addr(...). Both call sites silenced the failure with if-let-Ok, leaving derived_addresses empty and causing false negatives. Pass the range only for ranged descriptors and propagate derivation errors.
…10000 A single call with a fixed count of 10000 silently dropped older transactions in large wallets. Fetch pages of tx_page_size (default 1000, injectable via with_tx_page_size) with count+skip until a short page, reassembling pages oldest-first to keep the previous ordering.
Two concurrent scans starting in the same millisecond produced the
same _stealth_scan_{millis} wallet name, so one createwallet failed.
Extract name generation into scan_wallet_name(timestamp) and append a
global atomic counter: _stealth_scan_{millis}_{n}.
…llet The if-let-Ok silently produced empty derived_addresses on failure, the same false-negative class as the derive_addresses fix. Propagate unconditionally: legacy (non-descriptor) wallets are not a supported target since Core 30 removed legacy wallet loading entirely.
Confirm each funding tx in its own block so blockheights are distinct, then assert non-decreasing heights in the final list. Verified the assertion catches removal of the page reverse (mutation check).
count+skip pagination is not atomic: a block arriving between pages shifts the skip windows and can drop entries after a reorg. Snapshot getblockcount around the page loop and refetch once if the tip moved. No deterministic test seam exists for a mid-loop block; the ordering and completeness assertions in the pagination test cover reassembly.
The atomic counter is per-process, so CLI and API scans starting in
the same millisecond could still collide on the shared node. Name
format is now _stealth_scan_{millis}_{pid}_{n}.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness fixes in the Bitcoin Core gateway, found by a code quality audit and hardened after an independent review pass.
deriveaddresses was called with a range parameter for every descriptor, which Core rejects for un-ranged ones like addr(...). Worse, both call sites swallowed the error, so derived_addresses came back empty and is_ours() silently missed the wallet's own addresses. The range is now only sent for ranged descriptors and errors propagate. Same treatment for list_wallet_descriptors in scan_wallet, which had the same swallowing pattern.
listtransactions used a fixed 10000 cap, silently truncating busy wallets. It now paginates with count and skip until a short page, preserving the oldest-first ordering across pages (the integration test asserts non-decreasing block heights and was mutation-checked: removing the reordering makes it fail). If the chain tip moves mid-pagination the pages are refetched once.
Temporary scan wallet names were derived from the timestamp alone, so concurrent scans in the same millisecond collided. Names now include the process id and an atomic counter.
Tests: new integration module (engine/tests/integration/gateway.rs) covering the addr() derivation and pagination end to end on regtest, plus a unit test for name uniqueness. Full workspace suite passes 41/41 with clippy -D warnings clean. All work was test-first; each fix has recorded red-then-green evidence.