fog omits compressed commitment from TxOutRecord #71

cbeck88 · 2021-05-03T20:57:56Z

This updates TxOutRecord in a few ways:

amount_compressed_commitment_data is now optional, and omitted
except for backwards compat. (If backwards compat is not needed,
then that field could be deleted entirely.)
amount_compressed_commitment_data_crc32 is now present when
that field is omitted. This is an IEEE crc32 of the omitted data.
The sample paykit is updated so that it attempts to reconstruct
the compressed commitment using the view private key. (This was
koe's idea.) Then it checks the crc32 to confirm correct recomputation.
This saves ~30 bytes in fog view, which we can use for the memo.
A method is added to construct TxOutRecord more nicely, and then
a bunch of test code and the enclave code is simplified.

cbeck88 · 2021-05-03T20:58:33Z

fog/api/proto/view.proto

-    /// Public key.
-    external.CompressedRistretto public_key = 3;
-}
-


this is omitted because it's not actually used in any public facing APIs, kyle and alex thought it's better to remove it if that isn't the case since it might confuse someone

cbeck88 · 2021-05-03T20:59:12Z

fog/api/tests/fog_types.rs

-        round_trip_message::<fog_types::view::FogTxOut, fog_api::view::FogTxOut>(&test_val);
-    });
-}
-


this test is removed because FogTxOut type is removed from protobuf per discussion

cbeck88 · 2021-05-03T21:01:29Z

fog/api/proto/view.proto

+    /// the full tx_out_amount_commitment_data is omitted.
+    /// The client can recompute the tx_out_amount_commitment from the other data that we include.
+    /// They can confirm correct recomputation by checking this crc value.
+    fixed32 tx_out_amount_commitment_data_crc32 = 8;


this change to the proto is the main point of the PR -- the fog ingest enclave will only set this crc32 field and leave the earlier commtiment_data field empty.

the crc32 is also possibly not necessary, but kyle suggested that we should retain the same level of "checkability" as before, and this only adds a few bytes

cbeck88 · 2021-05-03T21:03:00Z

fog/fog_types/src/view.rs

}
+

this shows how reconstructing the commitment and checking the crc32 value works

cbeck88 · 2021-05-03T21:03:38Z

fog/fog_types/src/view.rs

+    /// The timestamp of the block in which this TxOut appeared, in seconds
+    /// since the Unix epoch.
+    pub timestamp: u64,
+}


this is added in order to have a nice constructor function for TxOutRecord, otherwise you take three different u64's as arguments and if they are in the wrong order, you have a bug

cbeck88 · 2021-05-03T21:04:04Z

fog/ingest/enclave/impl/tests/tx_processing.rs

@@ -82,7 +82,7 @@ fn test_ingest_enclave(logger: Logger) {
        assert_eq!(tx_rows.len(), 10);

        // Check that the tx row ciphertexts have the right size
-        const EXPECTED_PAYLOAD_SIZE: usize = 188; // The observed tx_row.payload size
+        const EXPECTED_PAYLOAD_SIZE: usize = 159; // The observed tx_row.payload size


this shows the change in size of TxOutRecord when it is serialized, so we saved 29 bytes

eranrund

LGTM

eranrund · 2021-05-03T21:22:10Z

fog/api/proto/view.proto

@@ -231,6 +217,8 @@ message FogTxOut {
 /// and that's the version of the TxOut that you should use when building a transaction.
 message TxOutRecord {
    /// The (compressed ristretto) bytes of commitment associated to amount field in the TxOut that was recovered
+    ///
+    /// Note: This field is omitted in recent versions, because it can be reconstructed by the recipient instead.


You can annotate it as deprecated: see deprecated on https://developers.google.com/protocol-buffers/docs/proto3

for running deployments of fog, though, we can't update these serialized TxOutRecords. so whether the field can be deprecated depends on if those fogs will be wiped and started again after beta or something like this. otherwise, sometimes people will have to keep reading this field

after further discussion, we cannot deprecate this field, we believe it will continue to exist in the wild

eranrund · 2021-05-03T21:24:20Z

fog/fog_types/src/view.rs

+
+    /// Extract a FogTxOut object from the (flattened) TxOutRecord object
+    /// Note that this discards some metadata (timestamp, block_index,
+    /// global_index).
    pub fn get_fog_tx_out(&self) -> Result<FogTxOut, KeyError> {


I believe this should be TryInto

cbeck88 · 2021-05-04T05:45:23Z

This passed deploy

kylefleming

Proto interface LGTM.

This updates TxOutRecord in a few ways: * amount_compressed_commitment_data is now optional, and omitted except for backwards compat. (If backwards compat is not needed, then that field could be deleted entirely.) * amount_compressed_commitment_data_crc32 is now present when that field is omitted. This is an IEEE crc32 of the omitted data. * The sample paykit is updated so that it attempts to reconstruct the compressed commitment using the view private key. (This was koe's idea.) Then it checks the crc32 to confirm correct recomputation. This saves ~30 bytes in fog view, which we can use for the memo. * A method is added to construct TxOutRecord more nicely, and then a bunch of test code and the enclave code is simplified.

cbeck88 · 2021-05-05T09:12:19Z

rebased on master

This reverts commit 1e1fc1c. This is a breaking change, but we need to test the fee changes first, so we will revert this for now and re-apply it later Changes to be committed: modified: Cargo.lock modified: fog/api/proto/view.proto modified: fog/api/tests/fog_types.rs modified: fog/fog_types/Cargo.toml modified: fog/fog_types/src/view.rs modified: fog/ingest/enclave/impl/src/lib.rs modified: fog/ingest/enclave/impl/tests/tx_processing.rs modified: fog/ingest/enclave/trusted/Cargo.lock modified: fog/ledger/enclave/trusted/Cargo.lock modified: fog/sample-paykit/src/cached_tx_data/mod.rs modified: fog/sample-paykit/src/client.rs modified: fog/sample-paykit/src/error.rs modified: fog/test_infra/src/mock_users.rs modified: fog/view/enclave/trusted/Cargo.lock modified: fog/view/protocol/src/user_private.rs

…" (#75)" This reverts commit 8b97079. This re-applies that commit after the release branch has been created

…" (#75)" This reverts commit 8b97079. This re-applies that commit after the release branch has been created and we have agreed to release this commit in the next release

…" (#75)" (#95) This reverts commit 8b97079. This re-applies that commit after the release branch has been created and we have agreed to release this commit in the next release

cbeck88 · 2021-06-24T18:35:47Z

fog/sample-paykit/src/cached_tx_data/mod.rs

+        let fog_tx_out = rec.get_fog_tx_out()?;
+
+        // Reconstute TxOut from FogTxOut and our view private key
+        let tx_out = fog_tx_out.try_recover_tx_out(&account_key.view_private_key())?;


this shows where the sample paykit is using the new code

cbeck88 requested review from jcape, kylefleming, voloshyn, eranrund, tubi70, queenbdubs, sugargoat and a team May 3, 2021 20:57

cbeck88 commented May 3, 2021

View reviewed changes

fog/fog_types/src/view.rs

}

Copy link

Contributor Author

cbeck88 May 3, 2021

Choose a reason for hiding this comment

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

this shows how reconstructing the commitment and checking the crc32 value works

cbeck88 commented May 3, 2021

View reviewed changes

eranrund approved these changes May 3, 2021

View reviewed changes

voloshyn approved these changes May 4, 2021

View reviewed changes

kylefleming approved these changes May 5, 2021

View reviewed changes

cbeck88 added 2 commits May 5, 2021 12:10

clippy

4a87b8c

cbeck88 force-pushed the fog_omits_compressed_commitment branch from b32d0f0 to 4a87b8c Compare May 5, 2021 09:10

cbeck88 merged commit 1e1fc1c into master May 5, 2021

cbeck88 deleted the fog_omits_compressed_commitment branch May 5, 2021 09:42

cbeck88 added a commit that referenced this pull request May 7, 2021

Revert "Revert "fog omits compressed commitment from TxOutRecord (#71)…

6c406b1

…" (#75)" This reverts commit 8b97079. This re-applies that commit after the release branch has been created

cbeck88 mentioned this pull request Jun 23, 2021

Revert "Revert "fog omits compressed commitment from TxOutRecord (#71)" (#75)" #95

Merged

cbeck88 commented Jun 24, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fog omits compressed commitment from TxOutRecord #71

fog omits compressed commitment from TxOutRecord #71

cbeck88 commented May 3, 2021 •

edited

cbeck88 May 3, 2021

cbeck88 May 3, 2021

cbeck88 May 3, 2021 •

edited

cbeck88 May 3, 2021 •

edited

cbeck88 May 3, 2021

cbeck88 May 3, 2021

cbeck88 May 3, 2021

eranrund left a comment

eranrund May 3, 2021

cbeck88 May 3, 2021 •

edited

cbeck88 May 4, 2021

eranrund May 3, 2021

cbeck88 commented May 4, 2021

kylefleming left a comment

cbeck88 commented May 5, 2021

cbeck88 Jun 24, 2021

fog omits compressed commitment from TxOutRecord #71

fog omits compressed commitment from TxOutRecord #71

Conversation

cbeck88 commented May 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeck88 May 3, 2021 • edited

Choose a reason for hiding this comment

cbeck88 May 3, 2021 • edited

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

eranrund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeck88 May 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeck88 commented May 4, 2021

kylefleming left a comment

Choose a reason for hiding this comment

cbeck88 commented May 5, 2021

Choose a reason for hiding this comment

cbeck88 commented May 3, 2021 •

edited

cbeck88 May 3, 2021 •

edited

cbeck88 May 3, 2021 •

edited

cbeck88 May 3, 2021 •

edited