feat: add optional range proof types #5372

hansieodendaal · 2023-05-08T15:45:39Z

Description

Added optional range proof types where the Bulletproof+ is no longer the only acceptable proof.
The RevealedValue proof hinges on the prover revealing the commitment value as equal to the minimum_value_promise
and creating a special version of the metadata signature where the deterministic ephemeral_commitment nonce r_a is
set equal to zero (see RFC-0182).
As a result of the optional range proof, we do not use Bulletproof+ range-proof rewinding anymore. Encrypted value was replaced with encrypted openings and updated scanning and recovery based on encrypted openings.
Updated the igor and esmeralda genesis block faucets to make use of the new optional range proofs.

Motivation and Context

This will allow faucet outputs in the genesis block without the data overhead of a Bulletproof+ range proof for each. Other use cases include special transactions with revealed value outputs, but subject to the consensus constants where RevealedValue range proofs need to be switched on explicitly.

How Has This Been Tested?

Unit tests
Cucumber tests
System-level tests:

On igor - base nodes, SHA3 mining, merged mining, console wallets
On esmeralda - base nodes, SHA3 mining, merged mining, console wallets

What process can a PR reviewer use to test or verify this change?

Recommend a system-level test

Breaking Changes

None
Requires data directory on base node to be deleted
Requires hard fork
Other - Please specify

BREAKING CHANGE: New genesis block for igor and esmeralda, thus restart the blockchains.

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

AaronFeickert · 2023-05-08T16:36:47Z

applications/tari_app_grpc/proto/transaction.proto

+    // Encrypted Pedersen commitment openings (value and mask) for the output
+    EncryptedOpenings encrypted_openings = 6;
+    // The type of range proof used in the output
+    uint32 range_proof_type = 7;


Would it make more sense to make this a proper enum for clarity and consistency?

I would keep it like this as it is a simple conversion and consistent with uint32 output_type = 2;

AaronFeickert · 2023-05-08T16:38:53Z

base_layer/core/src/proto/transaction.proto

+    // Encrypted Pedersen commitment openings (value and mask) for the output
+    EncryptedOpenings encrypted_openings = 6;
+    // The type of range proof used in the output
+    uint32 range_proof_type = 7;


Would it make more sense to make this a proper enum for clarity and consistency?

I would keep it like this as it is a simple conversion and consistent with uint32 output_type = 2;

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

hansieodendaal · 2023-05-09T08:03:11Z

As per discussion with @stringhandler and @SWvheerden, it seems like a good strategy to implement the following:

Replace pub encrypted_value: EncryptedValue, with pub encrypted_openings: EncryptedOpeningsX, in TransactionOutput and remove encrypted_openings: Option<EncryptedOpeningsX> from OutputFeatures (this will add 24 + 32 = 56 bytes to TransactionOutput)
Change wallet recovery and one-sided scanning to make exclusive use of decrypting the encrypted_openings and verifying the commitment openings afterwards
Use standard Bulletproof+ range proofs only, thus without supplying the optional seed nonce that enables extracting ~~embedding~~ the mask for non-aggregated Bulletproof+ rewinding ~~(embedding a different private key into the range proof could have interesting use cases in future)~~
Also remove EncryptedOpeningsS as a contender

Any comments?

AaronFeickert · 2023-05-10T18:13:44Z

I'm not sure this does everything you might hope.

I'll use the signer/helper terminology that I've used elsewhere, where the helper is a semi-trusted device that must not gain spend authority, but which is tasked with scanning for outputs intended for the signer.

For standard interactive outputs, it was presumably already possible for the helper to receive the value encryption key at some point from the signer. At this point, it could attempt value decryption as an initial check. Without the rewind key, it would be unable to extract the mask, which could only be done by the signer when it wishes to authorize a spend. Note that the helper has no way to itself determine if a commitment is spendable without the signer.

For legacy one-sided outputs, it was presumably already possible for the helper to scan for these. It could quickly check scripts to watch for the signer's wallet public key to appear. However, it could not decrypt values or perform mask extraction without being able to complete the sender's ephemeral Diffie-Hellman exchange, which would require access to the signer's wallet secret key.

For stealth one-sided outputs, it was not already possible for the helper to perform any scanning itself, since it could not check scripts for one-time public keys without completing the sender's ephemeral Diffie-Hellman exchange, which again would require access to the signer's wallet secret key. Further, it could not decrypt values or perform mask extraction for the same reason.

The proposed change does unify the handling of masks, which previously would have been different in the case of revealed-value outputs that do not have range proofs to use for mask extraction.

