feat!: add recovery byte to output features #3727

hansieodendaal · 2022-01-21T06:25:30Z

Description

Added recovery_byte to OutputFeatures. The recovery byte will uniformly distribute all outputs in the blockchain into 255 "bins" that can be identified by the commitment of the output in combination with a master recovery byte key.

This change will come into effect on dibbler testnet once the OutputFeatures version is updated from V0 to V1, which will result in a hard fork.

All funds will be retained after the hard fork.

Motivation and Context

The reasoning behind this is that it will enable a client to request only a specific subset of full-on outputs in a set of blocks. The commitment only takes up a fraction (less than 5%) of the byte space for a full-on output. A typical use case for this feature would be a wallet recovery client that needs to scan all outputs in the blockchain from the birthdate of the wallet to be recovered. The client would request all commitments in the set of blocks from a base node and identify all those that belong to their own "bin", after which a tailor made request can be submitted to retrieve the full-on output data for the identified commitments. This will result in a better than 20x saving in the amount of data transmitted for wallet recovery.

How Has This Been Tested?

Unit tests
Cucumber integration tests
Performed system-level tests using a wallet that contains embedded output features in json text that does not contain the recovery byte.

philipr-za

Generally looking really good, couple comments but the main one is that the three methods added to the OMS API for adding a rewindable output seem like they don't belong outside of the tests.

Please keep further updates to this PR as their own commits so I can review the new stuff only.

base_layer/core/src/consensus/consensus_constants.rs

base_layer/core/src/transactions/transaction_protocol/single_receiver.rs

base_layer/tari_stratum_ffi/src/lib.rs

philipr-za · 2022-01-24T10:21:46Z

base_layer/wallet/src/output_manager_service/handle.rs

@@ -295,6 +315,24 @@ impl OutputManagerHandle {
        }
    }

+    pub async fn add_rewindable_output(


Are add_rewindable_output, add_rewindable_output_with_tx_id, add_rewindable_unvalidated_output only used for testing? I am assuming that they are because how would a normal client of this service be able to construct its own rewindable output externally.

So based on that I would say these three methods don't actually belong in API of this service. Now the question is how do you get these kind of outputs into the output manager for testing. The answer is that in the testing code you have access to the OMS database backend and you can insert them directly.

I would counter that their counterparts, that is without the rewindable part, also need to be taken out of the API then. This is expanding on the pattern that is already there.

Your opinion?

The counter parts are used for importing of outputs manually and by the UTXO scanner.

~~As discussed, I will remove these from the API.~~

Maybe for a next PR.

base_layer/wallet/src/output_manager_service/handle.rs

base_layer/wallet/src/output_manager_service/service.rs

base_layer/core/src/consensus/consensus_encoding/integers.rs

base_layer/core/src/consensus/consensus_constants.rs

hansieodendaal

Changes requested:
The json text embedded in the wallet should only be done at wallet startup, not every time output or transaction data is read from the db.

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs

base_layer/wallet/src/output_manager_service/storage/sqlite_db/output_sql.rs

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs

delta1

Well done this was a lot of work. I have a few comments and questions, and my only real concern is that a developer must remember to update the recovery byte when creating OutputFeatures. I would vastly prefer it to be automatically updated whenever the commitment of the output is set or updated.

applications/tari_app_grpc/src/conversions/output_features.rs

base_layer/core/src/proto/transaction.rs

delta1 · 2022-03-07T08:44:36Z

base_layer/core/src/chain_storage/tests/blockchain_database.rs

@@ -779,6 +783,7 @@ mod fetch_utxo_by_unique_id {
            lock: 0,
            features: features
        )]);
+        features.set_recovery_byte(tx_outputs[0].features.recovery_byte);


What happens if someone creates an OutputFeatures and forgets to call set_recovery_byte ?

A typical flow to create an output is (only discussing the pertinent information):

Select output features using a default/random recovery byte

Calculate output weight & determine the fee

Finalize the commitment information (i.e. select the blinding factor)

Set the recovery byte with the commitment as input (i.e. finalize the output features)

Calculate the partial or final metadata signature, also signing the output features

Finalize the output as an unblinded output, using all previous information as inputs

Recovery byte checks is done in strategic places in the code base and automatically set whenever it needs to be updated within the various protocols. If someone adds functionality and forget to set the recovery byte as required for the protocol, recovery byte validation will not succeed the validation in unblinded_output::as_transaction_output, unblinded_output::as_rewindable_transaction_output and transaction_initializer::with_output.

