wallet: complete hot-cold implementation for Carrot/FCMP++

[jeffro256]

Carrot/FCMP++ hot-cold wallets

Design goals

Protocol goals

• Compact exported Carrot outputs format
• Compact signed Carrot/FCMP++ transaction format (SA/L's and key image associations only)
• Deferred FCMP / BP+ proving to hot wallets at time of submission
• Allows for "state-less" transaction proposals and signing
• Allows for state-less, cold-initiated transaction proposals
• Allows for hot-cold setups for Carrot-key-hierarchy wallets
• Allows hiding of sender-receiver secrets from hot wallet

Wallet implementation goals

• New hot wallets backwards compatible with old cold wallets until FCMP++ hard fork activation
• New cold wallets backwards compatible with old hot wallets until FCMP++ hard fork activation
• User experience for hot-cold setups on CLI and RPC is unchanged before and after FCMP++ hard fork activation
• Wallet RPC interface completely backwards compatible until FCMP++ hard fork activation
• Wallet RPC interface only differs in fetching of transaction secret keys after FCMP++ hard fork activation

Testing

• Importing/exporting outputs, subaddress and coinbase included, legacy key hierarchy
• Importing/exporting outputs, subaddress and coinbase included, Carrot key hierarchy
• Carrot-key-hierarchy account signing
• State-less transaction proposals and signing
• Cold-initiated transaction proposals
  • Unit test
  • wallet2 integration tests

New quirks

• Sender-receiver secrets for Carrot/FCMP++ transactions now need to fetched by the signable transaction hash, not TXID, from the cold wallet. The hot wallet RPC returns the signable transaction hash in the submit_transfer method. This is a breaking change.
• Because finalizing transaction proposals into pruned Carrot/FCMP++ transactions requires knowing the private view-incoming key, a hot wallet without a differing private view-incoming key will cryptographically not be able to submit a signed transaction set. This shouldn't be a breaking change since the signed transaction payloads are encrypted using the private view-incoming key anyways.

Depends: #74, #75, #76, #77, #97, #98, #214, #216, #296, #404, #424, monero-project#10790

Will break this PR up into smaller pieces once the above are merged.

[jeffro256]

Legacy cold_signing tests pass now (hard-fork table not updated). Also, src/wallet2.* are about ~1500 lines smaller ;)

[ComputeryPony]

OUTPUT_EXPORT_FILE_MAGIC

[jeffro256]

Thank you

[ComputeryPony]

Not sure if it's just not implemented yet or a bug but I can almost make and use an offline wallet with this PR but I get an error when running submit_transfer at the very end.

[wallet 55WeP6]: submit_transfer
Error: unexpected error: std::get: wrong index for variant

[jeffro256]

@ComputeryPony thanks for bringing that up, it was a bug. Does the latest commit fix it? It seems to work for me now

[ComputeryPony]

@ComputeryPony thanks for bringing that up, it was a bug. Does the latest commit fix it? It seems to work for me now

Yup, that fixed it. Thanks!

[jeffro256]

Rebased against fcmp++-stage and remove usage of tools::add_element().

[j-berman]

Review part 1

I just noticed the cold_signing test is failing. #358 is passing so shouldn't be too bad to fix (I haven't looked into why).

I've reviewed the complete export / import outputs flow, and have reviewed some of the rest but haven't completed my review on the rest yet. This PR is a beast. I think there's a lot to really like here with many of the design decisions, thank you for the solid work on this.

[j-berman]

The pre-fork flow is currently borked on my end because of the following:

1. This default inserts Carrot outs into the front of m_tx.vout.

2. Causes carrot::is_carrot_transaction_v1(td.m_tx) to return true here.

3. Which incorrectly enters the Carrot flow for spending a non-Carrot out, ultimately throwing here. EDIT: tbc, I'm sure there are other spots where it's incorrectly entering the Carrot flow because of this issue, but this is where the test presently ends up throwing.

