fix(wallet): treat unreachable relative locktimes as unsatisfied

[trakshan-mishra]

Fixes #557.

Problem

Older::check_older (src/wallet/utils.rs) computes the height at which a relative locktime is satisfied as create_height + n and unwraps the sum with expect("Overflowing addition"). Wallet::finalize_psbt maps an unconfirmed previous transaction to a create_height of u32::MAX, so that addition overflows and panics for any older(n) with n greater than zero.

The Older satisfier is only consulted when the input's nSequence does not already satisfy the CSV branch, so this is reachable through ordinary use: spending an unconfirmed UTXO of a descriptor that contains an older() branch which is not the one being used, e.g. spending via pk(A) of or_d(pk(A),and_v(v:pk(B),older(144))). Since Wallet::sign finalizes by default, callers reach it without opting in.

Fix

Return false when the satisfaction height does not fit in a u32, rather than panicking. Such a height can never be reached, so the branch is simply not satisfied — the same answer the comparison would give with wider integers. This matches the behaviour the issue asks for ("the older() branch should simply be treated as not satisfied").

The change is in the Satisfier impl, so it covers both call sites: finalization in src/wallet/mod.rs and policy extraction in src/descriptor/policy.rs.

After::check_after performs no addition and is unaffected.

Tests

• test_finalize_psbt_with_unconfirmed_input_and_unused_csv_branch (tests/wallet.rs) reproduces the reported panic end to end. Verified to fail on master with panicked at src/wallet/utils.rs:117: Overflowing addition, and to pass with this change.
• test_check_older_unreachable_satisfaction_height_is_not_satisfied covers the overflow directly at the unit level.
• test_check_older_compares_against_the_satisfaction_height covers the ordinary satisfied and unsatisfied heights, which had no unit coverage before.

Checklist

• All commits are GPG signed.
• The PR description links to the issue it solves.
• Ran the just pre-push steps locally: cargo +nightly fmt --all -- --check, both cargo check variants, clippy with -D warnings on the stable and unstable surfaces, cargo test --workspace on both surfaces, and cargo doc with -D warnings. All pass.
• Tests reproducing the bug (and now passing) have been added.
• No API change, so nothing breaking.

One note: cargo +nightly fmt on current master also wants to reformat src/descriptor/policy.rs, which is unrelated to this fix (and looks like the nightly drift tracked in #535), so I left that file untouched.

[j-kon]

ACK 7461a43

Verified:

• Confirmed panic reproduction on master at src/wallet/utils.rs:117 in both debug and release builds when finalizing an unconfirmed input in a descriptor with an unused older(n) branch.
• Confirmed PR #565 cleanly resolves the panic and passes all unit/integration tests in both profiles.
• Verified arithmetic boundary transitions around u32::MAX (create_height = u32::MAX - 1, u32::MAX - 100, etc.) and confirmed exact >= comparison matches BIP68 relative locktime semantics.
• Verified that returning false on unreachable height correctly signals an unsatisfied condition to Miniscript satisfiers.

[Dmenec]

Thank you for the fix!

I'm not a big fan of relying on the overflow to decide that the timelock isn't met yet. Wouldn't it be cleaner to get rid of the u32::MAX placeholder and make the state explicit, maybe with some enum?

so check_older can just return false for unconfirmed inputs?

[trakshan-mishra]

Thanks, that makes sense. The None => false arm only works because of the u32::MAX placeholder, and the comments I had to add to explain that were a sign it's the wrong place to fix it.

One thing I ran into while looking at this: create_height: None already has a meaning. try_finalize_psbt passes None when the previous tx isn't in the canonical set, and check_older does unwrap_or(0) on it, so that case counts the timelock as met. Reusing None for "unconfirmed" would flip that case to unsatisfied. That's a behavior change I don't want to put into this PR by accident.

So I'd go with an enum:

pub(crate) enum ConfirmationHeight {
    Confirmed(u32),
    Unconfirmed,
}

• finalize_psbt builds a HashMap<Txid, ConfirmationHeight> instead of mapping unconfirmed txs to u32::MAX.
• Older::create_height becomes Option<ConfirmationHeight>, so None still means "not found" and keeps its current behavior.
• check_older returns false for Unconfirmed directly. For Confirmed(h) I'd keep checked_add, but return false on overflow instead of panicking.
• policy.rs passes Confirmed(input_max_height), so nothing changes on that path.

Older isn't re-exported from utils, so this doesn't touch the public API. The regression test in tests/wallet.rs stays as is.

Does that shape work for you, or would you rather have a third Unknown variant instead of the Option wrapper? I'll push once you confirm and ask @j-kon to take another look, since this replaces the commit they ACKed.

[trakshan-mishra]

I went ahead and pushed the enum change so you can see the code rather than the description: bc886ff, as a separate commit on top of the original fix so the delta is easy to review. Happy to squash if you'd prefer one commit. @j-kon, this changes the code you ACKed, could you take another look when you have time?

[j-kon]

I went ahead and pushed the enum change so you can see the code rather than the description: bc886ff, as a separate commit on top of the original fix so the delta is easy to review. Happy to squash if you'd prefer one commit. @j-kon, this changes the code you ACKed, could you take another look when you have time?

Thanks for pushing this as a separate commit. The enum approach looks cleaner and makes the unconfirmed state explicit while keeping None semantics separate. Since this replaces the commit I previously ACKed, I’ll re-review the bc886ff delta and rerun the relevant CSV/unconfirmed regression tests before updating my review.

[yan-pi]

tACK bc886ff

I agree that the enum change is cleaner than using u32::MAX. It makes the unconfirmed case explicit while preserving the existing None behavior.

Could we add a unit test for create_height = None? The current tests cover confirmed, unconfirmed, and confirmed-height overflow, but not the not-found path.

IMO This is non-blocking.

[trakshan-mishra]

Thanks @yan-pi, added in b806e04. It covers both sides of the boundary for the not-found path: with create_height = None, older(144) is satisfied at height 144 and not at 143.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.68%. Comparing base (6fc6846) to head (b806e04).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   81.91%   81.68%   -0.23%     
==========================================
  Files          25       25              
  Lines        6535     6344     -191     
  Branches      302      302              
==========================================
- Hits         5353     5182     -171     
+ Misses       1075     1055      -20     
  Partials      107      107              

Flag	Coverage Δ
rust	81.68% <100.00%> (-0.23%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.