Skip to content

Add wallet birthday height for seed recovery on pruned nodes#822

Open
FreeOnlineUser wants to merge 3 commits into
lightningdevkit:mainfrom
FreeOnlineUser:upstream/wallet-birthday-pr
Open

Add wallet birthday height for seed recovery on pruned nodes#822
FreeOnlineUser wants to merge 3 commits into
lightningdevkit:mainfrom
FreeOnlineUser:upstream/wallet-birthday-pr

Conversation

@FreeOnlineUser
Copy link
Copy Markdown

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 pruned
  • Default behavior (checkpoint at tip) misses historical funds

With 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:

  1. Birthday set: fetch block hash at birthday height, checkpoint there
  2. No birthday, no recovery mode: checkpoint at current tip (existing behavior)
  3. Recovery mode without birthday: sync from genesis (existing behavior)

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:

  1. User restores seed
  2. App runs scantxoutset to find UTXOs and their block heights
  3. Sets birthday to min(heights) - 10
  4. ldk-node syncs from that height, balance appears

Tested end-to-end on real hardware: 10,844 sats recovered in seconds.

Closes #818

@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Mar 10, 2026

👋 I see @tnull was un-assigned.
If you'd like another reviewer assignment, please click here.

@FreeOnlineUser FreeOnlineUser marked this pull request as ready for review March 18, 2026 13:10
@ldk-reviews-bot ldk-reviews-bot requested a review from tnull March 18, 2026 13:11
@FreeOnlineUser FreeOnlineUser force-pushed the upstream/wallet-birthday-pr branch from 5f532f9 to caa173d Compare March 19, 2026 08:17
@ldk-reviews-bot
Copy link
Copy Markdown

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 2nd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link
Copy Markdown

🔔 3rd Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Copy Markdown
Contributor

@Jolah1 Jolah1 left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/builder.rs
Comment on lines +1419 to +1444
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
})?;
}
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. Return BuildError::WalletSetupFailed so the caller can retry (or fall back at the application layer).
  2. 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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Now returns BuildError::WalletSetupFailed when a birthday is set but the block hash can't be fetched, rather than silently checkpointing at the tip.

Comment thread src/chain/mod.rs
}

/// Fetches the block hash at the given height from the chain source.
pub(crate) async fn get_block_hash_by_height(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done. Returns Error::ChainAccessFailed now, consistent with the other ChainSource methods, and mirrored into the UDL error enum.

Comment thread src/builder.rs
self
}

/// Sets the wallet birthday height for seed recovery on pruned nodes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Documented: the birthday height takes precedence over recovery mode.

Comment thread src/chain/mod.rs Outdated
@@ -17,6 +17,7 @@ use bitcoin::{Script, Txid};
use lightning::chain::{BestBlock, Filter};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed with a cargo fmt --all pass, and regrouped the lightning_block_sync import while here.

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.
Addresses review on lightningdevkit#822:

- Fail with BuildError::WalletSetupFailed when the birthday block hash
  can't be fetched, instead of silently checkpointing at the chain tip.
  Silent fallback would scan from tip and recover nothing, leaving the
  user to assume their seed is wrong.
- Return a typed Error::ChainAccessFailed from
  ChainSource::get_block_hash_by_height instead of Result<_, ()>, and
  mirror the variant in the UDL Error enum.
- Expose set_wallet_birthday_height in the UniFFI bindings.
- Document that the birthday height takes precedence over recovery mode.
- Add an integration test (onchain_wallet_recovery_with_birthday) that
  recovers funds from a wallet birthday over random_chain_source, plus
  the TestConfig plumbing for it.
- Run cargo fmt and group the lightning_block_sync import with the
  external crates.

Co-Authored-By: Joe (Claude Opus 4.8)
@FreeOnlineUser FreeOnlineUser force-pushed the upstream/wallet-birthday-pr branch from caa173d to 3d8a5f1 Compare June 3, 2026 12:42
@FreeOnlineUser
Copy link
Copy Markdown
Author

@Jolah1 Thank you, this was a genuinely useful review and I appreciate you picking up a PR that had gone quiet. The silent-fallback catch especially: you're right that degrading to chain tip turns a recovery feature into a 0-sats surprise for exactly the user it's meant to help.

Up front: I use AI assistance (Claude) in this work and verify its output against the code before it goes up. Flagging it so you know what you're reviewing.

All points addressed in the latest push:

  • Silent fallback (builder.rs): now returns BuildError::WalletSetupFailed when a birthday is set but the block hash can't be fetched, instead of silently checkpointing at the tip.
  • Result<_, ()>: ChainSource::get_block_hash_by_height now returns a typed Error::ChainAccessFailed, consistent with the other ChainSource methods, and mirrored into the UDL error enum.
  • Precedence: documented that the birthday height takes precedence over recovery mode.
  • UDL exposure: set_wallet_birthday_height is now in bindings/ldk_node.udl.
  • fmt: cargo fmt --all pass; the four flagged spots are clean.
  • Tests: added onchain_wallet_recovery_with_birthday, which recovers funds via a wallet birthday over random_chain_source (exercised against bitcoind, esplora, and electrum).
  • Rebase: rebased onto current main.

Thanks again.

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.

Wallet birthday height for seed recovery on pruned nodes

3 participants