In the case of legacy and stealth one-sided outputs, the ability (or lack thereof) of the helper to scan for outputs does not change, as the value-with-mask encryption key would still be derived from a Diffie-Hellman exchange that requires the signer's wallet secret key to complete.

In the case of standard interactive outputs, the scanning process seems to be made worse by the new design, depending on the handling of scripts. The use of a single encrypted field for value and mask means that a helper who can authenticate the ciphertext obtains both the value and mask, which grants it spend authority in the absence of any additional script public key whose corresponding secret key is known only to the signer. This seems out of line with the security goals.

AaronFeickert · 2023-05-10T18:32:37Z

Another comment worth noting for standard interactive outputs is that if an additional script key (known only to the signer) is always required for spend authority in addition to knowledge of the commitment mask, the use of range proof offloading (which requires nontrivial state-machine interaction between signer and helper) would be unnecessary.

github-actions · 2023-05-14T06:00:56Z

Test Results

  4 files +  4   2 errors 12 suites +12 16m 15s ⏱️ + 16m 15s
28 tests +28   9 ✔️ +  9 0 💤 ±0 19 ❌ +19
66 runs +66   9 ✔️ +  9 0 💤 ±0 57 ❌ +57

For more details on these parsing errors and failures, see this check.

Results for commit f86a5f4. ± Comparison against base commit 8809b9f.

♻️ This comment has been updated with latest results.

github-actions · 2023-05-15T09:47:41Z

Test Results (pull_request)

792 tests 791 ✔️ 3m 29s ⏱️
  18 suites     0 💤
    1 files     1 ❌

For more details on these failures, see this check.

Results for commit e767067.

github-actions · 2023-05-15T10:32:40Z

Test Results (Integration tests)

29 tests +29 29 ✔️ +29 16m 41s ⏱️ + 16m 41s
12 suites +12   0 💤 ±  0
  2 files +  2   0 ❌ ±  0

Results for commit f2b1aad. ± Comparison against base commit 53ee32b.

♻️ This comment has been updated with latest results.

SWvheerden

This looks good

base_layer/core/src/blocks/genesis_block.rs

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

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

base_layer/wallet/migrations/2023-05-15-112514_encrypted_openings/up.sql

base_layer/wallet/src/output_manager_service/service.rs

github-actions · 2023-05-15T16:04:15Z

Test Results (CI)

1 147 tests +6 1 147 ✔️ +6 9m 59s ⏱️ +51s
    37 suites ±0       0 💤 ±0
      1 files ±0       0 ❌ ±0

Results for commit f2b1aad. ± Comparison against base commit 53ee32b.

This pull request removes 7 and adds 13 tests. Note that renamed tests count towards both.

tari_core ‑ transactions::coinbase_builder::test::valid_coinbase_with_rewindable_output
tari_core ‑ transactions::transaction_components::encrypted_value::test::it_encrypts_and_decrypts_correctly
tari_core ‑ transactions::transaction_components::test::test_output_rewinding_bulletproofs
tari_core ‑ transactions::transaction_components::test::unblinded_input_with_rewind_data
tari_core ‑ transactions::transaction_protocol::recipient::test::single_round_recipient_with_rewinding_bulletproofs
tari_wallet_ffi ‑ test::test_encrypted_value_empty
tari_wallet_ffi ‑ test::test_encrypted_value_filled

tari_core ‑ transactions::coinbase_builder::test::valid_coinbase_with_recoverable_output
tari_core ‑ transactions::transaction_components::encrypted_data::test::const_sizes_for_serialization_is_optimized
tari_core ‑ transactions::transaction_components::encrypted_data::test::it_converts_correctly
tari_core ‑ transactions::transaction_components::encrypted_data::test::it_encrypts_and_decrypts_correctly
tari_core ‑ transactions::transaction_components::range_proof_type::tests::it_converts_from_byte_to_output_type
tari_core ‑ transactions::transaction_components::test::test_output_recover_openings
tari_core ‑ transactions::transaction_components::test::unblinded_input_with_recovery_data
tari_core ‑ transactions::transaction_components::transaction_output::test::invalid_revealed_value_proofs_are_blocked
tari_core ‑ transactions::transaction_components::transaction_output::test::it_does_batch_verify_with_mixed_range_proof_types
tari_core ‑ transactions::transaction_protocol::recipient::test::single_round_recipient_with_recovery
…

♻️ This comment has been updated with latest results.

stringhandler

I might have missed it, but I don't see the block validation to make sure a transaction is correct when the value is revealed

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

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

stringhandler · 2023-05-17T11:35:36Z

I prefer the name encrypted_data to encrypted_openings, but either way I think the value and mask should be encrypted separately, although I suppose that's not directly used

