Add view tags to outputs to reduce wallet scanning time #8061

j-berman · 2021-11-15T14:15:25Z

Overview

Implements view tags as proposed by @UkoeHB in in MRL issue monero-project/research-lab#73

At tx construction, the sender adds a 1-byte view tag to each output. The view tag is derived from the sender-receiver shared secret (view tag = first byte of hash(shared secret) . When scanning for outputs, the receiver can check the view tag for a match in order to reduce scanning time. When the view tag matches (expected match 1/256 outputs), the wallet proceeds to derive the output public key to check if the output belongs to the receiver. When the view tag does not match, the wallet avoids the more expensive EC operations when deriving the output public key.¹ In tests on my machine, I saw reductions in scanning time upwards of 40% (and a minimum around 30%).

Update: switched to using cn_fast_hash to hash the shared secret (edit: see discussion below for more on this choice)
As suggested by @UkoeHB, the shared secret is hashed using SipHash 2-4 (the default), a keyed hash function that is specifically designed to protect a secret key and yield collisions, and is speedier than cn_fast_hash.

In order to maintain transaction uniformity, view tags are not supported until the fork height. After the fork height, all outputs must have a view tag. There is a grace period that allows both types at the fork height which allows the pool to clear without kicking tx's people create right at the fork height.

¹ It's expected the skippable EC operations would be skipped for approximately 99.6% of outputs (expected false positive rate = 1/2⁸ = 1/256 = 0.4%, therefore expected negative rate = 100% - 0.4% = 99.6%). This is why 1 byte is the optimal sweet spot for max performance gains without needing more bytes. Skipping just 0.4% more outputs does not yield noticeable gains.

Choices I made worth pointing out

For starters, I thought through various approaches deeply, and settled on what I feel is the sanest one. However, I'm absolutely prepared to refactor if necessary. I understand the various sections of the code that touch this fairly well enough at this point, would be happy to refactor if there is a stronger approach.

Added a new `tx_out_to_tagged_key` boost variant type, to replace `tx_out_to_key` at the next fork height

This was likely the most significant choice I made. It seemed to be the most sensible way to fit view tags into the code without introducing overly cumbersome structural changes across the code. I think the biggest issue with this approach is in JSON parsing. Example: on the chain today (with no view tags), the tx.vout target serializes to JSON as follows:

{ 
  "target": {
    "key": "ea3f..."
  }
}

In this PR, at the fork height when view tags are set to be supported, the tx.vout target starts serializing to JSON as follows:

{ 
  "target": {
    "tagged_key": {
      "key": "ea3f...",
      "view_tag": "a1"
    }
  }
}

This will be a breaking change for anyone who was relying on target.key, which I figure isn't ideal. I figure ideally it would instead serialize to the following:

{ 
  "target": {
    "key": "ea3f...",
    "view_tag": "a1"
  }
}

Since view tags aren't strictly necessary when reading for the key, it seemed ideal to maintain the prior JSON structure if possible, and not force people to change how they consume the JSON object downstream if they don't need to. But I couldn't get this ideal approach working in a clean way. Perhaps I'm missing something simple.

Generally very much so open to thoughts on this tx_out_to_tagged_key approach.

2 pre-rct tests aren't working

There are 2 pre-rct tests meant to test view tags on pre-rct outputs after the fork height that aren't working, because the tests aren't set up to support pre-rct outputs past v12, and I felt I spent enough time trying to get them to work. I tested it manually by mining pre-rct outputs, and calling sweep_unmixable after the fork height and scanning for them in a wallet making sure they used the view tags as expected, and they did. I figure priority-wise makes sense to move back over to binning/decoy selection work over getting these 2 minor tests working at this point. I added 15 working tests in total, not including those 2 pre-rct ones.

Still to-do

There will be some merge conflicts with Bulletproof+ by sarang, tied to consensus #7170. I'm happy to take care of them.
Update blockchain_utilities for tx_out_to_tagged_key.
anything else?

EDIT: clarified choice to use siphash + description of view tags

UkoeHB · 2021-11-15T14:22:23Z

There are 2 pre-rct tests meant to test view tags on pre-rct outputs after the fork height that aren't working, because the tests aren't set up to support pre-rct outputs past v12, and I felt I spent enough time trying to get them to work.

Does this imply a new transaction format for all currently-active transaction types, which includes coinbase, the main RCTTypeCLSAG type, the pre-RCT-to-RCT transition type, and the denomination-to-pre-RCT transition type? How is versioning handled around this?

j-berman · 2021-11-15T14:48:50Z

There are 2 pre-rct tests meant to test view tags on pre-rct outputs after the fork height that aren't working, because the tests aren't set up to support pre-rct outputs past v12, and I felt I spent enough time trying to get them to work.

Does this imply a new transaction format for all currently-active transaction types, which includes coinbase, the main RCTTypeCLSAG type, the pre-RCT-to-RCT transition type, and the denomination-to-pre-RCT transition type? How is versioning handled around this?

Yes, all currently used types. All newly created outputs that could previously be of type tx_out_to_key, transition to type tx_out_to_tagged_key at the fork height. Unless I missed a spot in the code.

On versioning: at the fork height, the wallet (and miners) will start to construct outs of type tx_out_to_tagged_key instead of tx_out_to_key. At consensus, tx_out_to_key outputs are rejected at the fork height. When reading transaction data (and scanning for outputs) in the wallet, there is a helper function that first gets the boost variant type of the output, then gets the output public key.

In the code before this PR, when reading the output for a public key, there were these all over the code base: boost::get<txout_to_key>(tx.vout[n].target).key. I consolidated the boost getter into a helper function get_output_public_key that checks the boost variant type, then uses the correct getter to return the output public key.

~~EDIT: outputs don't have to be tx_out_to_tagged_key at the fork height, but tx_out_to_key types become invalid.~~ (incorrect, ignore)

sboulden · 2021-11-15T18:00:55Z

Hi, early this year I (intentionally) sent myself a very substantial amount of Monero with a 1000000 block lock time. There is still ~3 years remaining on this transaction being unlocked.

I realize this locked_transaction feature is getting removed. I was ensured that my transaction would still be unaffected.
Can you confirm that this hardfork, which mandates a change to all transaction types, will not affect my transaction (which is a completed transaction, with thousands of confirmations, but is still pending unlock).

Thank you

j-berman · 2021-11-15T18:28:58Z

@sboulden your transaction will still be unaffected. This PR doesn't have any impact on locked outputs, or the locked_transfer feature. This PR would only impact newly created outputs at the fork height. I also sanity checked it locally just to be absolutely certain, your Monero will be just fine :)

The proposal to deprecate the locked_transfer feature is over here: monero-project/research-lab#78. No code has been written to move forward on that. And even if deprecation does move forward in this fork or in some future fork, to my knowledge it was never considered to render outputs un-spendable (or to mess with existing locked outputs at all). No one has proposed altering existing locked outputs in any way to my knowledge.

sboulden · 2021-11-15T18:37:53Z

I appreciate you confirming! Sometimes my mind gets worried that the impact on locked_transfers could be easily overlooked, so I just like to shout it out there.

Thanks again for your work on this pull, looking forward to it

src/crypto/crypto.cpp

src/cryptonote_basic/cryptonote_format_utils.cpp

src/cryptonote_config.h

UkoeHB

Can you add the test vectors for siphash? They can be found in my seraphis_perf branch.

src/cryptonote_core/blockchain.cpp

src/cryptonote_core/cryptonote_tx_utils.cpp

UkoeHB · 2021-11-15T20:24:07Z

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -628,7 +634,8 @@ namespace cryptonote
        }
      }

