Skip to content

Conversation

@GJaubert
Copy link

@GJaubert GJaubert commented Mar 2, 2025

I've completed the implementation of BIP324, which requires an update to the Bitcoin crate to version 0.32.5. This PR addresses both changes.
While I know some optimizations can be made, I hope this saves much time if the bitcoin version of Nakamoto is updated in the future.

@GJaubert GJaubert mentioned this pull request Mar 2, 2025
use nakamoto_common as common;
use nakamoto_common::bitcoin;
use nakamoto_common::bitcoin::blockdata::block::BlockHeader;
use nakamoto_common::bitcoin::blockdata::block::Header as BitcoinBlockHeader;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that renaming this alias to BlockHeader would save some code churn on this refactor.

checkpoints: BTreeMap<Height, BlockHash>,
params: Params,
/// Total cumulative work on the active chain.
chainwork: Uint256,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of casts back and forth between the bitcoin type for Work and this big integer representation. Within the bitcoin crate the Work type is implemented as a UInt256 anyway, so I think this field should be refactored to the Work type


/// Rollback active chain to the given height. Returns the list of rolled-back headers.
fn rollback(&mut self, height: Height) -> Result<Vec<(Height, BlockHeader)>, Error> {
fn rollback(&mut self, height: Height) -> Result<Vec<(Height, Header)>, Error> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, ideally the BlockHeader naming is preserved to condense the size of the diff

pos += header.consensus_encode(&mut stream)? as u64;
pos += header
.consensus_encode(&mut stream)
.expect("consensus encoded success") as u64;
Copy link

@rustaceanrob rustaceanrob Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't add an expect here. I believe you'll have to modify the error variant to accept the consensus_encode error

self.event(Event::MessageReceived {
from: addr,
message: Arc::new(msg.payload),
message: Arc::new(msg.payload().clone()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there is no clone here. I believe there exists a method into_payload

start: Bound<Height>,
end: Bound<Height>,
watch: Vec<Script>,
watch: Vec<Box<Script>>,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this require a Box?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants