feat: support for stealth addresses in one-sided transactions #4310

agubarev · 2022-07-14T10:04:45Z

Description

Added support of one-sided transactions with stealth addresses to the output scanner.

Motivation and Context

https://rfc.tari.com/RFC-0203_StealthAddresses.html

How Has This Been Tested?

manually, unit tests

… of one-sided transactions, with simple and stealth addresses Signed-off-by: Andrei Gubarev <[email protected]>

Signed-off-by: Andrei Gubarev <[email protected]>

…ealth_addresses_clean

…esses_clean # Conflicts: # base_layer/wallet/src/output_manager_service/service.rs

Signed-off-by: Andrei Gubarev <[email protected]>

…ealth_addresses_clean

Signed-off-by: Andrei Gubarev <[email protected]>

SWvheerden

Looks good.

SWvheerden · 2022-07-18T05:01:29Z

base_layer/wallet/src/output_manager_service/service.rs

+                        let ks = PublicKey::from_secret_key(&c) + public_key;
+
+                        if &ks != provided_spending_public.as_ref() {
+                            return None;


I don't think return here will give the correct behaviour.
If this case here is not correct, we should not drop all other correct UTXO and return back with none. This is in the middle of an iteration, I suspect you only want to continue here?

Yes, return None is to skip, to return from filter_map closure. This function is to only find and add certain outputs, it's not supposed to pass anything through.

I was also thrown by this. I think the filter_map is too large and you should probably rather use a simple for loop

base_layer/wallet/src/output_manager_service/service.rs

Signed-off-by: Andrei Gubarev <[email protected]>

base_layer/wallet/src/output_manager_service/service.rs

base_layer/wallet/src/transaction_service/service.rs

changed DSH label for stealth addresses to `com.tari.stealth_address` Signed-off-by: Andrei Gubarev <[email protected]>

stringhandler · 2022-07-19T17:40:55Z

base_layer/wallet/src/output_manager_service/service.rs

+                        let ks = PublicKey::from_secret_key(&c) + public_key;
+
+                        if &ks != provided_spending_public.as_ref() {
+                            return None;


I was also thrown by this. I think the filter_map is too large and you should probably rather use a simple for loop

stringhandler · 2022-07-19T17:42:17Z

base_layer/wallet/src/output_manager_service/service.rs

+                    // ----------------------------------------------------------------------------
+                    // simple one-sided address
+                    [Opcode::PushPubKey(spending_key_public)] => {
+                        let keys = match self.resources.db.get_all_known_one_sided_payment_scripts() {


Calling this db method for every output is not necessary, rather move it outside the loop

Signed-off-by: Andrei Gubarev <[email protected]>

hansieodendaal

LGTM - just a couple of small comments

hansieodendaal · 2022-07-20T07:30:47Z

base_layer/wallet/src/output_manager_service/service.rs

@@ -1888,97 +1891,159 @@ where
        Ok(())
    }

-    /// Attempt to scan and then rewind all of the given transaction outputs into unblinded outputs based on known
-    /// pubkeys
    fn scan_outputs_for_one_sided_payments(


Suggested change

fn scan_outputs_for_one_sided_payments(

/// Attempt to scan and then rewind all of the given transaction outputs into unblinded outputs based on known

/// pubkeys

fn scan_outputs_for_one_sided_payments(

hansieodendaal · 2022-07-20T11:55:55Z

base_layer/wallet/src/output_manager_service/service.rs

+                    // ----------------------------------------------------------------------------
+                    // simple one-sided address
+                    [Opcode::PushPubKey(spending_key_public)] => {
+                        let keys = match self.resources.db.get_all_known_one_sided_payment_scripts() {


hansieodendaal · 2022-07-20T12:08:50Z

base_layer/wallet/src/output_manager_service/service.rs

+                // NOTE: [RFC 203 on Stealth Addresses](https://rfc.tari.com/RFC-0203_StealthAddresses.html)
+                [Opcode::PushPubKey(nonce), Opcode::Drop, Opcode::PushPubKey(scanned_pk)] => {
+                    // computing shared secret
+                    let c = RistrettoSecretKey::from_bytes(


Suggested change

let c = RistrettoSecretKey::from_bytes(

let stealth_key = PrivateKey::from_bytes(

We should use the generic types as far as possible

hansieodendaal · 2022-07-20T13:07:14Z

Please add unit and cucumber tests (see Scenario: Wallet sending and receiving one-sided transactions) for one sided stealth addresses.

Cifko · 2022-07-20T13:35:34Z

Please add unit and cucumber tests (see Scenario: Wallet sending and receiving one-sided transactions) for one sided stealth addresses.

This is planned in next PR, because for that we need both PRs (this is scanning, and other one is sending) to be merged.

stringhandler · 2022-07-20T13:36:25Z

Your PR has already been merged @Cifko

Signed-off-by: Andrei Gubarev <[email protected]>

# Conflicts: # base_layer/wallet/src/output_manager_service/service.rs

Signed-off-by: Andrei Gubarev <[email protected]>

…ealth_addresses_clean # Conflicts: # integration_tests/features/support/wallet_steps.js # integration_tests/features/support/world.js

stringhandler · 2022-07-22T08:36:53Z

integration_tests/features/WalletTransactions.feature

@@ -32,6 +32,35 @@ Feature: Wallet Transactions
    Then all nodes are at height 30
    Then I wait for wallet WALLET_C to have at least 1500000 uT

+


Suggested change

@critical

stringhandler · 2022-07-22T08:38:25Z

base_layer/wallet_ffi/src/lib.rs

+/// # Safety
+/// The `public_key_destroy()` must be called when finished with a `TariPublicKey` to prevent a memory leak
+#[no_mangle]
+pub unsafe extern "C" fn private_key_add(


I don't think these are needed

Signed-off-by: Andrei Gubarev <[email protected]>

reworked scan_outputs_for_one_sided_payments() to handle both types…

55d3e59

… of one-sided transactions, with simple and stealth addresses Signed-off-by: Andrei Gubarev <[email protected]>

agubarev marked this pull request as draft July 14, 2022 10:05

agubarev and others added 7 commits July 14, 2022 13:13

Merge branch 'development' into stealth_addresses_clean

a5030ed

bugfix

a81d353

Signed-off-by: Andrei Gubarev <[email protected]>

Merge remote-tracking branch 'origin/stealth_addresses_clean' into st…

30ce16e

…ealth_addresses_clean

Merge remote-tracking branch 'upstream/development' into stealth_addr…

90a6517

…esses_clean # Conflicts: # base_layer/wallet/src/output_manager_service/service.rs

fix

3fad93d

Signed-off-by: Andrei Gubarev <[email protected]>

Merge remote-tracking branch 'origin/stealth_addresses_clean' into st…

5d69b5d

…ealth_addresses_clean

fix

bc89132

Signed-off-by: Andrei Gubarev <[email protected]>

agubarev marked this pull request as ready for review July 15, 2022 16:52

SWvheerden reviewed Jul 18, 2022

View reviewed changes

Merge branch 'tari-project:development' into stealth_addresses_clean

a683f45

sdbondi reviewed Jul 18, 2022

View reviewed changes

base_layer/wallet/src/output_manager_service/service.rs Outdated Show resolved Hide resolved

small refactor before merging with another PR

1a65870

Signed-off-by: Andrei Gubarev <[email protected]>

agubarev marked this pull request as draft July 18, 2022 12:22

agubarev marked this pull request as ready for review July 18, 2022 16:36

AaronFeickert reviewed Jul 18, 2022

View reviewed changes

base_layer/wallet/src/output_manager_service/service.rs Outdated Show resolved Hide resolved

AaronFeickert reviewed Jul 18, 2022

View reviewed changes

base_layer/wallet/src/transaction_service/service.rs Outdated Show resolved Hide resolved

reverted merge

31715a0

changed DSH label for stealth addresses to `com.tari.stealth_address` Signed-off-by: Andrei Gubarev <[email protected]>

agubarev force-pushed the stealth_addresses_clean branch from ff8cf24 to 31715a0 Compare July 19, 2022 07:28

stringhandler reviewed Jul 19, 2022

View reviewed changes

stringhandler requested a review from hansieodendaal July 19, 2022 17:44

agubarev and others added 4 commits July 20, 2022 14:03

refactored scan_outputs_for_one_sided_payments()

a063105

Signed-off-by: Andrei Gubarev <[email protected]>

Merge branch 'development' into stealth_addresses_clean

bbc8030

Merge branch 'tari-project:development' into stealth_addresses_clean

9667354

Merge branch 'development' into stealth_addresses_clean

11273b5

hansieodendaal reviewed Jul 20, 2022

View reviewed changes

ffi: added helper functions needed to test stealth addresses in cucumber

e8dcd3c

Signed-off-by: Andrei Gubarev <[email protected]>

agubarev marked this pull request as draft July 21, 2022 06:09

agubarev and others added 8 commits July 21, 2022 09:13

Merge branch 'development' into stealth_addresses_clean

f2dd1c2

# Conflicts: # base_layer/wallet/src/output_manager_service/service.rs

fix

3e4ee3a

Signed-off-by: Andrei Gubarev <[email protected]>

Merge branch 'development' into stealth_addresses_clean

20bb419

Merge branch 'development' into stealth_addresses_clean

0bf1c02

Merge branch 'development' into stealth_addresses_clean

b5d8081

cucumber test for one-sided stealth transactions

652e0e3

Signed-off-by: Andrei Gubarev <[email protected]>

cucumber test for one-sided stealth transactions

31573ae

Signed-off-by: Andrei Gubarev <[email protected]>

Merge remote-tracking branch 'origin/stealth_addresses_clean' into st…

59b02f0

…ealth_addresses_clean # Conflicts: # integration_tests/features/support/wallet_steps.js # integration_tests/features/support/world.js

agubarev marked this pull request as ready for review July 21, 2022 18:06

agubarev marked this pull request as draft July 22, 2022 06:13

stringhandler reviewed Jul 22, 2022

View reviewed changes

cleanup

e282c26

Signed-off-by: Andrei Gubarev <[email protected]>

agubarev marked this pull request as ready for review July 22, 2022 10:53

Merge branch 'development' into stealth_addresses_clean

909026f

stringhandler previously approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'development' into stealth_addresses_clean

b5b20b5

agubarev dismissed stringhandler’s stale review via 0f1c16b July 27, 2022 08:45

agubarev force-pushed the stealth_addresses_clean branch from 1daf76e to b5b20b5 Compare July 27, 2022 13:07

agubarev and others added 3 commits July 27, 2022 18:25

Merge branch 'development' into stealth_addresses_clean

3f25f56

fix: refactored code to use the new hashing API approach

32d5577

Signed-off-by: Andrei Gubarev <[email protected]>

Merge branch 'development' into stealth_addresses_clean

db8302e

stringhandler approved these changes Jul 28, 2022

View reviewed changes

stringhandler merged commit c73de62 into tari-project:development Jul 28, 2022

agubarev deleted the stealth_addresses_clean branch July 28, 2022 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for stealth addresses in one-sided transactions #4310

feat: support for stealth addresses in one-sided transactions #4310

agubarev commented Jul 14, 2022 •

edited

Loading

SWvheerden left a comment

SWvheerden Jul 18, 2022

agubarev Jul 18, 2022

stringhandler Jul 19, 2022

stringhandler Jul 19, 2022

stringhandler Jul 19, 2022

hansieodendaal Jul 20, 2022

hansieodendaal left a comment

hansieodendaal Jul 20, 2022

hansieodendaal Jul 20, 2022

hansieodendaal Jul 20, 2022

hansieodendaal commented Jul 20, 2022

Cifko commented Jul 20, 2022

stringhandler commented Jul 20, 2022

stringhandler Jul 22, 2022

stringhandler Jul 22, 2022

	let c = RistrettoSecretKey::from_bytes(
	let stealth_key = PrivateKey::from_bytes(

		@@ -32,6 +32,35 @@ Feature: Wallet Transactions
		Then all nodes are at height 30
		Then I wait for wallet WALLET_C to have at least 1500000 uT

feat: support for stealth addresses in one-sided transactions #4310

feat: support for stealth addresses in one-sided transactions #4310

Conversation

agubarev commented Jul 14, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

SWvheerden 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

hansieodendaal 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 commented Jul 20, 2022

Cifko commented Jul 20, 2022

stringhandler commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubarev commented Jul 14, 2022 •

edited

Loading