AaronFeickert · 2023-05-20T14:07:24Z

There is no advantage in encrypting value and mask separately, it will just use more data (the tag size) and be more complex in the code.

You'd also need separate extended nonces, and would therefore bloat the ciphertext by an extra 40 bytes. Agreed that there is no advantage unless there's a use case where an entity would need to be able to authenticate the ciphertext without learning, say, the commitment mask. And it sounds like that is not expected to be the case.

Added optional range proof ...

hansieodendaal · 2023-05-23T14:00:06Z

I think we need some new tests for:

receiving onesided payments with revealed values

block and mempool validation for revealed values

recovery of interactive and one sided payments with revealed values

Must do ^^

AaronFeickert · 2023-05-24T20:41:40Z

@hansieodendaal: Is it always guaranteed that a value-revealing range proof check will also have a separate check for a valid metadata signature? This is required according to the verification step described in RFC-0182.

Previously, range proof verification was sufficient on its own to assert a commitment value is valid. Now, doing the zero-nonce scalar check in a value-revealing case is insufficient to conclude anything unless the entire metadata signature is also verified, which is not done internally to your range proof verifier.

My concern would be that some code path might assume validity without signature verification.

AaronFeickert · 2023-05-25T14:30:38Z

My concern would be that some code path might assume validity without signature verification.

Addressed in #5411 with a signature verification check.

## [0.50.0-pre.2](v0.50.0-pre.1...v0.50.0-pre.2) (2023-05-29) ### ⚠ BREAKING CHANGES * add optional range proof types (5372) ### Features * add metadata signature check ([5411](#5411)) ([9c2bf41](9c2bf41)) * add optional range proof types ([5372](#5372)) ([f24784f](f24784f)) * added burn feature to the console wallet ([5322](#5322)) ([45685b9](45685b9)) * improved base node monitoring ([5390](#5390)) ([c704890](c704890)) ### Bug Fixes * **comms:** only set final forward address if configured to port 0 ([5406](#5406)) ([ff7fb6d](ff7fb6d)) * deeplink to rfc spec ([5342](#5342)) ([806d3b8](806d3b8)) * don't use in memory datastores for chat client dht in integration tests ([#5399](#5399)) ([cbdca6f](cbdca6f)) * fix panic when no public addresses ([5367](#5367)) ([49be2a2](49be2a2)) * loop on mismatched passphrase entry ([5396](#5396)) ([ed120b2](ed120b2)) * use domain separation for wallet message signing ([5400](#5400)) ([7d71f8b](7d71f8b))

ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels May 8, 2023

AaronFeickert requested changes May 8, 2023

View reviewed changes

AaronFeickert reviewed May 8, 2023

View reviewed changes

base_layer/core/src/transactions/transaction_components/encrypted_openings_x.rs Outdated Show resolved Hide resolved

hansieodendaal force-pushed the ho_optional_range_proof branch from c1c40bb to 6021f47 Compare May 9, 2023 06:06

hansieodendaal force-pushed the ho_optional_range_proof branch from c237c62 to e5d5363 Compare May 10, 2023 02:53

hansieodendaal force-pushed the ho_optional_range_proof branch from f16bbca to e52af63 Compare May 11, 2023 15:10

ghpbot-tari-project added the CR-too_long Changes Requested - Your PR is too long label May 11, 2023

hansieodendaal force-pushed the ho_optional_range_proof branch 5 times, most recently from 60aa71f to c107dbe Compare May 14, 2023 05:44

hansieodendaal changed the title ~~[wip] feat: add optional range proof types~~ feat: add optional range proof types May 14, 2023

ghpbot-tari-project added the CR-one_job label May 14, 2023

hansieodendaal changed the title ~~feat: add optional range proof types~~ [wip] feat: add optional range proof types May 15, 2023

hansieodendaal force-pushed the ho_optional_range_proof branch from f86a5f4 to e767067 Compare May 15, 2023 09:34

SWvheerden reviewed May 15, 2023

View reviewed changes

hansieodendaal force-pushed the ho_optional_range_proof branch from e767067 to 2e617f3 Compare May 15, 2023 15:45

hansieodendaal force-pushed the ho_optional_range_proof branch from 2e617f3 to 5033485 Compare May 16, 2023 13:06

hansieodendaal changed the title ~~[wip] feat: add optional range proof types~~ feat: add optional range proof types May 16, 2023

stringhandler requested changes May 17, 2023

View reviewed changes

base_layer/core/src/transactions/transaction_components/encrypted_openings.rs Outdated Show resolved Hide resolved

base_layer/core/src/chain_storage/tests/blockchain_database.rs Show resolved Hide resolved

