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

update rust-bitcoin to 0.24.0 #130

Merged
merged 2 commits into from Sep 18, 2020
Merged

update rust-bitcoin to 0.24.0 #130

merged 2 commits into from Sep 18, 2020

Conversation

apoelstra
Copy link
Member

No description provided.

@darosior
Copy link
Contributor

Rustfmt is unhappy but otherwise lgtm

@apoelstra
Copy link
Member Author

oops. ran cargo fmt

@darosior
Copy link
Contributor

darosior commented Sep 11, 2020

Hmm i think the failure on 1.22 means you need to pin CC as you did on rust-bitcoin ?
Edit: see 1b4feae in #131

@apoelstra
Copy link
Member Author

Thanks! Cherry-picked your commit, let's see if it works now.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Copy link
Contributor

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Propose to use this diff, which simplifies code a bit:

diff --git a/src/descriptor/create_descriptor.rs b/src/descriptor/create_descriptor.rs
index d36a7cb..6d5a08b 100644
--- a/src/descriptor/create_descriptor.rs
+++ b/src/descriptor/create_descriptor.rs
@@ -24,9 +24,9 @@ use ToPublicKey;
 /// NOTE: Miniscript pushes should only be either boolean, 1 or 0, signatures, and hash preimages.
 /// As per the current implementation, PUSH_NUM2 results in an error
 fn instr_to_stackelem<'txin>(
-    ins: &Result<Instruction<'txin>, bitcoin::blockdata::script::Error>,
+    ins: Result<Instruction<'txin>, bitcoin::blockdata::script::Error>,
 ) -> Result<StackElement<'txin>, Error> {
-    match *ins {
+    match ins {
         //Also covers the dissatisfied case as PushBytes0
         Ok(Instruction::PushBytes(v)) => Ok(StackElement::from(v)),
         Ok(Instruction::Op(opcodes::all::OP_PUSHNUM_1)) => Ok(StackElement::Satisfied),
@@ -42,7 +42,7 @@ fn parse_scriptsig_top<'txin>(
 ) -> Result<(Vec<u8>, Stack<'txin>), Error> {
     let stack: Result<Vec<StackElement>, Error> = script_sig
         .instructions_minimal()
-        .map(|instr| instr_to_stackelem(&instr))
+        .map(instr_to_stackelem)
         .collect();
     let mut stack = stack?;
     if let Some(StackElement::Push(pk_bytes)) = stack.pop() {
@@ -64,7 +64,7 @@ fn verify_p2pk<'txin>(
     if let Ok(pk) = bitcoin::PublicKey::from_slice(&pk_bytes[1..script_pubkey_len - 1]) {
         let stack: Result<Vec<StackElement>, Error> = script_sig
             .instructions_minimal()
-            .map(|instr| instr_to_stackelem(&instr))
+            .map(instr_to_stackelem)
             .collect();
         if !witness.is_empty() {
             Err(Error::NonEmptyWitness)
@@ -228,7 +228,7 @@ pub fn from_txin_with_witness_stack<'txin>(
         //bare
         let stack: Result<Vec<StackElement>, Error> = script_sig
             .instructions_minimal()
-            .map(|instr| instr_to_stackelem(&instr))
+            .map(instr_to_stackelem)
             .collect();
         if !witness.is_empty() {
             return Err(Error::NonEmptyWitness);

@@ -39,7 +41,7 @@ fn parse_scriptsig_top<'txin>(
script_sig: &'txin bitcoin::Script,
) -> Result<(Vec<u8>, Stack<'txin>), Error> {
let stack: Result<Vec<StackElement>, Error> = script_sig
.iter(true)
.instructions_minimal()
.map(|instr| instr_to_stackelem(&instr))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply .map(instr_to_stackelem)? I tried locally, it compiles well with all versions, starting from MSRV

Copy link
Member

Choose a reason for hiding this comment

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

I think this is unrelated to the intent of the PR. It is indeed a useful simplification. Can you raise another PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done: #133

@sanket1729 sanket1729 merged commit 3515c6a into master Sep 18, 2020
@apoelstra
Copy link
Member Author

Can you make a commit with these changes that I can cherry-pick?

@dr-orlovsky
Copy link
Contributor

@apoelstra here it is: #133

@sanket1729 sanket1729 mentioned this pull request Oct 1, 2020
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

4 participants