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

multi: encode/decode keys with their parity byte #187

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

jharveyb
Copy link
Collaborator

@jharveyb jharveyb commented Nov 3, 2022

Spinoff of #176.

Currently public keys are sometimes (such as in addresses) unconditionally encoded with an even parity. If that key actually has odd parity, and is already stored, then key upserts behave incorrectly, causing other issues.

To resolve this, we migrate all key encoding & decoding away from the x-only format to the compressed format. This adds a few bytes to the size of some types when encoded, but other techniques can be used to compactly repesented key parity for those types.

Requires BIP updates.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Yeah I think just embracing the "JIT x-only" approach will simplify our lives a lot. We tried to skirt around it can consider the TLV format serialization as "going on chain" but still neded up with things being mixed a bit.

We could also do things like modify the serialization we use for key families before we hash them as well, though this smaller changes fixes some of the non-determinsitic failures we've been seeing elsewhere.

I think the only thing we need to keep in place is the schnorr adherence at the VM level, since that's essentially the same as verifying a taproot output on chain.

@jharveyb jharveyb requested a review from guggero November 4, 2022 03:35
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

address/encoding.go Outdated Show resolved Hide resolved
address/encoding.go Outdated Show resolved Hide resolved
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.

3 participants