base_layer/core/src/transactions/transaction_components/output_features.rs

base_layer/wallet/src/output_manager_service/handle.rs

base_layer/wallet/src/output_manager_service/service.rs

base_layer/wallet_ffi/src/lib.rs

hansieodendaal · 2022-03-07T12:48:01Z

I would vastly prefer it to be automatically updated whenever the commitment of the output is set or updated.

Not really possible to do this, please see detailed comment above.

stringhandler

I would suggest trying the #[serde(default= 0)] and not changing any of the existing json in the faucet or sqlite db.

I'll try test this locally as well

base_layer/core/src/test_helpers/mod.rs

base_layer/core/src/transactions/transaction_components/output_features.rs

base_layer/wallet/src/output_manager_service/service.rs

stringhandler · 2022-03-09T10:48:17Z

base_layer/wallet/src/output_manager_service/service.rs

@@ -848,13 +896,13 @@ where
            unique_id,
            fee_per_gram,
        );
-        let output_features = OutputFeatures::default();
+        let output_features_estimate = OutputFeatures { ..Default::default() };


This is the same thing

The wording choice of output_features_estimate is important here to let the reader know at this stage these features are not the real features and only used to for fee estimation.

Think he means OutputFeatures { ..Default::default() }; is a convoluted way of writing the previous code OutputFeatures::default()

Yip, that was a refactoring remnant.

base_layer/wallet/src/output_manager_service/storage/sqlite_db/mod.rs

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs

Added recovery_byte to OutputFeatures. This will not change the genesis block and only become effective with version V1 of the OutputFeatures, resulting in a hard fork. All funds in the wallet will be retained. Co-authored-by: Mike the Tike <mikethetike@tari.com> Co-authored-by: David Main <51991544+StriderDM@users.noreply.github.com>

* development: (118 commits) chore: clean up providing seed words from LibWallet (tari-project#3906) chore: move tari_script into its own crate (tari-project#3909) fix(consensus): check blockchain version within valid range (tari-project#3916) ci: fix missing npm deps and add javascript ci (tari-project#3910) refactor: use clap as a commands parser (tari-project#3867) chore: use git tagged tari_utilities and tari-crypto deps (tari-project#3913) fix: aligned tables left (tari-project#3899) ci: fix vue build v0.29.0 feat!: add recovery byte to output features (tari-project#3727) add ffi ci check (tari-project#3915) fix(block-sync): use avg latency to determine slow sync peer for block sync (tari-project#3912) fix: fix merge mining proxy pool mining (tari-project#3814) revert: remove use of blocking tasks for DHT db (reverts tari-project#3887) (tari-project#3901) chore: add license info missing from some crates (tari-project#3892) fix(core): correctly filter pruned sync peers for block sync (tari-project#3902) ci: revert bors squash merge (tari-project#3900) fix: update metadata size calculation to use FixedSet.iter() docs(rfc): deep links structure convention - deep links is use (tari-project#3897) ci: use squash merge for bors (tari-project#3896) ...

* development: chore: clean up providing seed words from LibWallet (tari-project#3906) chore: move tari_script into its own crate (tari-project#3909) fix(consensus): check blockchain version within valid range (tari-project#3916) ci: fix missing npm deps and add javascript ci (tari-project#3910) refactor: use clap as a commands parser (tari-project#3867) chore: use git tagged tari_utilities and tari-crypto deps (tari-project#3913) fix: aligned tables left (tari-project#3899) ci: fix vue build v0.29.0 feat!: add recovery byte to output features (tari-project#3727) add ffi ci check (tari-project#3915)

hansieodendaal force-pushed the ho_add_filter_byte branch 7 times, most recently from c6951ed to 355b4a5 Compare January 24, 2022 09:02

philipr-za reviewed Jan 24, 2022

View reviewed changes

base_layer/core/src/consensus/consensus_encoding/integers.rs Outdated Show resolved Hide resolved

base_layer/core/src/consensus/consensus_constants.rs Outdated Show resolved Hide resolved

hansieodendaal force-pushed the ho_add_filter_byte branch 4 times, most recently from 50f3f8a to 3f7b347 Compare January 27, 2022 03:03

hansieodendaal changed the title ~~[wip] feat!: add recovery byte to output features~~ feat!: add recovery byte to output features Jan 27, 2022

stringhandler added W-consensus_breaking Warn - A change requiring a hard fork to be activated P-do_not_merge Process - Not ready for merging labels Jan 27, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from 3f7b347 to 9f6e9a2 Compare March 3, 2022 07:11

hansieodendaal changed the title ~~feat!: add recovery byte to output features~~ [wip] feat!: add recovery byte to output features Mar 3, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from 9f6e9a2 to c750fe6 Compare March 3, 2022 09:42

hansieodendaal changed the title ~~[wip] feat!: add recovery byte to output features~~ feat!: add recovery byte to output features Mar 3, 2022

hansieodendaal changed the title ~~feat!: add recovery byte to output features~~ [wip] feat!: add recovery byte to output features Mar 3, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from 7f42f21 to dafe55f Compare March 3, 2022 15:06

hansieodendaal changed the title ~~[wip] feat!: add recovery byte to output features~~ feat!: add recovery byte to output features Mar 3, 2022

hansieodendaal changed the title ~~feat!: add recovery byte to output features~~ [wip] feat!: add recovery byte to output features Mar 4, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from 3442dd9 to c53dd74 Compare March 4, 2022 14:49

hansieodendaal changed the title ~~[wip] feat!: add recovery byte to output features~~ feat!: add recovery byte to output features Mar 4, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from c53dd74 to 70c34ec Compare March 7, 2022 09:15

hansieodendaal commented Mar 7, 2022

View reviewed changes

hansieodendaal changed the title ~~feat!: add recovery byte to output features~~ [wip] feat!: add recovery byte to output features Mar 7, 2022

delta1 reviewed Mar 7, 2022

View reviewed changes

hansieodendaal force-pushed the ho_add_filter_byte branch from 70c34ec to 865672c Compare March 8, 2022 08:53

hansieodendaal changed the title ~~[wip] feat!: add recovery byte to output features~~ feat!: add recovery byte to output features Mar 8, 2022

hansieodendaal removed W-consensus_breaking Warn - A change requiring a hard fork to be activated P-do_not_merge Process - Not ready for merging labels Mar 8, 2022

hansieodendaal force-pushed the ho_add_filter_byte branch from 865672c to 17986ba Compare March 8, 2022 12:48

stringhandler reviewed Mar 9, 2022

View reviewed changes

hansieodendaal force-pushed the ho_add_filter_byte branch 2 times, most recently from 79ee297 to efb9596 Compare March 11, 2022 02:43

hansieodendaal and others added 3 commits March 11, 2022 14:40

review comments 1

9d12d54

review comments 2

165f470

hansieodendaal force-pushed the ho_add_filter_byte branch from efb9596 to 165f470 Compare March 11, 2022 12:47

Merge branch 'development' into ho_add_filter_byte

ed811d2

stringhandler added the mq-approved label Mar 14, 2022

stringhandler approved these changes Mar 14, 2022

View reviewed changes

aviator-app bot merged commit c9985de into tari-project:development Mar 14, 2022

hansieodendaal deleted the ho_add_filter_byte branch March 14, 2022 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: add recovery byte to output features #3727

feat!: add recovery byte to output features #3727

hansieodendaal commented Jan 21, 2022 •

edited

Loading

philipr-za left a comment

philipr-za Jan 24, 2022

hansieodendaal Jan 24, 2022

philipr-za Jan 24, 2022

hansieodendaal Jan 24, 2022 •

edited

Loading

hansieodendaal left a comment

delta1 left a comment

delta1 Mar 7, 2022

hansieodendaal Mar 7, 2022 •

edited

Loading

hansieodendaal commented Mar 7, 2022

stringhandler left a comment

stringhandler Mar 9, 2022

hansieodendaal Mar 10, 2022

sdbondi Mar 14, 2022

hansieodendaal Mar 14, 2022

feat!: add recovery byte to output features #3727

feat!: add recovery byte to output features #3727

Conversation

hansieodendaal commented Jan 21, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

philipr-za 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

hansieodendaal Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

delta1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansieodendaal Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

hansieodendaal commented Mar 7, 2022

stringhandler 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

hansieodendaal commented Jan 21, 2022 •

edited

Loading

hansieodendaal Jan 24, 2022 •

edited

Loading

hansieodendaal Mar 7, 2022 •

edited

Loading