hansieodendaal changed the title ~~feat: add optional range proof types~~ [wip] feat: add optional range proof types May 19, 2023

hansieodendaal dismissed SWvheerden’s stale review via 7791f07 May 20, 2023 08:57

hansieodendaal force-pushed the ho_optional_range_proof branch from 7791f07 to 05b97e9 Compare May 23, 2023 10:30

hansieodendaal changed the title ~~[wip] feat: add optional range proof types~~ feat: add optional range proof types May 23, 2023

ghpbot-tari-project added the P-conflicts Process - The PR has merge conflicts that need to be resolved label May 23, 2023

hansieodendaal added 10 commits May 23, 2023 14:38

Add optional range proof

3f2f46a

Added optional range proof ...

wip review comments 1

fe42b30

encrypted_value -> encrypted_openings

024db07

remove rewindable outputs

e94203c

make range proof optional

e25959e

add faucets with optional range proofs

f287b3f

range_proof_type validation

62531bb

migrations

6fa781b

review comments, fix tests

857ec42

fix tests, review comments

f2b1aad

hansieodendaal force-pushed the ho_optional_range_proof branch from 05b97e9 to f2b1aad Compare May 23, 2023 12:40

ghpbot-tari-project removed the P-conflicts Process - The PR has merge conflicts that need to be resolved label May 23, 2023

hansieodendaal mentioned this pull request May 23, 2023

Create core code for revealed value outputs, with console wallet interface #5403

Closed

stringhandler approved these changes May 23, 2023

View reviewed changes

stringhandler merged commit f24784f into tari-project:development May 23, 2023

hansieodendaal mentioned this pull request May 23, 2023

Make range proofs optional if the value can be recovered from the metadata signature #5364

Closed

AaronFeickert mentioned this pull request May 24, 2023

feat!: update faucet value test and Igor genesis block #5328

Closed

hansieodendaal deleted the ho_optional_range_proof branch May 25, 2023 05:38

AaronFeickert mentioned this pull request May 25, 2023

docs: add range proof verification warnings #5412

Closed

SWvheerden mentioned this pull request May 29, 2023

chore: new Esmeralda version #5423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add optional range proof types #5372

feat: add optional range proof types #5372

hansieodendaal commented May 8, 2023 •

edited

Loading

AaronFeickert May 8, 2023

hansieodendaal May 9, 2023

AaronFeickert May 8, 2023

hansieodendaal May 9, 2023

hansieodendaal commented May 9, 2023 •

edited

Loading

AaronFeickert commented May 10, 2023

AaronFeickert commented May 10, 2023 •

edited

Loading

github-actions bot commented May 14, 2023 •

edited

Loading

github-actions bot commented May 15, 2023

github-actions bot commented May 15, 2023 •

edited

Loading

SWvheerden left a comment •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading

stringhandler left a comment

stringhandler commented May 17, 2023

AaronFeickert commented May 20, 2023

hansieodendaal commented May 23, 2023

AaronFeickert commented May 24, 2023 •

edited

Loading

AaronFeickert commented May 25, 2023

feat: add optional range proof types #5372

feat: add optional range proof types #5372

Conversation

hansieodendaal commented May 8, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

AaronFeickert May 8, 2023

Choose a reason for hiding this comment

hansieodendaal May 9, 2023

Choose a reason for hiding this comment

AaronFeickert May 8, 2023

Choose a reason for hiding this comment

hansieodendaal May 9, 2023

Choose a reason for hiding this comment

hansieodendaal commented May 9, 2023 • edited Loading

AaronFeickert commented May 10, 2023

AaronFeickert commented May 10, 2023 • edited Loading

github-actions bot commented May 14, 2023 • edited Loading

Test Results

github-actions bot commented May 15, 2023

Test Results (pull_request)

github-actions bot commented May 15, 2023 • edited Loading

Test Results (Integration tests)

SWvheerden left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 15, 2023 • edited Loading

Test Results (CI)

stringhandler left a comment

Choose a reason for hiding this comment

stringhandler commented May 17, 2023

AaronFeickert commented May 20, 2023

hansieodendaal commented May 23, 2023

AaronFeickert commented May 24, 2023 • edited Loading

AaronFeickert commented May 25, 2023

hansieodendaal commented May 8, 2023 •

edited

Loading

hansieodendaal commented May 9, 2023 •

edited

Loading

AaronFeickert commented May 10, 2023 •

edited

Loading

github-actions bot commented May 14, 2023 •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading

SWvheerden left a comment •

edited

Loading

github-actions bot commented May 15, 2023 •

edited

Loading

AaronFeickert commented May 24, 2023 •

edited

Loading