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.

Sign up for GitHub

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

Jump to bottom

Upgrade to rust-bitcoin 0.29 #770

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Upgrade to rust-bitcoin 0.29 #770

merged 4 commits into from
Oct 27, 2022

Conversation

afilini
Copy link
Member

@afilini afilini commented Sep 30, 2022 •
edited by notmandatory
Loading

Description

Upgrade BDK to rust-bitcoin 0.29

Missing pieces:

Notes to the reviewers

The commits still need to be reordered and cleaned up

Changelog notice

  • Upgrade rust-bitcoin to 0.29
  • Remove deprecated "address validators"

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@afilini afilini added this to the Release 0.24.0 Feature Freeze milestone Sep 30, 2022
@evanlinjin evanlinjin mentioned this pull request Oct 9, 2022
Closed
8 tasks
@afilini afilini mentioned this pull request Oct 10, 2022
Closed
@notmandatory
Copy link
Member

I added a link in the description to bitcoindevkit/rust-esplora-client#20, it should be merged shortly and then we can create a new rsut-esplora-client release.

This was referenced Oct 24, 2022
Closed
Closed
@benthecarman benthecarman mentioned this pull request Oct 24, 2022
Merged
@afilini afilini marked this pull request as ready for review October 25, 2022 09:19
@notmandatory notmandatory mentioned this pull request Oct 25, 2022
Merged
5 tasks
notmandatory
notmandatory reviewed Oct 25, 2022
View reviewed changes
src/testutils/blockchain_tests.rs
@@ -1,8 +1,18 @@
// Bitcoin Dev Kit
//
// Copyright (c) 2020-2021 Bitcoin Dev Kit Developers
Copy link
Member

Choose a reason for hiding this comment

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

Small nit, the year should be bumped to 2022 or soon 2023. But I think we should do this for all the files that need updating in one PR. I'll open a PR and we can discuss there.

notmandatory
notmandatory reviewed Oct 25, 2022
View reviewed changes
src/wallet/mod.rs
@@ -4832,7 +4752,7 @@ pub(crate) mod test {
let (wallet, _, _) = get_funded_wallet(get_test_tr_repeated_key());
let addr = wallet.get_address(AddressIndex::New).unwrap();

let path = vec![("rn4nre9c".to_string(), vec![0])]
let path = vec![("e5mmg3xh".to_string(), vec![0])]
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why did the policy path id change here? is it related to the miniscript update?

notmandatory
notmandatory reviewed Oct 25, 2022
View reviewed changes
src/wallet/utils.rs
assert!(result);
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

Were the tests here removed because they aren't needed/valid? or are did they break and need to be fixed?

notmandatory
notmandatory approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@notmandatory notmandatory left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK c7a43d9

I was only able to do a shallow review, and had a few questions but no show stoppers. Since this is primarily version bumping dependencies I'm counting our our test coverage to ensure nothing important was broke.

@notmandatory notmandatory mentioned this pull request Oct 27, 2022
Closed
24 tasks
@notmandatory notmandatory merged commit 5720e38 into bitcoindevkit:master Oct 27, 2022
danielabrozzoni
danielabrozzoni reviewed Oct 27, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

I have some comments, I still need to review src/descriptor/mod.rs

Cargo.toml
@@ -98,6 +98,11 @@ test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "electrsd/bitcoi
test-md-docs = ["electrum"]
test-hardware-signer = ["hardware-signer"]

# This feature is used to run `cargo check` in our CI targeting wasm. It's not recommended
# for libraries to explicitly include the "getrandom/js" feature, so we only do it when
# necessary for running our CI. See: https://docs.rs/getrandom/0.2.8/getrandom/#webassembly-support
Copy link
Member

Choose a reason for hiding this comment

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

nit: add something along the lines as

If you plan to use BDK with wasm, don't include this feature, but include getrandom instead. More info here: link to README

Copy link
Member

Choose a reason for hiding this comment

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

I found this a little confusing too, but I was thinking a little example project would be the best way to demo how to use bdk + wasm.

src/wallet/coin_selection.rs Show resolved Hide resolved
examples/address_validator.rs
@@ -1,63 +0,0 @@
// Bitcoin Dev Kit
Copy link
Member

Choose a reason for hiding this comment

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

Why are address validators being removed? Is it because they are replaced by something in the new rust-bitcoin? (If you could expand a bit more in the changelog as well, it'd be great!)

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh they were deprecated already, sorry

src/wallet/utils.rs
>= self
.create_height
.unwrap_or(0)
.checked_add(n.to_consensus_u32())
Copy link
Member

Choose a reason for hiding this comment

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

if n is in seconds, doing create_height + *seconds value* is wrong

Copy link
Member

@danielabrozzoni danielabrozzoni Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ugggggh this is the good ol' #642, which we never bothered to fix (an attempt was made in #669)

We ignored this for 4 months now, it can wait a little bit longer

src/descriptor/policy.rs
@@ -999,6 +1008,9 @@ impl<Ctx: ScriptContext + 'static> ExtractPolicy for Miniscript<DescriptorPublic

Policy::make_thresh(mapped, threshold)?
}

// Unsupported
Terminal::RawPkH(_) => None,
Copy link
Member

Choose a reason for hiding this comment

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

I am not really sure what's going on here, can you add a little comment explaining why it's unsupported? (If it's trivial, it's ok if you just reply to this comment without commenting the code :))

src/wallet/mod.rs
psbt_input.redeem_script = derived_descriptor.psbt_redeem_script();
psbt_input.witness_script = derived_descriptor.psbt_witness_script();
psbt_input
.update_with_descriptor_unchecked(&derived_descriptor)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining why it's unchecked here?

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

@notmandatory notmandatory notmandatory approved these changes

@danielabrozzoni danielabrozzoni danielabrozzoni left review comments

Assignees

@afilini afilini

Labels
None yet
Projects
BDK Maintenance
Archived in project
Milestone
Release 0.24.0 Feature Freeze
Development

Successfully merging this pull request may close these issues.
3 participants
@afilini @notmandatory @danielabrozzoni