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

PSBT lacking Eq implementation #931

Closed
dr-orlovsky opened this issue Apr 1, 2022 · 5 comments · Fixed by #932
Closed

PSBT lacking Eq implementation #931

dr-orlovsky opened this issue Apr 1, 2022 · 5 comments · Fixed by #932
Assignees
Labels
Milestone

Comments

@dr-orlovsky
Copy link
Collaborator

PartiallySignedTransaction, psbt::Output and psbt::Input all lacking Eq implementation (but have PartialEq). Was this intentional?

If not, I see two ways forward with the implementation: adding derive or manual implementation doing BIP-174 serialization to a vector and byte-comparing those vectors.

@dr-orlovsky dr-orlovsky added this to the 0.29.0 milestone Apr 1, 2022
@dr-orlovsky dr-orlovsky self-assigned this Apr 1, 2022
@dr-orlovsky dr-orlovsky added this to To do in PSBT via automation Apr 1, 2022
@dr-orlovsky dr-orlovsky changed the title PSBT Eq implementation PSBT lacking Eq implementation Apr 1, 2022
@dr-orlovsky dr-orlovsky added this to In Progress in Derive unification Apr 1, 2022
@dr-orlovsky dr-orlovsky moved this from To do to In progress in PSBT Apr 1, 2022
@sanket1729
Copy link
Member

If not, I see two ways forward with the implementation: adding derive or manual implementation doing BIP-174 serialization to a vector and byte-comparing those vectors.

Same Psbts can have different serializations because the order in which we serialize keys is not defined. Even though the serialization in rust-bitcoin is deterministic, other implementations need not follow the same. I think we have to derive it on the Psbt data structure.

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 derived in #932

@apoelstra
Copy link
Member

I'm not sure how to interpret Sanket's comment, but we should implemnet Eq everywhere here. This looks like an oversight.

@sanket1729
Copy link
Member

@apoelstra, I think we are on the same page. There were two ways to derive Eq.

  • [#derive(Eq)]
  • Compare the consensus encoding.

I suggested using [#derive(Eq)] because the same Psbt structures need to necessarily have the same consensus encoding(key-value pairs can be serialized in any order).

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 I think @apoelstra meant that we need to introduce support for hidden branches into PSBT.

Derive unification automation moved this from In Progress to Done Apr 1, 2022
PSBT automation moved this from In progress to Done Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
PSBT
Done
Development

Successfully merging a pull request may close this issue.

3 participants