New PSBT global keys by dr-orlovsky · Pull Request #499 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2020-10-11T03:11:08Z

Based/depends on #470

Part of epic #473

stevenroose

You also forgot to implement Map::get_pairs to do the serialization part.

Some unit tests that add some roundtrips of some PSBTs with the new keys would also be appreciated.

src/util/psbt/map/global.rs

apoelstra · 2020-11-05T19:21:08Z

needs rebase

dr-orlovsky · 2020-11-06T16:30:23Z

Rebased and addressed @stevenroose comments. NB: this is based on #470, which also had review comments (they are addressed now, but #470 needs to be merged first)

src/util/bip32.rs

sanket1729 · 2020-10-29T10:09:03Z

src/util/bip32.rs

nit: has wrong length

sanket1729 · 2020-11-06T17:35:51Z

src/util/bip32.rs

The BIP only specifies versions for Testnet, is it safe to use the same for Signet and Regtest

I think it's reasonable for now. Regtest copies a lot of values from testnet.

sanket1729 · 2020-11-06T17:45:21Z

src/util/bip32.rs

Can the from_slice() only fail if size is incorrect?
It looks like https://github.com/bitcoin-core/secp256k1/blob/9e5939d2848df5508e3a6eb038273b28d255248c/src/secp256k1.c#L550-L559 can fail if there is overflow or if secret key is zero.

Fixed in the other branch and now fine after the rebase

apoelstra · 2020-11-24T23:21:23Z

Can you squash these commits together? It's not too long to read, and it's a bit harder to review because you make changes that are later modified.

src/util/psbt/map/global.rs

justinmoon · 2020-11-25T20:09:06Z

I think this breaks PSBT serialization.

The impl_psbtmap_consensus_encoding macro calls Global::get_pairs which needs to be updated after you removed version and xpub values from Global::unknown and put them in their own fields.

Here's a failing test.

Otherwise it looks great!

dr-orlovsky · 2020-11-28T12:22:59Z

@justinmoon you are right, and even more: we need to do a proper merge according to BIP174 rules and resolve conflicts. Working on the change

dr-orlovsky · 2020-11-28T13:46:49Z

@apoelstra squashed old commits; new commits are on top and they do not change previous code but splits added functionality into logical blocks for review simplification

In general, it was not that trivial to do the merge procedure and key pair export; there were two questions which I do not know the right answer for, but proposing what I think will work the best:

What to do when two PSBTs global xpub match by value, but not derivation/key source? Spec says that it's up to an implementation to decide, so we have to decide :)
Do we need to serialize default 0 version, or just ignore this case? If we will serialize, this will break test vectors; if not, it may break roundtrips if the original PSBT contained explicit 0-version global key

My current implementation:

Uses special multi-step algorithm described here: 8b672c3#diff-2581ef26b841ed4069017f0e46e1393216bf6b413302ae34f248105526c8755bR137-R164
Does not serialize default 0 version as a separate key.

apoelstra · 2020-11-28T14:11:24Z

I'm happy with what you've done. I would've gone even simpler and just taken the lexicograpically first one :) but you're right that probably "longest first" is likely to do the right thing in realistic conflict scenarios.
Agreed, we'll just break round-tripping (and will have to add extra canonicalization to our fuzztests)

justinmoon · 2020-11-28T20:10:32Z

src/util/psbt/map/global.rs

This will cause runtime error. ret is a vec with length 0, so you can't use vec[0..4]

Very good point, thank you. Fixed

stevenroose · 2020-12-02T22:12:25Z

You seem to have some Rust v1.29 compilation error in the ordering algorithm.

dr-orlovsky · 2020-12-05T13:52:07Z

@stevenroose @apoelstra 1.29 fixed for all commit history. It's extremely stupid with borrow checks and making it compiling required an unnecessary clone; I have tried everything else and it simply was not working with 1.29 :(

src/util/psbt/map/global.rs

apoelstra · 2020-12-07T18:53:34Z

src/util/psbt/map/global.rs

nit: the correct term is "commutative"

apoelstra

ack 794c5196a0eb4ec635a35cf09708ff8da6a00d28

apoelstra

ack 794c5196a0eb4ec635a35cf09708ff8da6a00d28

stevenroose

utACK d1ace733e0de532707df7140ee5be1eb3faedbe0

stevenroose · 2020-12-15T14:23:39Z

src/util/psbt/map/global.rs

Largest here means just by ordering the bytes, right? So 0xdeadbeef wins from 0xbeadbeef? This is a bit arbitrary. But I suppose I can agree that it's better to do something then to error.

Correct. Actually since fingerprint is [u8; 4] and not u32 I assume it uses little-endian comparison. And yes, this is arbitrary

nit: I'd perhaps rephrase "choose largest pingerprint using binary ordering" because this suggests that the fingerprint "size" might have real meaning :p

stevenroose · 2020-12-15T14:33:46Z

@apoelstra you "approved" a different commit than the hash you mention inside the approve message. Can you confirm you meant to ack d1ace73?

stevenroose

Left a bunch of nits. I didn't want to make them to not hold up the MR, but since your CI fails and you need to push changes anyway, I thought I'd leave them anyway.

stevenroose · 2020-12-15T14:34:35Z

src/util/bip32.rs

nit: There's no need to do this. It's probably microscopically more efficient to leave it self.

This is not about efficiency, this is required by compiler, otherwise we get

error[E0631]: type mismatch in function arguments --> src/util/psbt/map/global.rs:182:71 | 182 | (fp, deriv.len(), deriv.into_iter().rposition(ChildNumber::is_normal)) | ^^^^^^^^^^^^^^^^^^^^^^ expected signature of `fn(&ChildNumber) -> _` | ::: src/util/bip32.rs:129:5 | 129 | pub fn is_normal(self) -> bool { | ------------------------------ found signature of `fn(ChildNumber) -> _`

basically you need &self in order to be able to use the functions inside the iterators

you can just add .cloned() on the iterator before .rposition. Or .copied() in Rust 1.36. But what you did here is fine.

stevenroose · 2020-12-15T14:34:40Z

src/util/bip32.rs

nit: There's no need to do this. It's probably microscopically more efficient to leave it self.

Same as above

stevenroose · 2020-12-15T14:36:33Z

src/util/psbt/map/global.rs

this can be for n in &derivation without the index. Or even

derivation.for_each(|n| ret.extend(&u32_to_array_le(n.into())));

No, it can't be: there is no iter() for Derivation. You can only use derivation.into_iter()

stevenroose · 2020-12-15T14:37:50Z

src/util/psbt/map/global.rs

nit: I think cmp::max(self.version, other.version) reads better; but very much a nit :)

stevenroose · 2020-12-15T14:38:43Z

src/util/psbt/map/global.rs

nit: I'd perhaps rephrase "choose largest pingerprint using binary ordering" because this suggests that the fingerprint "size" might have real meaning :p

stevenroose · 2020-12-15T14:39:11Z

src/util/psbt/map/global.rs

nit: Put on same line as string as long as it doesn't exceed 100 width :)

stevenroose · 2020-12-15T14:40:14Z

src/util/psbt/map/global.rs

nit: This exceeds 100 characters, right? how about putting the encode::Error::ParseFailed on the same line as .map_err?

stevenroose · 2020-12-15T14:41:12Z

src/util/psbt/map/global.rs

nit: I think you can actually do

let mut fingerprint = Fingerprint::default(); decoder.read_exact(&mut fingerprint[..])?;

but I'm not 100% sure

You can't: there is no IndexMut impl for Fingerprint

Plus necessary changes to BIP 32 implementations and error type

Conflicts: src/util/psbt/map/global.rs

dr-orlovsky · 2020-12-15T15:08:49Z

Had to rebase since it had conflicts with merged #530 and had a failed fuzz tests. Also used this opportunity to address @stevenroose nits. Will need a re-ACK from you guys, @apoelstra, @stevenroose

src/util/psbt/map/global.rs

sgeisler

I think by and large this PR is in a good state. I'm still not super happy with the merge logic, it seems way too convoluted, reasoning about it was somewhat hard and I'm not sure it's entirely consistent. But since many reviewed so far and didn't take issue with it I'd be fine with it since the BIP didn't specify to detect such inconsistencies afaik. @apoelstra, what do you think, keep polishing or merge?

