Skip to content

feat(wallet): complete derived-account lifecycle parity#92

Open
ben-kaufman wants to merge 5 commits into
mainfrom
codex/derived-account-lifecycle-parity
Open

feat(wallet): complete derived-account lifecycle parity#92
ben-kaufman wants to merge 5 commits into
mainfrom
codex/derived-account-lifecycle-parity

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Jul 14, 2026

Copy link
Copy Markdown

Summary

  • Load derived on-chain accounts from Config::onchain_wallet_accounts after validating each account index and xpub against the node seed.
  • Add runtime account removal that retains account-specific BDK state and safely ignores sync results that arrive after unload.
  • Add account-specific address metadata lookup by index and contiguous range.
  • Make Electrum full-scan batch size and stop gap configurable for non-primary wallets, with Bitkit-compatible values of 100 and 1000.
  • Bump the Synonym fork to 0.7.0-rc.55 and regenerate Swift, Android Kotlin, JVM Kotlin, Python, and native binding artifacts.

Behavior

Runtime add/remove changes are not persisted as registration state. Applications can persist { address_type, account_index, xpub } in their own configuration and supply it through Config::onchain_wallet_accounts on every build. Removing an account only unloads it from the aggregate wallet; re-adding it recovers the existing account-specific BDK namespace.

Account 0 remains reserved for the configured primary and monitored address-type wallets. Derived accounts cannot become the receive/change wallet.

ElectrumSyncConfig::additional_wallet_full_scan_batch_size and additional_wallet_full_scan_stop_gap apply only to monitored/derived wallets during full scans. Defaults remain 5 and 20; primary-wallet full scans and all incremental syncs retain their existing behavior.

Esplora and Electrum can recover confirmed history for newly loaded accounts through full scans. Bitcoind still starts brand-new accounts at the known tip and cannot recover confirmed pre-registration history.

Verification

  • cargo fmt --check
  • git diff --check origin/main...HEAD
  • cargo test --lib (72 passed)
  • cargo test --lib --features uniffi (82 passed)
  • cargo check --all-targets --all-features
  • cargo test --doc (12 passed)
  • Config-load/runtime-remove integration test in native and UniFFI modes
  • Account index/range lookup integration test in native and UniFFI modes
  • Configured-account Electrum restart recovery from externally funded index 30 with batch 100 and stop gap 1000, in native and UniFFI modes
  • Full ./bindgen.sh generation and platform builds
  • Swift archive checksum verified against Package.swift: 098d48d602fe000f1a9dbc7e758a0fe7bd8720c12b0036c38edf38283edd55f3

Strict cargo clippy --lib -- -D warnings remains blocked by pre-existing lint failures on origin/main; this PR does not add unrelated lint cleanup.

Release

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58260d37e5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

Comment thread src/builder.rs
@coreyphillips

coreyphillips commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

The following cropped up upon AI review:

1 (incremental sync misses later external addresses) - confirmed. get_incremental_sync_request uses start_sync_with_revealed_spks(), so after the initial full scan a running session only covers revealed scripts. A full scan does re-run on each process restart (synced_derived_accounts is in-memory) and the additional-wallet stop gap of up to 1000 bounds the exposure, so this is delayed discovery within a long-running session rather than strict funds loss, with reveal_receive_addresses_to_account as the manual workaround. Still a real gap. #93's rolling lookahead is the right fix, so I'd ship #92 and #93 together rather than release #92 alone.

2 (large full scan under fixed timeout) - confirmed. get_full_scan_wallet_update wraps both primary and additional full scans in timeout(BDK_WALLET_SYNC_TIMEOUT_SECS) with no scaling, while #93 scales the timeout only for its larger incremental request. A stop gap of 1000 against a slow remote Electrum can time out. Worth testing against a latent public server and scaling the full-scan timeout the same way #93 does for incremental.

3 (error inconsistency) - confirmed. get_address_info_for_account_at_index and get_address_infos_for_account map the aggregate WalletNotFound to WalletOperationFailed, so an unloaded account is indistinguishable from a real failure. Only remove_onchain_wallet_account surfaces OnchainWalletAccountNotRegistered. Mapping WalletNotFound consistently across the account APIs would let FFI clients tell the two apart. Cheap cleanup worth folding in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants