Skip to content

crypto: Upstream v0.35.x improvements#9255

Merged
cmwaters merged 3 commits intotendermint:mainfrom
oasisprotocol:yawning/feature/v0.35-crypto
Sep 21, 2022
Merged

crypto: Upstream v0.35.x improvements#9255
cmwaters merged 3 commits intotendermint:mainfrom
oasisprotocol:yawning/feature/v0.35-crypto

Conversation

@Yawning
Copy link
Contributor

@Yawning Yawning commented Aug 15, 2022

This is my stab at #9186. Per discussion in the issue, the order in which things were done is different from how the changes went into v0.35.x for ease of doing the backport. Note that for the sake of expediency I opted to do this manually instead of via git.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@Yawning Yawning marked this pull request as ready for review August 15, 2022 13:31
@Yawning Yawning requested a review from ebuchman as a code owner August 15, 2022 13:31
@Yawning Yawning requested a review from a team August 15, 2022 13:31
@cmwaters
Copy link
Contributor

Thanks @Yawning. Just to confirm - none of these changes will break downstream users?

@Yawning
Copy link
Contributor Author

Yawning commented Aug 16, 2022

Thanks @Yawning. Just to confirm - none of these changes will break downstream users?

This (like the 0.35.x version) will change sr25519 key derivation because the v0.34.x method of doing so is wacky. I think the consensus at the time was that basically no one is using sr25519. It's in CHANGELOG_PENDING.md.

@Yawning Yawning force-pushed the yawning/feature/v0.35-crypto branch from 05f2304 to cc1619d Compare August 19, 2022 07:32
gogotypes "github.com/gogo/protobuf/types"
"github.com/gtank/merlin"
pool "github.com/libp2p/go-buffer-pool"
"github.com/oasisprotocol/curve25519-voi/primitives/merlin"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more performant or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code's nicer (IMO), more full featured and it removes 2 dependencies. IIRC my strobe implementation is more performant for "sensible" message sizes, but the difference shouldn't be significant or noticeable for something like this.

@Yawning
Copy link
Contributor Author

Yawning commented Aug 26, 2022

Just to confirm - none of these changes will break downstream users?

Oh. I forgot to mention this. Switching to ZIP-215 is technically a breaking change as it is possible to produce signatures that will pass or fail verification depending on the nitpicky details things like ZIP-215 was written to address.

voi certainly supports "Behaving exactly like crypto/ed25519", and having it do so will be a one line change to this branch. However while it still will be faster, batch verification under the hood will be done serially for correctness reasons.

Looking at #5632, it appears that some of the sub-projects migrated to using ZIP-215 (for v0.35.x), so they would have to undo the change, or ZIP-215 happens...

@thanethomson thanethomson added the S:wip Work in progress (prevents stalebot from automatically closing) label Sep 2, 2022
@zmanian
Copy link
Contributor

zmanian commented Sep 12, 2022

ZIP 215 is technically not backwards compatible but practically speaking it is backwards compatible.

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Thanks for porting this change. Do you mind fixing the conflicts and updating the branch and then we can merge.

@Yawning Yawning force-pushed the yawning/feature/v0.35-crypto branch from cc1619d to f9d41a8 Compare September 20, 2022 12:27
This switches the ed25519, sr25519 and merlin provider to curve25519-voi
and additionally adopts ZIP-215 semantics for ed25519 verification.
This commit adds the batch verification interface, but does not enable
it for anything.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S:wip Work in progress (prevents stalebot from automatically closing)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants