Model Value, Asset and Nonce confidential variants using secp256k1-zkp types #77

luckysori · 2021-04-06T03:57:21Z

This is the part of PR #70 which should be less controversial.

When splitting up that PR and going through @sanket1729's review I noticed that we had broken the serde implementations of Value, Asset and Nonce (and I found this (our) bug in rust-secp256k1-zkp).

This also made me realise that, by delegating to said library when implementing serde on our types, we had introduced breaking changes. I've documented the original serde implementations with the tests in 1c53b17, to be able to highlight the changes introduced in 516ac0e. I made some choices there which could be questionable, so please take a look.

Once BlockstreamResearch/rust-secp256k1-zkp#10 is merged, we should remove the patch dependency to luckysori/rust-secp256k1-zkp in Cargo.toml.

thomaseizinger

Some nits, mainly concerning patch history :)

Thanks for pushing the upstream forward!

thomaseizinger · 2021-04-06T05:41:20Z

src/confidential.rs

-                Token::Tuple { len: 32 },
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::U8(1), Token::U8(1), Token::U8(1), Token::U8(1),
-                Token::TupleEnd,


So this used to serialize as a tuple? Very interesting! I guess for a serializer impl like JSON, it doesn't make a difference because JSON doesn't have support for tuples?

thomaseizinger · 2021-04-06T05:42:18Z

src/encode.rs

+            } => write!(
+                f,
+                "oversized vector allocation: requested {}, maximum {}",
+                r, m
+            ),


Nit: This is just a format change, not sure if it is wanted / tolerated?

thomaseizinger · 2021-04-06T05:43:04Z

src/transaction.rs

@@ -1661,7 +1660,7 @@ mod tests {

        // Output with pushes that are e.g. OP_1 are nulldata but not pegouts
        let output: TxOut = hex_deserialize!("\
-            0a2d3634393536d9a2d0aaba3823f442fb24363831fdfd0101010101010101010\
+            0a319c0000000000d3d3d3d3d3d3d3d3d3d3d3d3fdfdfd0101010101010101010\


Do you remember why this changed?

The test was incorrect. The asset is not a correct curve point

thomaseizinger · 2021-04-06T05:46:22Z

src/transaction.rs

 pub enum ConfidentialTxOutError {
    /// The address provided does not have a blinding key.
    NoBlindingKeyInAddress,
    /// Error originated in `secp256k1_zkp`.
    Upstream(secp256k1_zkp::Error),
 }

+impl fmt::Display for ConfidentialTxOutError {


Can we squash this into the one where it is introduced?

thomaseizinger · 2021-04-06T05:46:48Z

src/transaction.rs

@@ -404,6 +404,11 @@ impl std::error::Error for ConfidentialTxOutError {
    }
 }

+impl From<secp256k1_zkp::Error> for ConfidentialTxOutError {


Also a candidate for squashing I'd say.

thomaseizinger · 2021-04-06T05:48:23Z

src/hash_types.rs

@@ -55,9 +52,3 @@ impl_hashencode!(Wtxid);
 impl_hashencode!(SigHash);
 impl_hashencode!(BlockHash);
 impl_hashencode!(TxMerkleNode);
-
-impl ThirtyTwoByteHash for SigHash {


If this is deleted here, we should just drop the previous commit.

I must have messed something up, because this change was desired. I'll fix it.

thomaseizinger · 2021-04-06T05:48:48Z

src/script.rs

@@ -1158,8 +1158,6 @@ mod test {
        assert!(script_2 < script_3);
        assert!(script_3 < script_4);

-        assert_eq!(script_1, script_1);


sanket1729

Overall, I think this looks good, left some minor code style nits. It would be great to also incorporate suggestions which Thomas Eizenger suggested.

I am not sure about the serde breaking change. I will let @apoelstra comment on it.

sanket1729 · 2021-04-06T20:52:33Z

src/confidential.rs

+                seq.serialize_element(&n)?;
+            }
+            Asset::Confidential(commitment) => {
+                seq.serialize_element(&2u8)?;


@apoelstra curious what you think about this serde breaking change.

sanket1729 · 2021-04-06T21:02:21Z

src/transaction.rs

@@ -1661,7 +1660,7 @@ mod tests {

        // Output with pushes that are e.g. OP_1 are nulldata but not pegouts
        let output: TxOut = hex_deserialize!("\
-            0a2d3634393536d9a2d0aaba3823f442fb24363831fdfd0101010101010101010\
+            0a319c0000000000d3d3d3d3d3d3d3d3d3d3d3d3fdfdfd0101010101010101010\


The test was incorrect. The asset is not a correct curve point

sanket1729 · 2021-04-06T21:24:23Z

src/script.rs

@@ -1147,7 +1147,7 @@ mod test {
        assert_eq!(v_nonmin_alt.unwrap(), slop_v_nonmin_alt.unwrap());
    }

-	#[test]
+        #[test]


This is bugging me because locally I don't have this change D:

Make sure you are diffing against current master. Maybe another rebase helps!

sanket1729 · 2021-04-06T21:26:08Z

src/script.rs

@@ -1158,8 +1158,7 @@ mod test {
        assert!(script_2 < script_3);
        assert!(script_3 < script_4);

-        assert!(script_1 <= script_1);


These lines should not be deleted

Just curious, what's this supposed to catch? Doesn't the compiler enforce that script_1 is equal to script_1? I've replaced it with assert_eq, but maybe you actually wanted the original assertions. Let me know.

I think the test is script is for script_ord which is supposed to check basic comparison orderings like script_1 >= script_1. Even if it tests something that is trivial, it should not be removed as a part of this PR.

I've reverted it back to the original state.

luckysori · 2021-04-07T00:48:11Z

Squashed and I tried to deal with all the nits. Maybe someone else can deal with the silly indent in script.rs line 1150, because it's not showing up on my machine 🤷

apoelstra · 2021-04-13T20:53:38Z

Sorry for the long delay in reviewing.

concept ACK everything, including the breaking serde changes. (I'm not thrilled about using a sequence of (tag, bytes) like this, but I like my original scheme even less .... would prefer if we serialized as a flat sequence of bytes or a hex string, similar to what PublicKey et al do).

There are two problems blocking this now:

The unit tests fail with cargo test --features="serde-feature" as of 1c53b17
The cargo patch is still in place as of 285f565 ... and this doesn't compile anymore since the branch has been deleted!

Unsure why CI is not catching the unit test failure :( does anyone else see this?

sanket1729 · 2021-04-13T21:22:19Z

@apoelstra , I think the failure is related to deleted branch. I have the branch saved locally, so all commits in the PR compile and test with serde-feature correctly.

The branch was probably not deleted when the CI ran, so the CI did not detect the failure

sanket1729 · 2021-04-13T21:33:14Z

Looks like the branch was merged into rust-secp-zkp master, that is why it was deleted.

apoelstra · 2021-04-13T21:49:11Z

Right, sorry, I forgot that just merging rust-secp 10 wasn't enough. We also need to do a crates.io release :). We'll try to do that ASAP so we can move forward on this.

sanket1729 · 2021-04-13T22:17:47Z

rust-secp-zkp 0.2.1 has been released. So, we don't need any patch dependencies :)

apoelstra · 2021-04-13T22:18:58Z

Confirmed that a modified version of this PR with the [patch] removed now passes (unit tests pass on every commit)

apoelstra · 2021-04-13T23:16:37Z

Sorta-ack 7a248e0

This PR looks great! Thanks so much for this work, especially doing the fiddly computations for last vs not-last TxOuts. If you can remove the [patch] from Cargo.toml I'm happy to merge this.

To document the current serialization format and highlight future breaking changes. Co-authored-by: Thomas Eizinger <[email protected]>

This allows us to peek at the next value in the buffer without consuming it, which is useful when a prefix identifies a type but is also part of the data itself e.g. value commitments, asset generators, nonces. Co-authored-by: Thomas Eizinger <[email protected]>

This patch introduces a _breaking_ change in the (de)serialisation of confidential assets, confidential values and confidential nonces. In particular, we now support both readable and compact serialization, and they both include a serialization prefix `2` as opposed to the old prefix which is now part of the inner condifential value, since that is controlled by secp256k1-zkp. We choose the number `2` since we were already using `0` for null values and `1` for explicit values. We also had to modify some of the test data now that `secp256k1-zkp` is validating for us that the confidential assets are actually valid generators. Co-authored-by: Thomas Eizinger <[email protected]>

Co-authored-by: Thomas Eizinger <[email protected]>

A sighash fulfills the requirements of the `ThirtyTwoByteHash` trait and implementing it can be leveraged to convert a sighash into a `secp256k1::Message` for signing. Co-authored-by: Thomas Eizinger <[email protected]>

Co-authored-by: Thomas Eizinger <[email protected]>

luckysori · 2021-04-14T09:22:59Z

@apoelstra

Sorta-ack 7a248e0

I actually noticed that I had set RANGEPROOF_MIN_VALUE to 0 instead of 1. Maybe it's not a problem, but I've changed it back to 1 (see 38e89fb).

This PR looks great! Thanks so much for this work, especially doing the fiddly computations for last vs not-last TxOuts. If you can remove the [patch] from Cargo.toml I'm happy to merge this.

Thank you! I've now removed the patch dependency from all commits.

apoelstra

ack 38e89fb

This was referenced Apr 6, 2021

Allow unblinding of confidential TxOuts #78

Merged

Model confidential and explicit transaction outputs separately #70

Closed

thomaseizinger reviewed Apr 6, 2021

View reviewed changes

sanket1729 reviewed Apr 6, 2021

View reviewed changes

luckysori force-pushed the zkp-types branch from 115dfcb to c1bd344 Compare April 7, 2021 00:45

luckysori force-pushed the zkp-types branch from c1bd344 to 7a248e0 Compare April 9, 2021 03:55

luckysori force-pushed the zkp-types branch from 7a248e0 to b1a5768 Compare April 14, 2021 09:17

luckysori and others added 7 commits April 14, 2021 19:19

Add serde tests for Value, Asset and Nonce

c4929c8

To document the current serialization format and highlight future breaking changes. Co-authored-by: Thomas Eizinger <[email protected]>

Appease Clippy

acc702c

Co-authored-by: Thomas Eizinger <[email protected]>

Facilitate construction of transaction outputs

859a140

Co-authored-by: Thomas Eizinger <[email protected]>

Implement ThirtyTwoByteHash for SigHash

afb18df

A sighash fulfills the requirements of the `ThirtyTwoByteHash` trait and implementing it can be leveraged to convert a sighash into a `secp256k1::Message` for signing. Co-authored-by: Thomas Eizinger <[email protected]>

Extract private rangeproof arguments into constants

38e89fb

Co-authored-by: Thomas Eizinger <[email protected]>

luckysori force-pushed the zkp-types branch from b1a5768 to 38e89fb Compare April 14, 2021 09:19

apoelstra approved these changes Apr 19, 2021

View reviewed changes

apoelstra merged commit 9fd8dfe into ElementsProject:master Apr 19, 2021

sanket1729 mentioned this pull request Apr 21, 2021

Handle Result returns for engines #76

Merged

stevenroose mentioned this pull request Nov 25, 2021

Incompatible Value binary serialization format #105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model Value, Asset and Nonce confidential variants using secp256k1-zkp types #77

Model Value, Asset and Nonce confidential variants using secp256k1-zkp types #77

luckysori commented Apr 6, 2021 •

edited

Loading

thomaseizinger left a comment

thomaseizinger Apr 6, 2021

thomaseizinger Apr 6, 2021

thomaseizinger Apr 6, 2021

sanket1729 Apr 6, 2021

thomaseizinger Apr 6, 2021

thomaseizinger Apr 6, 2021

thomaseizinger Apr 6, 2021

luckysori Apr 7, 2021

thomaseizinger Apr 6, 2021

sanket1729 left a comment

sanket1729 Apr 6, 2021

sanket1729 Apr 6, 2021

sanket1729 Apr 6, 2021

luckysori Apr 7, 2021

thomaseizinger Apr 7, 2021

sanket1729 Apr 6, 2021

luckysori Apr 7, 2021

sanket1729 Apr 8, 2021

luckysori Apr 9, 2021

luckysori commented Apr 7, 2021

apoelstra commented Apr 13, 2021

sanket1729 commented Apr 13, 2021 •

edited

Loading

sanket1729 commented Apr 13, 2021

apoelstra commented Apr 13, 2021

sanket1729 commented Apr 13, 2021

apoelstra commented Apr 13, 2021

apoelstra commented Apr 13, 2021

luckysori commented Apr 14, 2021

apoelstra left a comment

Model Value, Asset and Nonce confidential variants using secp256k1-zkp types #77

Model Value, Asset and Nonce confidential variants using secp256k1-zkp types #77

Conversation

luckysori commented Apr 6, 2021 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori commented Apr 7, 2021

apoelstra commented Apr 13, 2021

sanket1729 commented Apr 13, 2021 • edited Loading

sanket1729 commented Apr 13, 2021

apoelstra commented Apr 13, 2021

sanket1729 commented Apr 13, 2021

apoelstra commented Apr 13, 2021

apoelstra commented Apr 13, 2021

luckysori commented Apr 14, 2021

apoelstra left a comment

Choose a reason for hiding this comment

luckysori commented Apr 6, 2021 •

edited

Loading

sanket1729 commented Apr 13, 2021 •

edited

Loading