src/util/psbt/map/global.rs

sgeisler

I found some logic bugs which is probably a good indicator to add some tests for the newly added logic.

src/util/psbt/map/global.rs

apoelstra

ack 7f5c279

dr-orlovsky mentioned this pull request Oct 11, 2020

[epic] PSBT and related BIP 32 refactoring tracking issue #473

Closed

13 tasks

apoelstra added the API break This PR requires a version bump for the next release label Oct 14, 2020

stevenroose requested changes Oct 25, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

dr-orlovsky requested a review from stevenroose November 6, 2020 17:08

sanket1729 reviewed Nov 6, 2020

View reviewed changes

justinmoon mentioned this pull request Nov 16, 2020

Handle PSBT_GLOBAL_XPUB in PSBT global scope #519

Closed

dr-orlovsky requested a review from apoelstra November 23, 2020 20:41

justinmoon reviewed Nov 25, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

justinmoon reviewed Nov 28, 2020

View reviewed changes

apoelstra reviewed Dec 5, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

dr-orlovsky requested a review from apoelstra December 7, 2020 15:42

apoelstra reviewed Dec 7, 2020

View reviewed changes

dr-orlovsky requested a review from apoelstra December 7, 2020 19:04

apoelstra reviewed Dec 7, 2020

View reviewed changes

apoelstra previously approved these changes Dec 7, 2020

View reviewed changes

stevenroose previously approved these changes Dec 15, 2020

View reviewed changes

stevenroose reviewed Dec 15, 2020

View reviewed changes

dr-orlovsky added 3 commits December 15, 2020 15:47

PSBT: adding global types (version, xpub)

af61d7e

PSBT: merging new global keys

2f83821

Plus necessary changes to BIP 32 implementations and error type

PSBT: Key pair serialization for new global keys

df8635c

Conflicts: src/util/psbt/map/global.rs

dr-orlovsky added 2 commits December 15, 2020 15:50

Improving PSBT merge with dedicated 1.29 rustc borrow scope

0235abf

Nits in new PSBT global types reviews

8b16662

dr-orlovsky dismissed stale reviews from stevenroose and apoelstra via 8b16662 December 15, 2020 15:04

sgeisler suggested changes Dec 15, 2020

View reviewed changes

dr-orlovsky added 2 commits December 16, 2020 16:39

BSPT: Improving global xpub merging algorithm

21c11e3

PSBT: Improved global keys version and xpub handling

b84faa7

dr-orlovsky requested review from apoelstra, sgeisler and stevenroose December 16, 2020 15:48

sgeisler reviewed Dec 16, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

sgeisler suggested changes Dec 19, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

sgeisler reviewed Dec 19, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

sgeisler reviewed Dec 19, 2020

View reviewed changes

src/util/psbt/map/global.rs Outdated Show resolved Hide resolved

PSBT global xpub merging algorithm reworked

7f5c279

sgeisler approved these changes Dec 20, 2020

View reviewed changes

apoelstra approved these changes Dec 20, 2020

View reviewed changes

apoelstra merged commit a6264cf into rust-bitcoin:master Dec 20, 2020

sgeisler mentioned this pull request Mar 11, 2021

Support for Tor addresses #421

Closed

apoelstra mentioned this pull request Nov 15, 2024

(New) fuzz failure in bitcoin_deserialize_psbt #3628

Closed

Conversation

dr-orlovsky commented Oct 11, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra commented Nov 5, 2020

Uh oh!

dr-orlovsky commented Nov 6, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Nov 24, 2020

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

justinmoon commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky commented Nov 28, 2020

Uh oh!

dr-orlovsky commented Nov 28, 2020

Uh oh!

apoelstra commented Nov 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Dec 2, 2020

Uh oh!

dr-orlovsky commented Dec 5, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Dec 15, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmoon commented Nov 25, 2020 •

edited

Loading

dr-orlovsky Nov 30, 2020 •

edited

Loading

dr-orlovsky Dec 15, 2020 •

edited

Loading