-      bool r = construct_tx_with_tx_key(sender_account_keys, subaddresses, sources, destinations, change_addr, extra, tx, unlock_time, tx_key, additional_tx_keys, rct, rct_config, msout);
+      bool shuffle_outs = true;


Ah, because the default value can't be used with an explicit use_view_tags. These function signatures are disturbing, but that's not your fault.

src/wallet/wallet2.cpp

selsta · 2021-11-16T00:31:17Z

That's what I get with performance tests. Are these the 2 pre-rct tests you mentioned above?

test_out_can_be_to_acc<false, true> (1000 calls) - OK: 113 µs/call
test_out_can_be_to_acc<true, false> - FAILED
test_out_can_be_to_acc<true, true> - FAILED

tevador · 2021-11-16T07:03:28Z

I'd like to point out that there may be a reason to have view tags slightly larger than 8 bits (possibly as a future upgrade).

src/crypto/crypto.cpp

src/cryptonote_basic/cryptonote_format_utils.cpp

j-berman · 2021-11-16T13:48:45Z

That's what I get with performance tests. Are these the 2 pre-rct tests you mentioned above?
test_out_can_be_to_acc<false, true> (1000 calls) - OK: 113 µs/call
test_out_can_be_to_acc<true, false> - FAILED
test_out_can_be_to_acc<true, true> - FAILED

