units: Fix off-by-one error in satisfied by height
What the developer wrote
Authored by Tobin C. Harding
units: Fix off-by-one error in satisfied by height
In #6384 we fixed an off-by-one error in the satisfied by time logic.
I swear I checked the satisfied by height code at that time but
apparently not. While reviewing #6581 I saw the same bug.
The problem is we both +1 to height _and_ use `<=`. Either of these on
its own is correct since we know the next block has height one higher
and the check is 'lower than the height of the next block'.
The unit boundary test was wrong too.
What changed, and why it matters
This commit fixes an off-by-one error in how the rust-bitcoin library decides whether a Bitcoin transaction's height-based locktime is satisfied. Because the code both added 1 to the current chain tip height and used a 'less than or equal to' comparison, it considered a locktime satisfied one block too early. A transaction that should only be spendable after block 101 could have been treated as spendable once block 100 was the chain tip. The same kind of bug was also present in relative height locktime checks. The fix removes the extra +1 and switches to a strict 'less than' comparison for absolute height locks, and removes the +1 for relative height locks.
Review any downstream code that relies on these locktime satisfaction helpers, especially wallet or mempool logic that might have accepted transactions one block too early. Consider whether the previous behavior could have caused invalid transactions to be constructed or accepted, and add regression tests for the corrected boundary.
Security signals we found
Off-by-one in locktime satisfaction logic
Incorrect boundary unit test reinforced the bug
Same class of bug previously fixed for time-based locktimes in PR #6384
Relative and absolute height locktime checks both affected
Evidence from the diff
In units/src/locktime/absolute/mod.rs, Height::is_satisfied_by computed next_block_height = chain_tip + 1 and then compared self <= next_block_height. This double-counts the next block, making the lock satisfied when self == chain_tip + 1, i.e., one block early. The fix changes the comparison to self < next_block_height. In units/src/locktime/relative/mod.rs, NumberOfBlocks::is_satisfied_by_height similarly added 1 to the difference between chain_tip and utxo_mined_at before comparing with <=. The fix removes the +1 and uses the raw difference. Unit tests were updated to reflect the corrected boundary behavior.
Changed components
units/src/locktime/absolute/mod.rsunits/src/locktime/relative/mod.rsHeight::is_satisfied_byNumberOfBlocks::is_satisfied_by_heightInspect captured patch +12 / −19
diff --git a/units/src/locktime/absolute/mod.rs b/units/src/locktime/absolute/mod.rs
index 93575516..11720c22 100644
--- a/units/src/locktime/absolute/mod.rs
+++ b/units/src/locktime/absolute/mod.rs
@@ -574,8 +574,9 @@ impl Height {
#[inline]
pub fn is_satisfied_by(self, height: Self) -> bool {
// Use u64 so that there can be no overflow.
+ // The next block will have a height chain tip + 1
let next_block_height = u64::from(height.to_u32()) + 1;
- u64::from(self.to_u32()) <= next_block_height
+ u64::from(self.to_u32()) < next_block_height
}
}
@@ -1035,14 +1036,14 @@ mod tests {
fn height_is_satisfied_by() {
let chain_tip = Height::from_u32(100).unwrap();
- // lock is satisfied if transaction can go in the next block (height <= chain_tip + 1).
- let locktime = Height::from_u32(100).unwrap();
+ // lock is satisfied if transaction can go in the next block (height < chain_tip + 1).
+ let locktime = Height::from_u32(99).unwrap();
assert!(locktime.is_satisfied_by(chain_tip));
- let locktime = Height::from_u32(101).unwrap();
+ let locktime = Height::from_u32(100).unwrap();
assert!(locktime.is_satisfied_by(chain_tip));
// It is not satisfied if the lock height is after the next block.
- let locktime = Height::from_u32(102).unwrap();
+ let locktime = Height::from_u32(101).unwrap();
assert!(!locktime.is_satisfied_by(chain_tip));
}
diff --git a/units/src/locktime/relative/mod.rs b/units/src/locktime/relative/mod.rs
index f2b68e3b..4ae2afda 100644
--- a/units/src/locktime/relative/mod.rs
+++ b/units/src/locktime/relative/mod.rs
@@ -440,17 +440,9 @@ impl NumberOfBlocks {
chain_tip: crate::BlockHeight,
utxo_mined_at: crate::BlockHeight,
) -> Result<bool, InvalidHeightError> {
- match chain_tip.checked_sub(utxo_mined_at) {
- Some(diff) => {
- if diff.to_u32() == u32::MAX {
- // Weird but ok none the less - protects against overflow below.
- return Ok(true);
- }
- // +1 because the next block will have height 1 higher than `chain_tip`.
- Ok(u32::from(self.to_height()) <= diff.to_u32() + 1)
- }
- None => Err(InvalidHeightError { chain_tip, 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())
}
}
@@ -857,7 +849,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(21));
+ let lock2 = LockTime::Blocks(NumberOfBlocks::from(20));
assert!(lock2.is_satisfied_by(chain_height, chain_mtp, utxo_height, utxo_mtp).unwrap());
let lock3 = LockTime::Time(NumberOf512Seconds::from_512_second_intervals(10));
@@ -1018,12 +1010,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(89);
+ let chain_state1 = BlockHeight::from_u32(90);
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(88);
+ let chain_state2 = BlockHeight::from_u32(89);
let utxo_state2 = BlockHeight::from_u32(80);
assert!(!height_lock.is_satisfied_by_height(chain_state2, utxo_state2).unwrap());
Why this scored 64/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.