This could use some careful fixing. Presumably we should be assuming the m_tx saved on legacy wallet outs may already have incorrect/inconsistent outs since legacy wallets do the same thing. So we may want to avoid using carrot::is_carrot_transaction_v1 for m_tx, and instead we do actually want a helper to determine output type from a td. (EDIT: we could also de-serialize m_tx manually making sure it always uses the right type for the front outs, and make sure to always use the right out type)

[j-berman]

Review part 2/3

Exporting/importing key images looks good

[j-berman]

It looks like there's an inconsistency inside make_sal_opening_hint_from_transfer_details for legacy.

This section uses the additional tx pub key if it's there and the output is received to a subaddress. However, the normal scan flow detects receives using either tx pub key or additional, checking the tx pub key first. So someone could theoretically receive to a subaddress but using the tx pub key for derivation, and make_sal_opening_hint_from_transfer_details would still use the additional tx pub key.

[jeffro256]

The behavior in make_sal_opening_hint_from_transfer_details() matches the legacy transaction construction behavior: https://github.com/monero-project/monero/blob/2c48374ecd2449c02bb400e5bcf20b7c6f11649b/src/device/device_default.cpp#L325-L326. I should either modify the scanning code to reject non-standard derivation paths, or make_sal_opening_hint_from_transfer_details() will require a view key device.

[j-berman]

Right it's only in the event someone makes a tx using something other than that exact flow that this would surface as a potential issue. The problem is a sender might have done this already, and an honest recipient's wallet2 would have detected it / been able to spend it, and so I think we need to account for that.

Couple more options to consider:

• Restructure the flow to not need to do crypto scanning ops on the opening hint?
• Store an optional additional_ephemeral_tx_pubkey on the legacy opening hint as well, then downstream crypto scanning ops on legacy opening hints could use it matching it to the normal scan flow behavior.

[jeffro256]

The pre-fork flow is currently borked on my end because of the following:

1. [This default inserts](https://github.com/jeffro256/monero/blob/924338ac383ffa831d410ef8364a72508268276a/src/wallet/hot_cold.cpp#L656) Carrot outs into the front of `m_tx.vout`.

2. Causes `carrot::is_carrot_transaction_v1(td.m_tx)` to return true [here](https://github.com/jeffro256/monero/blob/924338ac383ffa831d410ef8364a72508268276a/src/wallet/tx_builder.cpp#L789).

3. Which incorrectly enters the Carrot flow for spending a non-Carrot out, ultimately throwing [here](https://github.com/jeffro256/monero/blob/924338ac383ffa831d410ef8364a72508268276a/src/wallet/tx_builder.cpp#L805-L806). EDIT: tbc, I'm sure there are other spots where it's incorrectly entering the Carrot flow because of this issue, but this is where the test presently ends up throwing.

This could use some careful fixing. Presumably we should be assuming the m_tx saved on legacy wallet outs may already have incorrect/inconsistent outs since legacy wallets do the same thing. So we may want to avoid using carrot::is_carrot_transaction_v1 for m_tx, and instead we do actually want a helper to determine output type from a td. (EDIT: we could also de-serialize m_tx manually making sure it always uses the right type for the front outs, and make sure to always use the right out type)

Dang it, this is a very good find. So we need to keep backwards-compatible support for deserializing cryptonote::txout_to_script in some form. We can do this by either A) changing the variant code for carrot::txout_to_carrot_v1 (hardfork from Beta v2), or B) maintaining parallel deserialization code for pre-FCMP++ wallet caches. I recommend option A for fcmp++-stage, since it's a much simpler code change for upstream. If we want to be lazy, and since the hot/cold feature hasn't yet dropped for the Beta network, we could simply leave it as-is for fcmp++beta-stressnet. For the very few users who have had existing hot/cold wallets on the testnet, they can simply delete their cache, and re-import their outputs. How does that sound?

[j-berman]

I recommend option A for fcmp++-stage, since it's a much simpler code change for upstream

I think that solution will work. It's a little uncomfortable there will be legacy m_tx's around with inconsistent/incorrect tx outs (a pre-existing issue), and we have a function carrot::is_carrot_transaction_v1 that works only because we think we know they're broken in this exact way and we don't expect Carrot txs will ever be broken like that, but I'm ok with chalking it up to legacy cruft.

Maybe would be nice to note somewhere that legacy m_tx's might have inconsistent and incorrect tx vouts somewhere. Don't want anyone writing a function is_legacy_transaction that checks tx outs with wrong logic. But for purposes of this PR, I think we can move forward with that solution. Thanks

[j-berman]

EDIT: whoops, ok to ignore this. I see you already did this in the latest.

For the very few users who have had existing hot/cold wallets on the testnet, they can simply delete their cache, and re-import their outputs. How does that sound?

Because beta won't include #417, I think we would need to modify this logic to fill m_tx.vout with cryptonote::txout_to_key's so that legacy outs are still spendable on beta after import. Probably worth doing that anyway on a go-forward basis anyway.

[j-berman]

Looks solid! Great work!!! Really just small comments remaining and design questions.

It's really a lot of code and not the simplest to navigate (not your fault, it's just a lot). In case it may be helpful for anyone wanting to review this code later, I wrote this guide as I worked through the call stack for the CLI for a complete Hot/Cold wallet tx construction flow:

Hot wallet

1. wallet2::create_transactions_2

   • Carrot: make_pending_carrot_tx
     • mainly handles setting sources, dests, and change
     • no ephemeral priv keys generated yet
   • Legacy: basically same exact code as before

2. wallet2::save_tx

• wallet2::dump_tx_to_str
  • wallet::cold::generate_unsigned_tx_set_from_pending_txs
    • Carrot:
      • Generates a random 32 byte hot-cold seed used to derive many tx components downstream deterministically
      • compress_carrot_transaction_proposal_lossy
        • Selects only the necessary data the cold wallet needs to sign txs
        • Calls compress_carrot_selfsend_payment_proposal_lossy, which does the same
    • Legacy: mainly just grabs expected data type
  • wallet::cold::encrypt_unsigned_tx_set
• wallet2::save_to_file
  • Saves file as unsigned_monero_tx

Cold wallet

3. wallet2::sign_tx(const std::string &unsigned_filename, const std::string &signed_filename, std::vector<wallet2::pending_tx> &txs, std::function<bool(const wallet::cold::UnsignedTransactionSetVariant&)> accept_func, bool export_raw)

• wallet2::load_unsigned_tx
  • wallet2::parse_unsigned_tx_from_str
  • wallet::cold::decrypt_unsigned_tx_set
• accept_func
  • Carrot:
    • wallet2::get_transaction_proposals_from_unsigned_tx
      • wallet::cold::make_supplemental_input_proposals_fetcher lambda
        • This makes a custom function that gets the best output to spend, and makes an opening hint for it
      • wallet::cold::get_transaction_proposals_from_unsigned_tx_set
        • expand_carrot_transaction_proposals
          • extend_supplemental_input_proposals_fetcher
            • extends lambda for best inputs also checking newly imported outputs
          • expand_carrot_transaction_proposal
            • Gets the best inputs to spend using extended lambda from above
            • expand_carrot_transaction_proposal
              • expand_carrot_normal_payment_proposal
                • Uses hot-cold seed to derive ephemeral tx priv key
              • expand_carrot_selfsend_payment_proposal
                • Uses hot-cold seed to derive ephemeral tx priv key
                • Derives address from subaddr index
  • Legacy: same as before