@selsta these failing perf tests should be working now :)

For reference, the pre-rct tests I was referencing in the description are:

gen_rct_tx_pre_rct_has_no_view_tag_from_hf_view_tags
gen_rct_tx_pre_rct_has_view_tag_from_hf_view_tags

They're commented out and don't run. Slapped a TODO on them

jtgrassie · 2021-11-16T18:25:59Z

the shared secret is hashed using SipHash

I'm interested to know why you are not using cn_fast_hash, like practically everything else in the code does? I'd expected to see something like t=H(salt|D)[0] (with H being cn_fast_hash) in your derive_view_tag.

UkoeHB · 2021-11-16T19:13:30Z

I'm interested to know why you are not using cn_fast_hash, like practically everything else in the code does?

In testing for view tags, I found using siphash led to about 1.5% faster scanning than cn_fast_hash.

jtgrassie · 2021-11-16T20:08:23Z

In testing for view tags, I found using siphash led to about 1.5% faster scanning than cn_fast_hash.

Sure, but given the overall scan time improvement, is introducing another hash function implementation worth this extra 1.5% speed? The other implication that comes to mind of using siphash here is that key security is being reduced to 2^128. cn_fast_hash just seemed the obvious candidate given we're already using it everywhere else.

UkoeHB · 2021-11-16T20:21:07Z

Sure, but given the overall scan time improvement, is introducing another hash function implementation worth this extra 1.5% speed?

This PR is likely to be permanent and final, so yes it is worth it to get as much speed as possible while we still can (setting aside assembly-level optimizations).

The other implication that comes to mind of using siphash here is that key security is being reduced to 2^128. cn_fast_hash just seemed the obvious candidate given we're already using it everywhere else.

