Add wallet birthday height for seed recovery on pruned nodes#822
Add wallet birthday height for seed recovery on pruned nodes#822FreeOnlineUser wants to merge 2 commits into
Conversation
|
👋 I see @tnull was un-assigned. |
When set_wallet_birthday_height(height) is called, the BDK wallet checkpoint is set to the birthday block instead of the current chain tip. This allows the wallet to sync from the birthday forward, recovering historical UTXOs without scanning from genesis. This is critical for pruned nodes where blocks before the birthday are unavailable, making recovery_mode (which scans from genesis) unusable. Three-way logic: - Birthday set: checkpoint at birthday block - No birthday, no recovery mode: checkpoint at current tip (existing) - Recovery mode without birthday: sync from genesis (existing) Falls back to current tip if the birthday block hash cannot be fetched. Resolves the TODO: 'Use a proper wallet birthday once BDK supports it.' Closes lightningdevkit#818
Extend get_block_hash_by_height to work with Esplora and Electrum in addition to bitcoind. Esplora uses its native get_block_hash API. Electrum uses block_header_raw and extracts the hash from the header. For Electrum, if the runtime client hasn't started yet (called during build), a temporary connection is created for the lookup.
FreeOnlineUser
force-pushed
the
upstream/wallet-birthday-pr
branch
from
5f532f9 to
caa173d
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
Jolah1
left a comment
Jolah1
left a comment
There was a problem hiding this comment.
Nice Work taking this up!
Took a look at the changes, the feature is well-motivated and the real-hardware validation is great, but a few things I'd like to see addressed before this lands.
- The silent fallback to chain tip when the birthday hash fetch fails (src/builder.rs:1419-1444) silently nullifies the feature for the exact users it's meant to help. Detail inline.
- Result<_, ()> on ChainSource::get_block_hash_by_height is inconsistent with the rest of the file and drops error context that would help the call site above.
- Precedence between set_wallet_birthday_height and set_wallet_recovery_mode isn't documented.
- set_wallet_birthday_height isn't exposed in bindings/ldk_node.udl — and the motivating use case is mobile (Android via Kotlin FFI).
- CI is red on cargo fmt --check in four places.
Missing: there are no tests for the new setter, the build-time branching, or the new chain-source method. The branching has four distinct paths (birthday set / birthday + fetch failure / no birthday + tip / no birthday +
recovery) and at minimum a unit test that the setter updates the field would help. Integration coverage for the birthday-checkpoint path would be ideal given the existing regtest harness, but I won't block on that.
Also: branch will need a rebase onto current main once the above is sorted rebaseable: false on the API right now.
| Err(e) => { | ||
| log_error!( | ||
| logger, | ||
| "Failed to fetch block hash at birthday height {}: {:?}. \ | ||
| Falling back to current tip.", | ||
| birthday_height, | ||
| e | ||
| ); | ||
| // Fall back to current tip | ||
| if let Some(best_block) = chain_tip_opt { | ||
| let mut latest_checkpoint = wallet.latest_checkpoint(); | ||
| let block_id = bdk_chain::BlockId { | ||
| height: best_block.height, | ||
| hash: best_block.block_hash, | ||
| }; | ||
| latest_checkpoint = latest_checkpoint.insert(block_id); | ||
| let update = bdk_wallet::Update { | ||
| chain: Some(latest_checkpoint), | ||
| ..Default::default() | ||
| }; | ||
| wallet.apply_update(update).map_err(|e| { | ||
| log_error!(logger, "Failed to apply fallback checkpoint: {}", e); | ||
| BuildError::WalletSetupFailed | ||
| })?; | ||
| } | ||
| }, |
There was a problem hiding this comment.
If the user set a wallet birthday they almost certainly don't want to be silently checkpointed at chain tip on a transient hash-fetch failure, that defeats the whole point of the feature. A mobile recovery user would see 0 sats restored, assume their seed is wrong, and likely never look at the logs.
Two safer alternatives:
- Return BuildError::WalletSetupFailed so the caller can retry (or fall back at the application layer).
- Fall back to no checkpoint, i.e. the recovery_mode genesis-sync behavior, so the user still gets historical coverage at the cost of a full scan.
I'd lean toward option 1 (surface the error). Falling back to tip is the worst outcome for the user. WDYT?
| } | ||
|
|
||
| /// Fetches the block hash at the given height from the chain source. | ||
| pub(crate) async fn get_block_hash_by_height( |
There was a problem hiding this comment.
Every other method on ChainSource in this file returns Result<, Error> (e.g. start at L188, update_fee_rate_estimates at L446). Result<, ()> here drops useful context from all three backends: UtxoSource::get_block_hash_by_height returns BlockSourceResult, esplora_client.get_block_hash returns Result<, EsploraError>, and block_header_raw returns Result<, electrum_client::Error>. Surfacing those via crate::Error would be consistent and would let the call site reason about the failure mode (relevant to the comment on builder.rs:1419-1444).
| self | ||
| } | ||
|
|
||
| /// Sets the wallet birthday height for seed recovery on pruned nodes. |
There was a problem hiding this comment.
The current code makes birthday take precedence over set_wallet_recovery_mode (the if let Some(birthday_height) = … else if !recovery_mode { … } chain in build_with_store_internal). Worth a sentence here noting the precedence so callers who set both know what to expect.
"Use a conservative estimate" is direction-ambiguous. A height after the first tx would miss funds.
Maybe: "use a height earlier than your first transaction, lower is safer than higher."
| @@ -17,6 +17,7 @@ use bitcoin::{Script, Txid}; | |||
| use lightning::chain::{BestBlock, Filter}; | |||
There was a problem hiding this comment.
Heads up: cargo fmt --all -- --check fails on this branch in 4 spots (builder.rs:1181, builder.rs:1402, chain/mod.rs:17, chain/mod.rs:29), which is what's red on build (ubuntu-latest, stable) in CI. A cargo fmt --all pass + rebase should clear it.
3 participants
Adds
set_wallet_birthday_height(height)to the builder, allowing the on-chain wallet to sync from a specific block height instead of the current tip or genesis.This complements the existing
set_wallet_recovery_mode()from #769 by supporting pruned nodes where scanning from genesis fails due to missing blocks.Motivation
When restoring a wallet from seed on a pruned node:
recovery_mode(scan from genesis) fails: blocks are prunedWith a birthday height, the wallet checkpoints at a known block before the first transaction, recovering funds without needing the full chain.
How it works
Three-way logic in wallet creation:
Falls back to current tip if the birthday block hash cannot be fetched.
Real-world use
Built and verified in Bitcoin Pocket Node, an Android app running bitcoind + ldk-node on GrapheneOS with pruned storage. Recovery flow:
scantxoutsetto find UTXOs and their block heightsmin(heights) - 10Tested end-to-end on real hardware: 10,844 sats recovered in seconds.
Closes #818