Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migration to bitcoin 0.30 #145

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vincenzopalazzo
Copy link
Collaborator

This is a WIP, so this patch will be out just for early review.

@vladimirfomene
Copy link

Hi @vincenzopalazzo ! I was wondering if you have been able to make progress with this PR locally. We will need it for BDK's integration with Nakamoto. If not, I'm willing to help if you don't have time to focus on it.

@vincenzopalazzo
Copy link
Collaborator Author

In reality, I was waiting bdk team to do this because I was blocked on my project for the same reason 😄 I did not finish this before because everyone was not upgrading to the new version of rust-bitcoin

So now that the bdk team has done it, I will finish the PR

@vladimirfomene
Copy link

vladimirfomene commented Aug 8, 2023

In reality, I was waiting bdk team to do this because I was blocked on my project for the same reason 😄 I did not finish this before because everyone was not upgrading to the new version of rust-bitcoin

So now that the bdk team has done it, I will finish the PR

Thanks

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Small self note that I need to look at


target = target.mul_u32(adjusted_timespan);
target = target / Target::from_u64(params.pow_target_timespan).unwrap();
target = target * adjusted_timespan;
Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo Aug 13, 2023

Choose a reason for hiding this comment

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

looks like this is not correct, the target is a u256 so this will overflow.

Maybe rust-bitcoin should make available the same operation?

@vincenzopalazzo
Copy link
Collaborator Author

This is blocked by rust-bitcoin/rust-bitcoin#1993

let mut best_work = Uint256::zero();
// FIXME: this need to be a Work by network? or the min is equal for
// every network?
let mut best_work = Work::MAINNET_MIN;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be general and not with mainet only

use std::sync::{Arc, RwLock};

use quickcheck::{Arbitrary, Gen};
use quickcheck_macros::quickcheck;

use nakamoto_common::bitcoin;
use nakamoto_common::bitcoin::blockdata::block::BlockHeader;
use nakamoto_common::bitcoin::{self, CompactTarget};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

split in two line

p2p/src/fsm.rs Outdated
return self.disconnect(addr, DisconnectReason::PeerMagic(msg.magic));
return self.disconnect(
addr,
DisconnectReason::PeerMagic(u32::from_be_bytes(msg.magic.to_bytes())),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mh maybe it is possible to better than this at the API level

@@ -3,7 +3,7 @@ use std::collections::BTreeSet;
use std::ops::RangeInclusive;
use std::rc::Rc;

use nakamoto_common::bitcoin::util::bip158;
use nakamoto_common::bitcoin::{bip158, ScriptBuf};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this need to be splitted

@@ -170,7 +170,7 @@ impl FeeEstimator {

let fee = received - sent;
let weight = tx.weight();
let rate = fee as f64 / (weight as f64 / WITNESS_SCALE_FACTOR as f64);
let rate = fee as f64 / (weight.to_wu() as f64 / WITNESS_SCALE_FACTOR as f64);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make a double check, what is to_wu

@@ -6,6 +6,7 @@
//! with specific capabilities, eg. peer disconnection, message sending etc. to
//! communicate with the network.
use log::*;
use nakamoto_common::bitcoin::network::Magic;
Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo Aug 13, 2023

Choose a reason for hiding this comment

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

fix the import order

@@ -7,9 +7,11 @@ use std::io;
use std::iter;
use std::net;
use std::ops::Bound;
use std::str::FromStr;
use std::sync::Arc;

use log::*;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix the import order

@@ -520,9 +510,9 @@ fn test_bitcoin_difficulty() {
cache.import(
height,
BlockHeader {
version: 1,
version: Version::from_consensus(1),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
version: Version::from_consensus(1),
version: Version::ONE,

Copy link
Collaborator Author

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

This is my self review to see the blocking step to get this PR merged and also to see if I introduce regression

@vladimirfomene
Copy link

vladimirfomene commented Sep 22, 2023

Hi @vincenzopalazzo ! I know you are very busy doing other important work but I just wanted to check if there has been any progress. We are still waiting on it to implement BDK's CBF syncing feature.

@cloudhead
Copy link
Owner

Still waiting on rust-bitcoin/rust-bitcoin#2090 afaict

@vincenzopalazzo
Copy link
Collaborator Author

Ok finally this is merged rust-bitcoin/rust-bitcoin#2740

Waiting so see what will be the next release where this will be merged and I will finish this PR or maybe open a new one

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.

None yet

3 participants