Ed25519 security is 2^128, there is no fundamental reduction here. Also note that view tags are only 1 byte, so no more than 8 bits can be leaked even in the worst case. (unless @tevador's idea is adopted)

jtgrassie · 2021-11-16T20:40:35Z

This PR is likely to be permanent and final

? I wouldn't be so quick to use absolute statements like this. There's very little in this project that remains "permanent and final".

Ed25519 security is 2^128

Ed25519 security is at least 2^128. siphash is at most 2^128. Whilst I accept your point that only 1 byte can ever be leaked, that 1 byte is enough to fingerprint, and it's this hashing of the shared secret that is preventing the fingerprint.

tevador · 2021-11-16T20:47:29Z

Using siphash to calculate the view tags perfectly matches the intended use case of the hash function: hashtables. View tags are essentially transforming the list of outputs into many hashtables with the hash keys being shared between the sender and the receiver.

key security is being reduced to 2^128

Any hash (even keccak) truncated to 8 bits will have a security of at most 8 bits against preimages and 4 bits against collisions. But even if the whole output was used, siphash doesn't leak any information about he secret key. Even a reduced siphash-1-2 variant requires at least 2^98 operations to recover the secret key.

UkoeHB

Thank you!

jtgrassie · 2021-11-18T14:54:20Z

src/crypto/crypto.cpp

+    memwipe(siphash_key, 16);
+
+    // only need a slice of view_tag_full to realize optimal perf/space efficiency
+    static_assert(sizeof(crypto::view_tag) <= sizeof(view_tag_full));


The message parameter is only optional since c++17 and the codebase required minimum is c++14.

vtnerd · 2021-11-19T20:12:22Z

I'm with @jtgrassie on the siphash discussion. IIRC, the intended use case was/is hashtables, not cryptographic usage. Truncating a sha3 hash is safer, and a 1.5% penalty is low.

vtnerd · 2021-11-19T20:36:19Z

@tevador to follow up on that argument - cryptographic hash functions have a requirement against inversion (pre-image resistance). This is a (lesser?) requirement for siphash - the preimage resistance might be ~2^64. This is critical because it relates to privacy.

I don't see a good argument for siphash, unless there is some evidence that pre-image resistance is higher.

UkoeHB · 2021-11-19T20:42:15Z

@tevador to follow up on that argument - cryptographic hash functions have a requirement against inversion (pre-image resistance). This is a (lesser?) requirement for siphash - the preimage resistance might be ~2^64. This is critical because it relates to privacy.

I don't see a good argument for siphash, unless there is some evidence that pre-image resistance is higher.

Pre-image resistance is irrelevant here, because the hash message is public knowledge. Siphash is a keyed hash function, and the key in our case is 16 bytes of the sender-receiver derivation.

SChernykh · 2022-03-29T13:08:01Z

src/cryptonote_basic/cryptonote_format_utils.cpp

+      }
+      else if (hf_version < HF_VERSION_VIEW_TAGS)
+      {
+        // require outputs to be of type txout_to_key


Did you check that all historical tx outputs are of type txout_to_key? You can check by syncing the node running this code from scratch.

Yep + I synced from scratch running this code. Going to try it again to be safe.

SChernykh · 2022-03-29T13:28:13Z

@j-berman please read https://www.reddit.com/r/Monero/comments/jdh5to/psa_a_bug_has_caused_some_nodes_to_get_stuck_on/g98nuwl/?utm_source=reddit&utm_medium=web2x&context=3
I couldn't find where you limit mempool transactions to view tags only starting from HF_VERSION_VIEW_TAGS. check_output_types still allows old transactions in the mempool during the grace period and we can end up with the same fiasco we had with CLSAG/MLSAG hardfork.

j-berman · 2022-03-29T17:16:28Z

@SChernykh

#7169 in combination with this PR's code ejects txout_to_key tx's from the pool once hf_version > HF_VERSION_VIEW_TAGS (via m_tx_pool.validate -> add_tx (after take_tx) -> check_tx_outputs -> check_output_types). During the grace period when hf_version == HF_VERSION_VIEW_TAGS, both output types are explicitly allowed into the pool and can be mined. It's not until hf_version > HF_VERSION_VIEW_TAGS that the txout_to_tagged_key becomes the only allowed output type.

SChernykh · 2022-03-29T17:20:52Z

Nice, so this issue was fixed properly shortly after the CLSAG fork. That means you don't have to address it in this PR at all.

j-berman · 2022-04-04T05:06:23Z

Rebased to master. Resolved conflicts in:

tests/crypto/main.cpp
tests/crypto/tests.txt

@UkoeHB

Implements view tags as proposed by @UkoeHB in MRL issue monero-project/research-lab#73 At tx construction, the sender adds a 1-byte view tag to each output. The view tag is derived from the sender-receiver shared secret. When scanning for outputs, the receiver can check the view tag for a match, in order to reduce scanning time. When the view tag does not match, the wallet avoids the more expensive EC operations when deriving the output public key using the shared secret.

monero-project/monero#8061

- monero-project/monero#8061 - also update import location for epee::misc_utils::get_gmt_time

j-berman · 2022-06-24T09:16:48Z

@tevador I'd like to include you in the bounty payout for providing useful input on this PR. I've been able to contact all other reviewers; not sure how best to reach you.

- monero-project/monero#8061 - also update import location for epee::misc_utils::get_gmt_time

j-berman force-pushed the view-tag branch from e9016b9 to 437ad18 Compare November 15, 2021 15:00

UkoeHB reviewed Nov 15, 2021

View reviewed changes

src/crypto/crypto.cpp Outdated Show resolved Hide resolved

UkoeHB reviewed Nov 15, 2021

View reviewed changes

src/cryptonote_basic/cryptonote_format_utils.cpp Outdated Show resolved Hide resolved

UkoeHB reviewed Nov 15, 2021

View reviewed changes

src/cryptonote_basic/cryptonote_format_utils.cpp Outdated Show resolved Hide resolved

UkoeHB reviewed Nov 15, 2021

View reviewed changes

src/cryptonote_config.h Show resolved Hide resolved

UkoeHB suggested changes Nov 15, 2021

View reviewed changes

UkoeHB reviewed Nov 16, 2021

View reviewed changes

src/crypto/crypto.cpp Outdated Show resolved Hide resolved

src/cryptonote_basic/cryptonote_format_utils.cpp Outdated Show resolved Hide resolved

j-berman force-pushed the view-tag branch from 3f9f4ef to 9d9d2c6 Compare November 17, 2021 23:52

UkoeHB approved these changes Nov 18, 2021

View reviewed changes

j-berman force-pushed the view-tag branch from d5ac6fa to a06213a Compare November 18, 2021 03:35

jtgrassie reviewed Nov 18, 2021

View reviewed changes

Rucknium mentioned this pull request Nov 18, 2021

Open Research Questions monero-project/research-lab#94

Open

sethforprivacy mentioned this pull request Nov 19, 2021

Monero v15 hard-fork planning monero-project/meta#630

Closed

j-berman force-pushed the view-tag branch from 22e5386 to dbb228a Compare February 21, 2022 15:46

rbrunner7 mentioned this pull request Mar 27, 2022

Monero Dev Meeting: v15 network upgrade - Sat 2 April 2022 @ 17:00 UTC monero-project/meta#680

Closed

SChernykh reviewed Mar 29, 2022

View reviewed changes

SChernykh approved these changes Mar 29, 2022

View reviewed changes

SChernykh mentioned this pull request Apr 2, 2022

v15 hardfork changes SChernykh/p2pool#144

Merged

UkoeHB mentioned this pull request Apr 4, 2022

Monero Multisig for HF #8237

Closed

j-berman force-pushed the view-tag branch from dbb228a to 7b9a7e8 Compare April 4, 2022 02:27

SChernykh approved these changes Apr 4, 2022

View reviewed changes

UkoeHB approved these changes Apr 4, 2022

View reviewed changes

SChernykh mentioned this pull request Apr 16, 2022

Optimized keccak implementation #8262

Merged

j-berman force-pushed the view-tag branch from 7b9a7e8 to cef125f Compare April 16, 2022 20:42

UkoeHB approved these changes Apr 17, 2022

View reviewed changes

j-berman force-pushed the view-tag branch from cef125f to ea87b30 Compare April 18, 2022 08:04

UkoeHB approved these changes Apr 18, 2022

View reviewed changes

luigi1111 merged commit 96758a7 into monero-project:master Apr 20, 2022

plowsof mentioned this pull request Apr 20, 2022

Upcoming hard fork [view tags?] monero-ecosystem/monero-python#113

Closed

j-berman mentioned this pull request Apr 21, 2022

Update for view tags in next hf moneroexamples/onion-monero-blockchain-explorer#266

Merged

plowsof mentioned this pull request Apr 21, 2022

Upcoming hard fork [view tags / identify outputs] moneroexamples/xmregcore#5

Open

j-berman added a commit to j-berman/monero-lws that referenced this pull request Apr 27, 2022

Add support for view tags when scanning

3cdc45c

monero-project/monero#8061

j-berman added a commit to j-berman/monero-lws that referenced this pull request Apr 27, 2022

Add support for view tags when scanning

3aa0a9e

- monero-project/monero#8061 - also update import location for epee::misc_utils::get_gmt_time

selsta mentioned this pull request Jun 20, 2022

feat(trezor): add HF15 support, BP+ #8299

Merged

ph4r05 mentioned this pull request Jun 20, 2022

fix(xmr): add missing view_tags to hf15 trezor/trezor-firmware#2345

Merged

j-berman mentioned this pull request Jun 29, 2022

View_tag #8406

Closed

vtnerd pushed a commit to vtnerd/monero-lws that referenced this pull request Jul 25, 2022

Add support for view tags when scanning

271f0dc

- monero-project/monero#8061 - also update import location for epee::misc_utils::get_gmt_time

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add view tags to outputs to reduce wallet scanning time #8061

Add view tags to outputs to reduce wallet scanning time #8061

j-berman commented Nov 15, 2021 •

edited

Loading

UkoeHB commented Nov 15, 2021 •

edited

Loading

j-berman commented Nov 15, 2021 •

edited

Loading

sboulden commented Nov 15, 2021

j-berman commented Nov 15, 2021

sboulden commented Nov 15, 2021

UkoeHB left a comment

UkoeHB Nov 15, 2021

selsta commented Nov 16, 2021

tevador commented Nov 16, 2021

j-berman commented Nov 16, 2021

jtgrassie commented Nov 16, 2021

UkoeHB commented Nov 16, 2021 •

edited

Loading

jtgrassie commented Nov 16, 2021

UkoeHB commented Nov 16, 2021

jtgrassie commented Nov 16, 2021

tevador commented Nov 16, 2021

UkoeHB left a comment

jtgrassie Nov 18, 2021

vtnerd commented Nov 19, 2021

vtnerd commented Nov 19, 2021

UkoeHB commented Nov 19, 2021 •

edited

Loading

SChernykh Mar 29, 2022

j-berman Mar 29, 2022

SChernykh commented Mar 29, 2022 •

edited

Loading

j-berman commented Mar 29, 2022

SChernykh commented Mar 29, 2022

j-berman commented Apr 4, 2022

j-berman commented Jun 24, 2022

Add view tags to outputs to reduce wallet scanning time #8061

Add view tags to outputs to reduce wallet scanning time #8061

Conversation

j-berman commented Nov 15, 2021 • edited Loading

Overview

Choices I made worth pointing out

Added a new tx_out_to_tagged_key boost variant type, to replace tx_out_to_key at the next fork height

2 pre-rct tests aren't working

Still to-do

UkoeHB commented Nov 15, 2021 • edited Loading

j-berman commented Nov 15, 2021 • edited Loading

sboulden commented Nov 15, 2021

j-berman commented Nov 15, 2021

sboulden commented Nov 15, 2021

UkoeHB left a comment

Choose a reason for hiding this comment

UkoeHB Nov 15, 2021

Choose a reason for hiding this comment

selsta commented Nov 16, 2021

tevador commented Nov 16, 2021

j-berman commented Nov 16, 2021

jtgrassie commented Nov 16, 2021

UkoeHB commented Nov 16, 2021 • edited Loading

jtgrassie commented Nov 16, 2021

UkoeHB commented Nov 16, 2021

jtgrassie commented Nov 16, 2021

tevador commented Nov 16, 2021

UkoeHB left a comment

Choose a reason for hiding this comment

jtgrassie Nov 18, 2021

Choose a reason for hiding this comment

vtnerd commented Nov 19, 2021

vtnerd commented Nov 19, 2021

UkoeHB commented Nov 19, 2021 • edited Loading

SChernykh Mar 29, 2022

Choose a reason for hiding this comment

j-berman Mar 29, 2022

Choose a reason for hiding this comment

SChernykh commented Mar 29, 2022 • edited Loading

j-berman commented Mar 29, 2022

SChernykh commented Mar 29, 2022

j-berman commented Apr 4, 2022

j-berman commented Jun 24, 2022

j-berman commented Nov 15, 2021 •

edited

Loading

Added a new `tx_out_to_tagged_key` boost variant type, to replace `tx_out_to_key` at the next fork height

UkoeHB commented Nov 15, 2021 •

edited

Loading

j-berman commented Nov 15, 2021 •

edited

Loading

UkoeHB commented Nov 16, 2021 •

edited

Loading

UkoeHB commented Nov 19, 2021 •

edited

Loading

SChernykh commented Mar 29, 2022 •

edited

Loading