Skip to content

fix(wallet): check timelocks against the PSBT in finalize_psbt - #582

Open
Dmenec wants to merge 3 commits into
bitcoindevkit:masterfrom
Dmenec:fix/finalize-psbt-timelocks
Open

Dmenec wants to merge 3 commits into
bitcoindevkit:masterfrom
Dmenec:fix/finalize-psbt-timelocks

Conversation

@Dmenec

@Dmenec Dmenec commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #580, fixes #557.

finalize_psbt satisfied descriptors with the tuple (PsbtInputSatisfier, After, Older). The satisfiers are treated as an OR, and After and Older only look at the chain height. So a PSBT whose nLockTime or nSequence does not enforce the timelock was still finalized through the after/older branch.

PsbtInputSatisfier already checks nLockTime and nSequence, so this PR drops After and Older from the finalizer. SignOptions::assume_height only fed them, so its no longer used.

Notes to the reviewers

After and Older date back to when the finalizer used psbt::Input as its satisfier, which cannot see the transaction and so could not check timelocks on its own. They stayed in the tuple when it was switched to PsbtInputSatisfier in 100f0aa, and since then they could only turn a false into a true.

This also fixes the panic in #557. It came from Older adding the relative locktime to the u32::MAX height used for unconfirmed inputs, as finalize_psbt no longer goes through Older.

Changelog notice

  • Fixed Wallet::finalize_psbt finalizing PSBTs whose nLockTime or nSequence does not enforce the descriptor's timelock.
  • SignOptions::assume_height is deprecated

Before submitting

Bugfixes

@codecov

codecov Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.44%. Comparing base (e46e668) to head (2c5c01a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
- Coverage   81.66%   81.44%   -0.23%     
==========================================
  Files          25       25              
  Lines        6339     6295      -44     
  Branches      302      301       -1     
==========================================
- Hits         5177     5127      -50     
- Misses       1055     1061       +6     
  Partials      107      107              
Flag Coverage Δ
rust 81.44% <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.

@Dmenec
Dmenec force-pushed the fix/finalize-psbt-timelocks branch from 6d86b26 to 3092e28 Compare October 3, 2026 13:25
Dmenec added 3 commits October 5, 2026 21:15
- test_finalize_psbt_requires_locktime_for_cltv
  A PSBT whose `nLockTime` does not enforce `after` must not be finalized, even if the tip is past the lock height.
- test_finalize_psbt_requires_sequence_for_csv
  A PSBT whose `nSequence` does not enforce `older` must not be finalized, even if the input has enough confirmations.
`After` and `Older` are no longer needed in the finalizer to check a PSBT's timelocks, since miniscript's `PsbtInputSatisfier` already checks `nLockTime` and `nSequence`. The satisfiers were combined as a tuple, which miniscript treats as an OR, so `After` and `Older`, which only look at the chain height, could finalize a PSBT that does not enforce the timelock.
`SignOptions::assume_height` only fed `After` and `Older`, so it no longer has any use and is deprecated.
@Dmenec
Dmenec force-pushed the fix/finalize-psbt-timelocks branch from 3092e28 to 2c5c01a Compare October 5, 2026 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

1 participant