Psbt hash preimages (again) by sanket1729 · Pull Request #478 · rust-bitcoin/rust-bitcoin

sanket1729 · 2020-09-11T22:36:31Z

The accidental merge of #465 was reverted by #477. This opens PR is cherry-pick of commits from #465 rebased to master.

dr-orlovsky · 2020-09-30T18:19:15Z

src/util/psbt/error.rs

+
+/// Support hash-preimages in psbt
+#[derive(Debug)]
+pub enum PsbtHash{


nit: According to rust API guidelines , we should not use something like psbt::PsbtHash, so I propose to name this enum HashKeys

Tbh, I find the current name way more intuitive.

I think we should keep the current name, and I think that the rust API guidelines should make an exception for the word Hash which is already overloaded a zillion times :P

So, I had to remove this enum completely in order to support a generic function because then I would need to somehow construct a concrete error type from a generic parameter.

So, now the question is having the macro worth having if get slightly detailed error types? Or should I use go String return type instead.(as implemented in the latest revision).

Edit: Or maybe there is some other way that I cannot think of.

dr-orlovsky · 2020-09-30T18:20:33Z

src/util/psbt/macros.rs

 }

+#[cfg_attr(rustfmt, rustfmt_skip)]
+macro_rules! impl_psbt_insert_hash_pair {


I think we have a need to write a derive crate for this library, since we have tooo many macros all around..
I has nothing to do with this specific PR, just a general thought

dr-orlovsky · 2020-09-30T18:22:01Z

One small naming nit, otherwise I'm happy with 81ebc67

sgeisler · 2020-09-30T20:00:33Z

src/util/psbt/error.rs

+
+/// Support hash-preimages in psbt
+#[derive(Debug)]
+pub enum PsbtHash{


Tbh, I find the current name way more intuitive.

src/util/psbt/macros.rs

sgeisler · 2020-09-30T20:04:46Z

src/util/psbt/macros.rs

+// macros for serde of hashes
+macro_rules! impl_psbt_hash_de_serialize {
+    ($thing:ty) => {
+        impl_psbt_hash_serialize!($thing);


thing is kinda generic.

src/util/psbt/map/input.rs

sgeisler · 2020-09-30T20:13:06Z

src/util/psbt/macros.rs

 }

+#[cfg_attr(rustfmt, rustfmt_skip)]
+macro_rules! impl_psbt_insert_hash_pair {


Why exactly is this a macro? Couldn't this be a generic function? If possible (without causing redundancy) I think we should avoid macros.

I agree. I think this can be turned into a generic function without any redundancy. Will do so

dr-orlovsky

utACK with only minor typos / nits

src/util/psbt/error.rs

src/util/psbt/map/input.rs

sanket1729 · 2020-10-07T23:46:13Z

Rebased an addressed the nits.

.gitignore

stevenroose · 2020-10-10T17:43:57Z

src/util/psbt/map/input.rs

I got a branch for this, but it's like 2 years old :)

stevenroose

Looks very good!

src/util/psbt/error.rs

src/util/psbt/map/input.rs

Added hash preimages to psbt as per updated bip174

sanket1729

Rebased to fix conflicts and addressed comments from @stevenroose .

stevenroose

LGTM!

apoelstra

ack d3b3ca8

dr-orlovsky previously approved these changes Sep 12, 2020

View reviewed changes

dr-orlovsky mentioned this pull request Sep 16, 2020

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

Closed

13 tasks

sanket1729 dismissed dr-orlovsky’s stale review via 81ebc67 September 30, 2020 14:33

sanket1729 force-pushed the psbt_again branch from d5dc1f0 to 81ebc67 Compare September 30, 2020 14:33

apoelstra previously approved these changes Sep 30, 2020

View reviewed changes

dr-orlovsky previously approved these changes Sep 30, 2020

View reviewed changes

sgeisler reviewed Sep 30, 2020

View reviewed changes

sanket1729 dismissed stale reviews from dr-orlovsky and apoelstra via 40769d1 September 30, 2020 21:40

sanket1729 force-pushed the psbt_again branch 4 times, most recently from c99918f to 096ee5f Compare September 30, 2020 22:41

sanket1729 mentioned this pull request Oct 1, 2020

Complete implementation of psbt Satisfier for hashlocks and timelocks rust-bitcoin/rust-miniscript#140

Closed

apoelstra previously approved these changes Oct 7, 2020

View reviewed changes

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

dr-orlovsky previously approved these changes Oct 7, 2020

View reviewed changes

src/util/psbt/error.rs Outdated Show resolved Hide resolved

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

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

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

sanket1729 dismissed stale reviews from dr-orlovsky and apoelstra via 9b444af October 7, 2020 23:40

sanket1729 force-pushed the psbt_again branch 2 times, most recently from 9b444af to c2f5743 Compare October 7, 2020 23:44

dr-orlovsky previously approved these changes Oct 8, 2020

View reviewed changes

stevenroose reviewed Oct 10, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

stevenroose reviewed Oct 10, 2020

View reviewed changes

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

Copy link
Copy Markdown

Collaborator

stevenroose Oct 10, 2020

Choose a reason for hiding this comment

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

I got a branch for this, but it's like 2 years old :)

stevenroose previously approved these changes Oct 10, 2020

View reviewed changes

src/util/psbt/error.rs Outdated Show resolved Hide resolved

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

dr-orlovsky mentioned this pull request Oct 11, 2020

PSBT proprietary key system matching BIP 174 #471

Merged

sanket1729 added 2 commits October 11, 2020 21:51

Added Hash preimages to psbt

48151ef

Added hash preimages to psbt as per updated bip174

Add tests for psbt hash serde

57b63d4

Removed Intellij idea files from .gitignore

d3b3ca8

sanket1729 commented Oct 12, 2020

View reviewed changes

sanket1729 dismissed stale reviews from stevenroose and dr-orlovsky via d3b3ca8 October 12, 2020 02:53

sanket1729 force-pushed the psbt_again branch from c2f5743 to d3b3ca8 Compare October 12, 2020 02:53

dr-orlovsky approved these changes Oct 12, 2020

View reviewed changes

stevenroose mentioned this pull request Oct 25, 2020

BIP155: AddrV2 #449

Merged

stevenroose approved these changes Oct 25, 2020

View reviewed changes

apoelstra approved these changes Nov 5, 2020

View reviewed changes

apoelstra merged commit 35d729d into rust-bitcoin:master Nov 5, 2020

apoelstra mentioned this pull request Nov 6, 2020

Fixing serde impl for PSBT inputs #512

Merged

sanket1729 mentioned this pull request Nov 19, 2020

Errors enum improvements #521

Merged

Conversation

sanket1729 commented Sep 11, 2020

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!

sanket1729 Sep 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!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 commented Oct 7, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sanket1729 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!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sanket1729 Sep 30, 2020 •

edited

Loading