Skip to content

Conversation

@apoelstra
Copy link
Member

We've got a ton of minor changes in, plus fixing the Parity type and adding some extra serde impls. Let's push a minor version out so that we can move on to updating the upstream libsecp.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

For what its worth, LGTM. Although I'm not sure its worth having links to all those docs changes, they are all pretty trivial.

I checked the markdown renders correctly (on GitHub) and that all the links work.

@apoelstra
Copy link
Member Author

Thanks! Normally I wouldn't bother linking to all the docs stuff, but there were a bunch and I could make the links pretty small.

sanket1729
sanket1729 previously approved these changes Feb 2, 2022
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

utACK 61b6add. Looks good to me. Did not verify the release notes.

@apoelstra
Copy link
Member Author

cc @elichai @TheBlueMatt could one of you ack this so I can merge and publish?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 8, 2022

Note that the Parity change is theoretically breaking change because someone could write this:

let Parity { .. } = some_fn_returning_parity();

(or in other pattern matching)

Which works before and breaks after changing to enum. I know, it's dumb, I posted about this in IRLO recently. My personal approach for my libs is "doing this is too weird and hopefully unlikely and trivial to fix, if you do this sorry-not sorry, I'm making non-breaking release".

While I'm in favor of ignoring this I believe it's the right thing for me to point this out as others may have a different view.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 8, 2022

One more thing, would you be up for ninja-doc-improvement of Parity before release? I now noticed the conversions number -> Parity are totally unclear - what values mean what? Could some panic?

@apoelstra
Copy link
Member Author

@Kixunil sure, go for it.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

Done. While working on it I noticed a few other problems:

  • Parity is serialized as i32 - this may waste space in binary encodings. I think this is significant, do we want this?
  • The serde error message is prefixed with "Expecting" which is wrong.
  • It'd be nicer to map parity error as invalid_value in the Deserialize impl.

@apoelstra
Copy link
Member Author

Oof, we should definitely fix the parity to be serialized as a single byte. I guess we can't do that in a minor release though.

The error stuff I think is ok to fix in a minor release, if you are so inclined.

But bear in mind that my intention is to do a major release basically ASAP after I get this merged, and to use the major release in the next rust-bitcoin release. Mainly I want #384 but I'm happy to mess with the Parity API too.

apoelstra added a commit that referenced this pull request Feb 9, 2022
705c9cf Clarified conversions between `Parity` and integers (Martin Habovstiak)

Pull request description:

  This was discussed in #390 (comment)

ACKs for top commit:
  apoelstra:
    ACK 705c9cf

Tree-SHA512: 3ba2ec566099c3c6d1c6f830e4959312b818b8766d924e3d995e6b23bd196ab747cc03d46f494ef451569188b0163f53e3236cacd20bfae9118ee76bcdbc9c02
@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

OK, will try to be quick and submit two PRs.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 9, 2022

I found one more thing: Error doesn't carry the original value which can be annoying for debugging. Also it's not great that it's the same Error type as others while it can only return one variant. (Same stuff I want to solve at bitcoin.)

@sanket1729
Copy link
Member

@Kixunil @apoelstra, anything I can help in unblocking this?

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 11, 2022

@sanket1729 I'm done with my PRs to this one.

elichai
elichai previously approved these changes Feb 15, 2022
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

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

ACK 61b6add

@apoelstra
Copy link
Member Author

We need #407 first. Sorry, my bad.

In the meantime I'll update the changelog here assuming that we have 407.

@apoelstra
Copy link
Member Author

@real-or-random @elichai can I get an ACK on this?

Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

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

ACK

@apoelstra apoelstra merged commit ce255fd into rust-bitcoin:master Feb 23, 2022
@apoelstra apoelstra deleted the 2022-01--secp-0.21.3 branch February 23, 2022 18:50
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
…`Parity` and integers

705c9cfbc15bb6fdcae7355ac9fc4de920876abd Clarified conversions between `Parity` and integers (Martin Habovstiak)

Pull request description:

  This was discussed in rust-bitcoin/rust-secp256k1#390 (comment)

ACKs for top commit:
  apoelstra:
    ACK 705c9cfbc15bb6fdcae7355ac9fc4de920876abd

Tree-SHA512: 3ba2ec566099c3c6d1c6f830e4959312b818b8766d924e3d995e6b23bd196ab747cc03d46f494ef451569188b0163f53e3236cacd20bfae9118ee76bcdbc9c02
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
65d32af6fd085c6073de89e4aa3c306e75f6feff bump version to 0.21.3 (Andrew Poelstra)

Pull request description:

  We've got a ton of minor changes in, plus fixing the Parity type and adding some extra serde impls. Let's push a minor version out so that we can move on to updating the upstream libsecp.

Top commit has no ACKs.

Tree-SHA512: 584c03106124b4152b8971ac6d0587a26d2aca9187f88d8228a356c2327bf066d2c9b8134149f9ee3bc5f3712f64559b32843aa8e92d3395c5a1bd53de5442ce
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
…`Parity` and integers

705c9cfbc15bb6fdcae7355ac9fc4de920876abd Clarified conversions between `Parity` and integers (Martin Habovstiak)

Pull request description:

  This was discussed in rust-bitcoin/rust-secp256k1#390 (comment)

ACKs for top commit:
  apoelstra:
    ACK 705c9cfbc15bb6fdcae7355ac9fc4de920876abd

Tree-SHA512: 3ba2ec566099c3c6d1c6f830e4959312b818b8766d924e3d995e6b23bd196ab747cc03d46f494ef451569188b0163f53e3236cacd20bfae9118ee76bcdbc9c02
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
65d32af6fd085c6073de89e4aa3c306e75f6feff bump version to 0.21.3 (Andrew Poelstra)

Pull request description:

  We've got a ton of minor changes in, plus fixing the Parity type and adding some extra serde impls. Let's push a minor version out so that we can move on to updating the upstream libsecp.

Top commit has no ACKs.

Tree-SHA512: 584c03106124b4152b8971ac6d0587a26d2aca9187f88d8228a356c2327bf066d2c9b8134149f9ee3bc5f3712f64559b32843aa8e92d3395c5a1bd53de5442ce
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.

5 participants