Blinding API cleanups #143

sanket1729 · 2022-08-03T22:49:40Z

This required the user to explicitly maintain whether the current TxIn
has issuance or not whereas it can directly be calculated from
assetIssuance field

The blinding APIs are separated into smaller chunks. This allows
- Blinding only the values or the assets but not both
- Not relying on rng to set asset/value blinding factors
- Better input parameters

Note I had to edit the test vectors the order in which they were sampled from rng had changed.

apoelstra · 2022-08-25T14:46:08Z

Should I review this? I have been holding off because of the sea of red in CI.

sanket1729 · 2022-08-30T20:42:41Z

The downstream options project is stable enough. I can rewive this PR today.

sanket1729 · 2022-09-30T11:43:15Z

Cherry-picked the MSRV commit here. This might not be an easy review, but I would really like this to go into the next release.

apoelstra · 2022-09-30T16:30:46Z

Will need rebase after #147 (and then I think we'll no longer need the MSRV bump, though we will need a CHANGELOG update). But I'll review this as-is.

apoelstra · 2022-09-30T16:47:50Z

src/pset/map/input.rs

+            AssetId::generate_asset_entropy(prevout, contract_hash)
+        } else {
+            // re-issuance
+            sha256::Midstate::from_inner(self.issuance_asset_entropy.unwrap_or([0u8; 32]))


In 8b41c13:

Should use unwrap_or_default here like above (or change both to unwrap_or([8; 32]); having two ways to write the same thing makes it look like these do different things, which is confusing.

Still present in 04ad35f

Addressed in fa753b4

apoelstra · 2022-09-30T16:49:13Z

src/pset/mod.rs

            .collect::<Vec<_>>();

        Ok((inp_secrets, blind_out_indices))
    }

+    /// Obtains the surjection inputs for this pset. This servers as the domain


In 8b41c13:

Typo servers should be serves.

I also wonder if you should define "domain" ... e.g. write domain (set of assets on the "input side") since this term isn't really used elsewhere, and it isn't obvious unless you're pretty familiar with the word "surjection".

Still present in 04ad35f

Addressed in fa753b4

src/blind.rs

apoelstra

ACK 49d1ec2

apoelstra · 2022-09-30T17:33:42Z

All my comments are nits which could be addressed with additional commits or even in another PR.

Also, can you add a commit which runs cargo fmt?

sanket1729 · 2022-09-30T20:22:36Z

Also, can you add a commit which runs cargo fmt?

@apoelstra, do you mean on the entire codebase

apoelstra · 2022-10-01T13:24:33Z

@sanket1729 let's say, just on blind.rs and psbt/mod.rs which are otherwise heavily modified in this PR (and there are a couple format nits I didn't want to write out in those files).

sanket1729 · 2022-10-01T18:15:06Z

@apoelstra, any interest in reviving #80? We have enforced rustfmt across rust-bitcoin org in many repos.

sanket1729 · 2022-10-01T18:26:19Z

Addressed the comments. Will rebase after we have #147 in

sanket1729 · 2022-10-01T18:55:22Z

Also added a test to test tx verification with partially blinded transactions

apoelstra

ACK 073e720

This required the user to explicitly maintain whether the current TxIn has issuance or not whereas it can directly be calculated from assetIssuance field

Now the blinding APIs are separated into smaller chunks. This allows 1) Blinding only the values or the assets but not both 2) Not relying on rng to set asset/value blinding factors 3) Bunch of API cleanups with better input parameters

Issuances are still not blinded in pset. We can add another parameter to Pset::blind_last and Pset::blind_non_last, but it is add another parameter to the API. We can implement this in future if required.

This was only used in issuance blinding nonce, so was never really tested

Should be 0x0a not 0x10 :)

While the positions of inputs and outputs don't usually matter, they do matter in covenant constructions. This allows easier construction rather than creating a new vec or swapping things around

apoelstra

ACK 582325e

sanket1729 force-pushed the blind_cleanups branch 5 times, most recently from 648e22e to 0584ed1 Compare August 9, 2022 23:40

sanket1729 force-pushed the blind_cleanups branch from 05c8446 to 0584ed1 Compare August 17, 2022 20:54

sanket1729 force-pushed the blind_cleanups branch 3 times, most recently from c082ee9 to 157e4ef Compare September 30, 2022 11:41

sanket1729 force-pushed the blind_cleanups branch from 157e4ef to 49d1ec2 Compare September 30, 2022 11:48

apoelstra reviewed Sep 30, 2022

