units: Restore the +1 in relative locktime satisfied by height
What the developer wrote
Authored by Kyle 🐆
units: Restore the +1 in relative locktime satisfied by height
Assisted-by: Claude Opus 5
What changed, and why it matters
This commit fixes an off-by-one bug in how rust-bitcoin checks whether a transaction can yet be spent based on block height. The code was treating a relative locktime of N blocks as satisfied after only N-1 confirmations, instead of the required N confirmations. This could let someone spend coins one block earlier than the Bitcoin protocol allows, which could cause a transaction to be rejected by the network or create inconsistent behavior between this library and other Bitcoin software.
Review whether this bug exists in any released version and, if so, assess whether it could cause users to construct invalid transactions. Consider a security advisory if the library is used by wallets or services that rely on accurate locktime enforcement. Verify that the fix matches Bitcoin Core's interpretation of BIP 68/112 relative locktime semantics.
Security signals we found
Off-by-one error in consensus-relevant locktime validation
Relative locktime satisfied one block too early
Could lead to premature transaction broadcast or mempool rejection
Consensus-adjacent code path in Bitcoin transaction validation
Evidence from the diff
The change is in NumberOfBlocks::is_satisfied_by_height in units/src/locktime/relative/mod.rs. Previously it compared self.to_height() <= diff, where diff is chain_tip - utxo_mined_at. Because a UTXO mined at height H has its first confirmation at height H+1, a locktime of N blocks should require diff >= N, but the old code allowed diff >= N-1. The patch subtracts 1 from the locktime value before comparison: u32::from(self.to_height()).saturating_sub(1) <= diff. Tests are updated to reflect the corrected boundary: a 10-block locktime on a UTXO mined at height 80 is now satisfied at chain tip 89 (9 additional blocks) rather than 90, and a new boundary test verifies behavior for multiple values including u16::MAX.
Changed components
units/src/locktime/relative/mod.rsNumberOfBlocks::is_satisfied_by_heightLockTime::Blocks satisfaction logicInspect captured patch +28 / −5
diff --git a/units/src/locktime/relative/mod.rs b/units/src/locktime/relative/mod.rs
index 4ae2afda..0671a6e2 100644
--- a/units/src/locktime/relative/mod.rs
+++ b/units/src/locktime/relative/mod.rs
@@ -440,9 +440,10 @@ impl NumberOfBlocks {
chain_tip: crate::BlockHeight,
utxo_mined_at: crate::BlockHeight,
) -> Result<bool, InvalidHeightError> {
- chain_tip.checked_sub(utxo_mined_at)
+ chain_tip
+ .checked_sub(utxo_mined_at)
.ok_or(InvalidHeightError { chain_tip, utxo_mined_at })
- .map(|diff| u32::from(self.to_height()) <= diff.to_u32())
+ .map(|diff| u32::from(self.to_height()).saturating_sub(1) <= diff.to_u32())
}
}
@@ -849,7 +850,7 @@ mod tests {
let lock1 = LockTime::Blocks(NumberOfBlocks::from(10));
assert!(lock1.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
- let lock2 = LockTime::Blocks(NumberOfBlocks::from(20));
+ let lock2 = LockTime::Blocks(NumberOfBlocks::from(21));
assert!(lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
let lock3 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(10));
@@ -1010,12 +1011,12 @@ mod tests {
let height_lock = LockTime::Blocks(NumberOfBlocks(10));
// Test case 1: Satisfaction (current_height >= utxo_height + required)
- let chain_state1 = BlockHeight::from_u32(90);
+ let chain_state1 = BlockHeight::from_u32(89);
let utxo_state1 = BlockHeight::from_u32(80);
assert!(height_lock.is_satisfied_by_height(chain_state1, utxo_state1).unwrap());
// Test case 2: Not satisfied (current_height < utxo_height + required)
- let chain_state2 = BlockHeight::from_u32(89);
+ let chain_state2 = BlockHeight::from_u32(88);
let utxo_state2 = BlockHeight::from_u32(80);
assert!(!height_lock.is_satisfied_by_height(chain_state2, utxo_state2).unwrap());
@@ -1026,6 +1027,28 @@ mod tests {
assert!(!max_height_lock.is_satisfied_by_height(chain_state3, utxo_state3).unwrap());
}
+ #[test]
+ fn satisfied_by_height_boundary() {
+ for n in [0, 2, 5, 9, 20, u16::MAX] {
+ let lock = NumberOfBlocks::from_height(n);
+ let mined_at = BlockHeight::from_u32(100);
+
+ let first_spendable_tip = BlockHeight::from_u32(100 + u32::from(n).saturating_sub(1));
+ assert!(
+ lock.is_satisfied_by(first_spendable_tip, mined_at).unwrap(),
+ "n={n} must be satisfied at tip {first_spendable_tip} ({n} confirmations)"
+ );
+
+ if n > 1 {
+ let too_early = BlockHeight::from_u32(100 + u32::from(n) - 2);
+ assert!(
+ !lock.is_satisfied_by(too_early, mined_at).unwrap(),
+ "n={n} must not be satisfied at tip {too_early}"
+ );
+ }
+ }
+ }
+
#[test]
fn test_max_height_satisfaction() {
// If the difference between these two is u32::MAX, we should get Ok(true)
Why this scored 61/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.