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

PSET: Various fixes #1121

Merged
merged 9 commits into from
Aug 9, 2022
Merged

Conversation

jgriffiths
Copy link
Contributor

@jgriffiths jgriffiths commented Jul 1, 2022

  • Fix PSET blinding crash
  • Fix failure to handle unknown extension fields
  • Fix PSBT field serialization order to match core; update test serializations to current expected format
  • Add parsepsbt rpc for testing
  • Fixes serialization of scalars to follow the PSET spec. To reduce interop concerns with 3rd party implementations, accept both the old and new scalar serialization when parsing.

Fixes #1065

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.

code review ACK 32c3a98. Double checked that rust-elements parser and parser that allen implemnted https://tools.blockstream.com/psbt parses the new serialization correctly.

src/psbt.h Show resolved Hide resolved
src/psbt.h Show resolved Hide resolved
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 32c3a98

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

utACK b4378ec

@jgriffiths jgriffiths changed the title PSET: Fix PSBT_ELEMENTS_GLOBAL_SCALAR serialization PSET: Various fixes Jul 19, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 32ac36a

A single 0x00 byte indicates a zero length field; we must skip parsing
that field otherwise the length will be expected to be read again for
the vector that is passed in to revieve the value.

This allows PSBT_ELEMENTS_GLOBAL_SCALAR to be parsed when it is
serialized according to the spec, i.e. both of the following cases
will correctly parse to the same representation:

$cli decodepsbt 'cHNldP8B+wQCAAAAAQIEAgAAAAEEAQABBQEAJ/wEcHNldAABAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fIAEAAA=='

and

$cli decodepsbt 'cHNldP8B+wQCAAAAAQIEAgAAAAEEAQABBQEAJ/wEcHNldAABAgMEBQYHCAkKCwwNDg8QERITFBUWFxgZGhscHR4fIAAA'

PSBT_ELEMENTS_GLOBAL_SCALAR is the only PSBT/PSET field that contains
key data but no value data and so is the only field that currently hits
this special case.
Note the re-parsing of the correct serialization is covered by the existing
PSBT tests which process PSBTs containing scalars.
While the order of fields is not explicit in the PSBT/PSET specifications,
third parties need to be able to support both with a single implementation.
Keeping the PSBT fields in the same order makes this significantly
easier.
ASSET_COMMITMENT and ASSET were in the wrong order, which prevents
simply iterating the constants in simple parsers/writers.
As for outputs. ISSUANCE_VALUE_COMMITMENT and ISSUANCE_VALUE were
misordered.
This makes global handling forwards-compatible when new fields are
added, and un-breaks any PSET implementations that serialize
PSBT_ELEMENTS_GLOBAL_TX_MODIFIABLE (which Elements does not yet
implement).
This allows these test cases to be re-used by alternate implementations
for round-trip serialization testing.
I found this useful for for creating test cases and checking validity.
In the event that alternative implementations do not aim for exact
serilization compatibility, this RPC can be used to validate and
re-serialize their output for testing.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 3be6e80

@psgreco psgreco merged commit 6132c01 into ElementsProject:master Aug 9, 2022
gwillen added a commit to gwillen/elements that referenced this pull request Aug 9, 2022
@jgriffiths jgriffiths deleted the pset_updates branch August 9, 2022 21:06
psgreco added a commit that referenced this pull request Aug 9, 2022
[backport] Backport #1121 and #1131 to elements-22.x for rc3
gwillen added a commit to gwillen/elements that referenced this pull request Mar 2, 2023
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.

Pset output scalar decoding bug.
4 participants