Skip to content

Commit 6d610bf

Browse files
committed
Merge #1601: fix(core): calling CheckPoint::insert with existing block must succeed
3ae9ecb test: fix off-by-one in `checkpoint_insert_existing` (valued mammal) e6aeaea doc(core): document panic for `CheckPoint::insert` (valued mammal) 2970b83 fix(core): calling `CheckPoint::insert` with existing block must succeed (志宇) Pull request description: ### Description Previously, we were panicking when the caller tried to insert a block at height 0. However, this should succeed if the block hash does not change. ### Notes to the reviewers For context: * lightningdevkit/ldk-node#358 * https://discord.com/channels/753336465005608961/978744259693916230/1283050849429356629 ### Changelog notice * Fix `CheckPoint::insert` to not panic when inserting existing genesis block (same block height and hash). ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Bugfixes: ~~* [ ] This pull request breaks the existing API~~ * [x] I've added tests to reproduce the issue which are now passing ACKs for top commit: ValuedMammal: Self ACK 3ae9ecb notmandatory: ACK 3ae9ecb Tree-SHA512: 638d8aacac59ad18b5ee369d08f39bb12cc37fa9b3f936404b60dbf44938ef850ca341d00566840b5a77909d31c9291ab56299d986d832005ca96cd91a0b0e55
2 parents 1b50d88 + 3ae9ecb commit 6d610bf

File tree

3 files changed

+50
-2
lines changed

3 files changed

+50
-2
lines changed

crates/core/src/checkpoint.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,17 +171,20 @@ impl CheckPoint {
171171
/// it. If the height already existed and has a conflicting block hash then it will be purged
172172
/// along with all block following it. The returned chain will have a tip of the `block_id`
173173
/// passed in. Of course, if the `block_id` was already present then this just returns `self`.
174+
///
175+
/// # Panics
176+
///
177+
/// This panics if called with a genesis block that differs from that of `self`.
174178
#[must_use]
175179
pub fn insert(self, block_id: BlockId) -> Self {
176-
assert_ne!(block_id.height, 0, "cannot insert the genesis block");
177-
178180
let mut cp = self.clone();
179181
let mut tail = vec![];
180182
let base = loop {
181183
if cp.height() == block_id.height {
182184
if cp.hash() == block_id.hash {
183185
return self;
184186
}
187+
assert_ne!(cp.height(), 0, "cannot replace genesis block");
185188
// if we have a conflict we just return the inserted block because the tail is by
186189
// implication invalid.
187190
tail = vec![];

crates/core/tests/common.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#[allow(unused_macros)]
2+
macro_rules! block_id {
3+
($height:expr, $hash:literal) => {{
4+
bdk_chain::BlockId {
5+
height: $height,
6+
hash: bitcoin::hashes::Hash::hash($hash.as_bytes()),
7+
}
8+
}};
9+
}

crates/core/tests/test_checkpoint.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#[macro_use]
2+
mod common;
3+
4+
use bdk_core::CheckPoint;
5+
6+
/// Inserting a block that already exists in the checkpoint chain must always succeed.
7+
#[test]
8+
fn checkpoint_insert_existing() {
9+
let blocks = &[
10+
block_id!(0, "genesis"),
11+
block_id!(1, "A"),
12+
block_id!(2, "B"),
13+
block_id!(3, "C"),
14+
];
15+
16+
// Index `i` allows us to test with chains of different length.
17+
// Index `j` allows us to test inserting different block heights.
18+
for i in 0..blocks.len() {
19+
let cp_chain = CheckPoint::from_block_ids(blocks[..=i].iter().copied())
20+
.expect("must construct valid chain");
21+
22+
for j in 0..=i {
23+
let block_to_insert = cp_chain
24+
.get(j as u32)
25+
.expect("cp of height must exist")
26+
.block_id();
27+
let new_cp_chain = cp_chain.clone().insert(block_to_insert);
28+
29+
assert_eq!(
30+
new_cp_chain, cp_chain,
31+
"must not divert from original chain"
32+
);
33+
assert!(new_cp_chain.eq_ptr(&cp_chain), "pointers must still match");
34+
}
35+
}
36+
}

0 commit comments

Comments
 (0)