View reviewed changes

src/blind.rs Outdated Show resolved Hide resolved

apoelstra reviewed Sep 30, 2022

View reviewed changes

src/blind.rs Outdated Show resolved Hide resolved

apoelstra approved these changes Sep 30, 2022

View reviewed changes

sanket1729 force-pushed the blind_cleanups branch from 49d1ec2 to 64eea2a Compare October 1, 2022 18:25

sanket1729 force-pushed the blind_cleanups branch from 64eea2a to 552368f Compare October 1, 2022 18:54

sanket1729 force-pushed the blind_cleanups branch from 552368f to 073e720 Compare October 1, 2022 19:10

apoelstra approved these changes Oct 3, 2022

View reviewed changes

Remove public has_issuance field from TxIn

d521aee

This required the user to explicitly maintain whether the current TxIn has issuance or not whereas it can directly be calculated from assetIssuance field

sanket1729 force-pushed the blind_cleanups branch from 073e720 to b53e51b Compare October 5, 2022 09:03

sanket1729 added 8 commits October 5, 2022 02:08

Cleanup bunch of blinding APIs

fa753b4

Now the blinding APIs are separated into smaller chunks. This allows 1) Blinding only the values or the assets but not both 2) Not relying on rng to set asset/value blinding factors 3) Bunch of API cleanups with better input parameters

Add support for issuance

3107c25

Issuances are still not blinded in pset. We can add another parameter to Pset::blind_last and Pset::blind_non_last, but it is add another parameter to the API. We can implement this in future if required.

Fix pset Tweak serde

2815e52

This was only used in issuance blinding nonce, so was never really tested

Fix pset key bug

3583ae2

Should be 0x0a not 0x10 :)

Add issuance surjection proof verification

cbc2d56

Allow inserting inputs/outputs at specified positions

2908bc1

While the positions of inputs and outputs don't usually matter, they do matter in covenant constructions. This allows easier construction rather than creating a new vec or swapping things around

Add liquid testnet parameters

759d301

rustfmt: add src/pset/mod.rs and src/blind.rs

582325e

sanket1729 force-pushed the blind_cleanups branch from b53e51b to 582325e Compare October 5, 2022 09:09

apoelstra approved these changes Oct 5, 2022

View reviewed changes

apoelstra merged commit 55b6cce into ElementsProject:master Oct 5, 2022

RCasatta mentioned this pull request Oct 6, 2022

release version 0.20.0 #151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blinding API cleanups #143

Blinding API cleanups #143

sanket1729 commented Aug 3, 2022 •

edited

Loading

apoelstra commented Aug 25, 2022

sanket1729 commented Aug 30, 2022

sanket1729 commented Sep 30, 2022

apoelstra commented Sep 30, 2022

apoelstra Sep 30, 2022

apoelstra Oct 3, 2022

sanket1729 Oct 5, 2022 •

edited

Loading

apoelstra Sep 30, 2022

apoelstra Oct 3, 2022

sanket1729 Oct 5, 2022 •

edited

Loading

apoelstra left a comment

apoelstra commented Sep 30, 2022

sanket1729 commented Sep 30, 2022

apoelstra commented Oct 1, 2022

sanket1729 commented Oct 1, 2022

sanket1729 commented Oct 1, 2022 •

edited

Loading

sanket1729 commented Oct 1, 2022

apoelstra left a comment

apoelstra left a comment

Blinding API cleanups #143

Blinding API cleanups #143

Conversation

sanket1729 commented Aug 3, 2022 • edited Loading

apoelstra commented Aug 25, 2022

sanket1729 commented Aug 30, 2022

sanket1729 commented Sep 30, 2022

apoelstra commented Sep 30, 2022

apoelstra Sep 30, 2022

Choose a reason for hiding this comment

apoelstra Oct 3, 2022

Choose a reason for hiding this comment

sanket1729 Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

apoelstra Sep 30, 2022

Choose a reason for hiding this comment

apoelstra Oct 3, 2022

Choose a reason for hiding this comment

sanket1729 Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 30, 2022

sanket1729 commented Sep 30, 2022

apoelstra commented Oct 1, 2022

sanket1729 commented Oct 1, 2022

sanket1729 commented Oct 1, 2022 • edited Loading

sanket1729 commented Oct 1, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Aug 3, 2022 •

edited

Loading

sanket1729 Oct 5, 2022 •

edited

Loading

sanket1729 Oct 5, 2022 •

edited

Loading

sanket1729 commented Oct 1, 2022 •

edited

Loading