Skip to content

PSBT proprietary key system matching BIP 174#471

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
BP-WG:feat/psbt-keytypes
Dec 21, 2020
Merged

PSBT proprietary key system matching BIP 174#471
apoelstra merged 2 commits intorust-bitcoin:masterfrom
BP-WG:feat/psbt-keytypes

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky commented Sep 10, 2020

This is a companion PR to #478 and a part of epic #473

This PR is based on previous PRs for BIP 32 binary serialization and versioning from #469 and #470 (which must be merged first) and includes commits from them. Commits specific to this PR starts from 325f323

Once #478 will be merged, I will add constant key name for the new hash key types into this PR

@stevenroose
Copy link
Copy Markdown
Collaborator

Again, could this perhaps be rebased on master?

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

@stevenroose done

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

Build fails b/c something strange with fuzztests, can't understand what's wrong

Copy link
Copy Markdown
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

I think the fuzz failure is due to the bug that I pointed out in the ProprietaryKey type.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will not roundtrip. I think if you want a default type, it should be pub struct ProprietaryType(u8); so that you can roundtrip the actual byte.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Alternatively, you could make ProprietaryKeyType a marker trait:

pub trait ProprietaryTypeKey: Copy + From<u8> + Into<u8> {}

But the plethora of naming will soon become confusing (ProprietaryType, ProprietaryTypeKey, ProprietaryKey).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If we would not use generic here this will allow using of conflicting proprietary key type systems, which is undesirable

@dr-orlovsky dr-orlovsky changed the title Updating PSBT key types to match recent BIP 174 changes PSBT proprietary key system matching BIP 174 Dec 21, 2020
@dr-orlovsky dr-orlovsky mentioned this pull request Dec 21, 2020
},
version: 0,
xpub: Default::default(),
proprietary: Default::default(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: You have mixed usage of Default::default() and BTreeMap::new() in this file.

Copy link
Copy Markdown
Collaborator

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
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 7400bcc

@apoelstra apoelstra merged commit 3c11173 into rust-bitcoin:master Dec 21, 2020
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