• wallet2::sign_tx(const wallet::cold::UnsignedTransactionSetVariant &exported_txs, const std::string &signed_filename, std::vector<wallet2::pending_tx> &txs, bool export_raw)
  • wallet2::sign_tx_dump_to_str
    • wallet2::sign_tx(const wallet::cold::UnsignedTransactionSetVariant &exported_txs, std::vector<wallet2::pending_tx> &ptx, wallet::cold::SignedTransactionSetVariant &signed_txs)
      • Legacy: wallet2::sign_tx(const wallet::cold::UnsignedPreCarrotTransactionSet &exported_txs, std::vector<wallet2::pending_tx> &txs, wallet::cold::SignedFullTransactionSet &signed_txes)
        • Imports any exported outputs into the wallet
        • wallet::cold::sign_pre_carrot_tx_set
          • Basically all same code as before pulled out into hot_cold.cpp (nice)
        • Add all tx keys to pending txs
        • Add all of the wallet's key images to the final signed txs
      • Carrot: wallet2::sign_tx(const wallet::cold::UnsignedCarrotTransactionSetV1 &exported_txs, wallet::cold::SignedCarrotTransactionSetV1 &signed_txs)
        • Imports new outputs and sets signed_txs.other_key_images (which are the newly imported outputs' key images)
        • wallet::cold::sign_carrot_tx_set_v1
          • Uses the same lambda as the one implemented above to get the best output to spend and make an opening hint for it (Why's this duplicated?)
          • expand_carrot_transaction_proposal_and_rerandomized_outputs
            • Re-calls expand_carrot_transaction_proposal
            • expand_rerandomization_scalars
              • Uses hot-cold seed to derive scalars used for re-randomized output for each input
            • carrot::get_sorted_input_key_images_from_proposal_v1
            • carrot::get_output_enote_proposals_from_proposal_v1
              • Gets all output enotes from the proposal using a Carrot core function, all randomness is expected set by this point including ephemeral priv keys
            • Offsets the last r_c in the re-randomized output to make sure we'll have a balanced pedersen commitment.
              • sums output amount blinding factors
              • subtracts input amount blinding factors
              • subtracts all re-randomized r_c except the last
              • now left with what the last r_c should become
            • Uses balanced re-randomized scalars to calculate final re-randomized outputs (includes the tx's pseudo out)
          • try_sign_carrot_transaction_proposal_v1
            • Makes the SAL proofs
        • Imports ephemeral priv keys into the wallet.
          - Note: FCMP++ doesn't import the priv keys tied to tx id here, but to signable tx hash.
    • wallet::cold::encrypt_signed_tx_set
  • wallet2::save_to_file
    • Saves file as signed_monero_tx

Hot wallet

4. wallet2::load_tx
   • wallet2::load_from_file

• wallet2::parse_tx_from_str
  • wallet::cold::decrypt_signed_tx_set
  • Prepares tx from a str
    • Legacy: same
    • Carrot: wallet::cold::finalize_signed_carrot_tx_set_v1_into_full_set
      • Does 1 million collections again
      • expand_carrot_transaction_proposal_and_rerandomized_outputs
        • Does all the re-expansion done above, using hot-cold seed as before for all the randomness
      • get_output_enote_proposals_from_proposal_v1
        • More expansions / copies / crypto ops
      • finalize_fcmps_and_range_proofs
        • Shared code with normal tx flow for getting membership proof with tree cache, constructing FCMP++, and constructing BP+
      • make_pending_carrot_tx
        • Pre-existing code for constructing the final pending tx ready for submission to the daemon
    • Sets all the imported wallet's key images on full_signed_txs
• accept_loaded_tx
  • Takes in the final signed tx and prints useful info back to user.
• Imports all key images both from the signed txs, and any new ones exported by the cold wallet.

5. wallet2::commit_tx

• Basically same as before, just submits the tx to the daemon.
• The one main difference here is that for FCMP++, the hot wallet also imports tx keys indexed by signable tx hash, in addition to the tx hash.

[j-berman]

nit: This seems like it would only be reachable potentially for full wallets (since hot wallets set the tx key on pending txs to identity in finalize_signed_carrot_tx_set_v1_into_full_set). Do you have a use case in mind for this section?

[jeffro256]

This unifies behavior between full and cold wallets: tx keys from both can be fetched using the signable tx hash. So upstream users can switch to only using signable tx hash to fetch tx keys and it'll work for everything.

[j-berman]

Context: the cold wallet calls this function to import an unsigned tx. Then after calling this, the cold wallet signs the tx.

For the Carrot flow, this function appears to be doing everything that sign_carrot_tx_set_v1 then does again after this. Specifically the call to expand_carrot_transaction_proposal is duplicated, which seems unnecessary.

[j-berman]

Checking my understanding: you "compress" to strictly data the cold wallet needs in order to construct the tx. The "random" fields are re-derived using the hot_cold_seed, so those fields don't need to be exported to the cold wallet. So you strip those fields from the proposal, hence "lossy."

I'm finding it a little confusing/hard to see what fields from the tx proposal that the hot wallet really didn't even need in the first place at this stage. Maybe it would be nice if the hot wallet was passing around a type that already didn't have the extraneous types to reduce cognitive load here/room for error, but I get how that might be a significant restructuring.

[jeffro256]

One of the core design decisions of CarrotTransactionProposalV1 is that it contains everything needed to reconstruct a signable transaction hash, which is what the SA/L proof binds to. This means that e.g. HW devices talking to hot devices can "talk" to each other with a cold signing protocol, and come to a shared 100% exact same description of all parts of a tx that the signature signs for. HotColdCarrotTransactionProposalV1 does not adhere to this. For one, it doesn't have full opening hints like CarrotTransactionProposalV1 has, rather, it has one-time addresses references to some inferred set of opening hints.

[jeffro256]

Also, the fact that a view-key device is required to view output information is not ideal for some the call sites elsewhere. In other words, I think that trying to use HotColdCarrotTransactionProposalV1 everywhere would shift a lot of the mess somewhere else. I like that it is compartamentalized here.

[j-berman]

So in the CLI, a cold wallet importing an unsigned tx set expands the Carrot tx proposals here when importing the unsigned tx set (accept_loaded_tx calls get_transaction_proposals_from_unsigned_tx_set).

And then it does it again when signing the unsigned tx set (sign_carrot_tx_set_v1 calls expand_carrot_transaction_proposal_and_rerandomized_outputs).

It seems like a major duplication of logic that also adds a lotta surface to the code. Any room here to avoid that duplication/only expand once?

[j-berman]

I'm wondering if cold wallets not being able to query for keys using tx id might lead to some disrupted flows. I think this is fine as is, but maybe we'll want signable tx hashes to be a more prominently featured item in the future (displayed in wallets / block explorer / queryable from the wallet) to work around this later on

[jeffro256]

Yeah it probably will. It's the one breaking API change AFAIK in this whole chageset. At least one the live side, if you have the TXID, you can lookup its signable tx hash...

[jeffro256]

Rebased due to merge of #424

[plowsof]

i have been able to create , sign and broadcast transactions on stressnet with an implementation of this, not a detailed test/review but i just wanted to note something annoying about walet2 and unsigned tx set : it will stuff any unknown outputs into the unsigned set (for convenience presumably) so if you e.g. restore a large wallet using the view-key - your first cold signing will create a massive file - then the following tx's wont - like a force key image export/import. it seems that we are not doing this here? only the outputs that to be signed are included in the unsigned set yes?

[jeffro256]

@plowsof It should at least export all unspent outputs on the sign request, by default. Did your test wallet have a lot of spent outputs, but few unspent?

[plowsof]

@plowsof It should at least export all unspent outputs on the sign request, by default. Did your test wallet have a lot of spent outputs, but few unspent?

yep and i had used the same cache, indeed, and the idea to save space on unsigned tx set size by attempting to spend unverified outputs could only be done on a local node (trusted) as it would reveal too much for convenience.