fix(core): calling CheckPoint::insert with existing block must succeed
#1601
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tnull
added a commit
to tnull/ldk-node
that referenced
this pull request
Sep 12, 2024
BDK currently panics if we start syncing the node while the tip height is still 0, which happens in our usual regtest setup. Here, we make sure to premine *before* starting up the nodes to avoid hitting this panic. While splitting premining and distributing funds in this way is fine (and even makes sense) in most cases, in some cases it's annoying as we wouldn't previously premine at all, but are now required to. So we should eventually drop/revert (at least this part of) this workaround as soon as BDK ships the fix on their end (bitcoindevkit/bdk#1601).
tnull
added a commit
to tnull/ldk-node
that referenced
this pull request
Sep 12, 2024
BDK currently panics if we start syncing the node while the tip height is still 0, which happens in our usual regtest setup. Here, we make sure to premine *before* starting up the nodes to avoid hitting this panic. While splitting premining and distributing funds in this way is fine (and even makes sense) in most cases, in some cases it's annoying as we wouldn't previously premine at all, but are now required to. So we should eventually drop/revert (at least this part of) this workaround as soon as BDK ships the fix on their end (bitcoindevkit/bdk#1601).
evanlinjin
force-pushed
the
fix_checkpoint_insert
branch
from
34fa146
to
e062d58
Compare
tnull
added a commit
to tnull/ldk-node
that referenced
this pull request
Sep 12, 2024
BDK currently panics if we start syncing the node while the tip height is still 0, which happens in our usual regtest setup. Here, we make sure to premine *before* starting up the nodes to avoid hitting this panic. While splitting premining and distributing funds in this way is fine (and even makes sense) in most cases, in some cases it's annoying as we wouldn't previously premine at all, but are now required to. So we should eventually drop/revert (at least this part of) this workaround as soon as BDK ships the fix on their end (bitcoindevkit/bdk#1601).
LagginTimes
reviewed
Sep 12, 2024
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.
evanlinjin
force-pushed
the
fix_checkpoint_insert
branch
from
e062d58
to
2970b83
Compare
|
ACK 2970b83 |
ValuedMammal
approved these changes
Sep 12, 2024
There was a problem hiding this comment.
I think it's a fine solution but ideally we should ensure bdk never calls CheckPoint::insert with a block height of 0 for example in electrum and esplora (still looking for a way to reproduce it).
We can add a Panics section to the docs, something like
/// # Panics
///
/// This panics if called with a genesis block that differs from that of `self`.
crates/core/tests/test_checkpoint.rs
Outdated
| let cp_chain = CheckPoint::from_block_ids(blocks[..=i].iter().copied()) | ||
| .expect("must construct valid chain"); | ||
|
|
||
| for j in 0..i { |
There was a problem hiding this comment.
Suggested change
| for j in 0..i { | |
| for j in 0..=i { |
tnull
added a commit
to tnull/ldk-node
that referenced
this pull request
Sep 13, 2024
BDK currently panics if we start syncing the node while the tip height is still 0, which happens in our usual regtest setup. Here, we make sure to premine *before* starting up the nodes to avoid hitting this panic. While splitting premining and distributing funds in this way is fine (and even makes sense) in most cases, in some cases it's annoying as we wouldn't previously premine at all, but are now required to. So we should eventually drop/revert (at least this part of) this workaround as soon as BDK ships the fix on their end (bitcoindevkit/bdk#1601).
This was referenced Sep 13, 2024
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Changelog notice
CheckPoint::insertto not panic when inserting existing genesis block (same block height and hash).Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes:
* [ ] This pull request breaks the existing API