perf: avoid holding write-lock across prove()

[dan-da]

Addresses #122

note: the logic changes are in src/models/state/mod.rs and src/rpc_server.rs. All other files are just updating syntax in unit tests.

Previously a write-lock was held across create_transaction() which includes a lengthy (potentially minutes) call to prove. This would be unacceptably bad for concurrency as most p2p and rpc operations would block waiting for the lock until prove() finally finishes.

This commit makes create_transaction() take &self instead of &mut self. So it is now a read-only op.

Only a single mutation was being performed inside add_change() which is indirectly called by create_transaction(). This is now moved into send() RPC after create_transaction() completes. The write-lock acquisition is likewise moved, so it should only be held very briefly while adding a record to wallet's expected_utxos.

I am making this a draft PR because:
a) 4 tests are failing due to the change and need to be fixed.
b) I have some q's I would like to resolve about updating the wallet before I consider this ready to merge.
c) it's possible this could be done a different way or more elegantly. I don't really like that create_transaction() now returns a tuple instead of just a Transaction.

I put my q's in a comment within send(). I will expand on them here:

1. Is it really necessary to add the change Utxo to wallet's expected UTXOs when creating a Tx? Why can't the wallet be informed of change UTXO when Tx enters mempool and/or block is confirmed like any other incoming utxo? I ask because If it is not necessary, we could then avoid any mutation/write (and write lock) during send().

2. A comment in the code indicates that we add the change UTXO to pool of expected incoming UTXOs so that we can synchronize it after it is confirmed. I don't understand this. a) What is meant by "synchronize" and why wouldn't it synchronize? b) what is special about this change UTXO vs the other UTXOs in the transaction we are sending (that we don't write anywhere)?

Regarding making this commit more elegant. I created a new struct ChangeUtxoData to return data from CreateTransaction for the caller to update wallet if necessary. I'm not 100% certain this new struct is necessary. There is already UtxoReceiverData which is similar. Anyway, I would say this area is worth reviewing. And also if it turns out we don't really have to update the wallet state, then ChangeUtxoData can go away anyway.

[Sword-Smith]

About the code
This looks good. I'm glad to see that you're adding the expected change UTXO before you're sending the transaction to the main thread. That's important.

I'm a bit skeptical about new ChangeUtxoData struct. Its addition_record field can be derived from its three other fields, so that probably shouldn't be there. Maybe we can use the UtxoReceiverData struct to carry this data about the change UTXO? I think that's more elegant.

Your questions

Is it really necessary to add the change Utxo to wallet's expected UTXOs when creating a Tx? Why can't the wallet be informed of change UTXO when Tx enters mempool and/or block is confirmed like any other incoming utxo? I ask because If it is not necessary, we could then avoid any mutation/write (and write lock) during send().

A Transaction structure includes a list of outputs in the form of Vec<AdditionRecord>. An AdditionRecord is only a digest pertaining the the UTXO¹. So when you receive a new block, how do you know if any of the AdditionRecords contained therein are yours? We answer this question in two different ways: You can either be informed off-chain to expect a certain UTXO, or the UTXO can be signaled to you through public (and encrypted) information in the block.

These two options are handled by update_wallet_state_with_new_block through its calls to scan_for_announed_utxos and scan_for_expected_utxos, respectively. The change amount is currently handled as an "expected UTXO", not an announced one. So it gets stored in an ephemeral data structure, the UtxoNotificationPool.

Footnotes

1. The AdditionRecord of a UTXO is the output of the commit function with arguments commit(Hash::hash(&utxo), sender_randomness, receiver_digest). ↩

[dan-da]

Thx for the helpful explanations.

The change amount is currently handled as an "expected UTXO", not an announced one.

yes, so this is getting towards the crux of my q's. I am trying to figure out why it is being handled as an expected UTXO, and if there would be any problem changing it to an announced one instead. I am also curious why it is treated differently from other UTXO's that I send to my own wallet.

I analyze as follows:

1. UTXO received from third parties are handled via announce. (works fine)

2. UTXO sent from self to self (except for change) are handled via announce. If I have a utxo worth 5 coins and I break it up and send myself 1,1,1 with change 2, then the current code is announcing the three utxo worth 1 each, and is adding the final change utxo worth 2 to expected_utxos. But it seems all these UTXO should be treated the same: either all should be added to expected_utxos, or all should be announced.

3. change could also be handled as announce. Since it is not, I presume there is a good reason. What is that reason? My first guess is that we save some blockchain space by not making an announcement.

4. If we could use announce mechanism for all, then it would seem the expected_utxos mechanism of the wallet could be removed and the code simplified. Or is this mechanism used elsewhere somehow?

5. So we have a choice to make: do we handle all UTXO sent from self to self as expected_utxo, or as announced? I think the answer depends on what if any advantages exist for handling as expected_utxo and I don't yet have understanding there.

edit: one final observation/question: if change UTXO do not have an announcement on the blockchain as other UTXO do, then it would seem that makes them distinguishable as change to observers. That seems harmful to privacy, no? I would think it opens up some kind of avenue for analysis...

[Sword-Smith]

When a transaction is initiated, the sender gets to select a digest of randomness, which is 320 bytes, and the receiver gets to dictate a digest of randomness (communicated via the receiving address), also 320 bytes. The sender needs a way to communicate the sender-randomness to the receiver, as the receiver cannot spend the UTXO without the sender randomness. This data can be communicated off-chain (not yet supported), or it can be attached to the block and communicated in an encrypted way on-chain. The latter makes the transaction big and will thus very significantly add to the transaction fees.

Notice the fields of MonitoredUtxo

pub struct MonitoredUtxo {
    pub utxo: Utxo,

    // Mapping from block digest to membership proof
    pub blockhash_to_membership_proof: VecDeque<(Digest, MsMembershipProof)>,

    pub number_of_mps_per_utxo: usize,

    // hash of the block, if any, in which this UTXO was spent
    pub spent_in_block: Option<(Digest, Duration, BlockHeight)>,

    // hash of the block, if any, in which this UTXO was confirmed
    pub confirmed_in_block: Option<(Digest, Duration, BlockHeight)>,

    /// Indicator used to mark the UTXO as belonging to an abandoned fork
    /// Indicates what was the block tip when UTXO was marked as abandoned
    pub abandoned_at: Option<(Digest, Duration, BlockHeight)>,
}

And the sender_randomness field of the MsMembershipProof:

pub struct MsMembershipProof {
    pub sender_randomness: Digest,
    pub receiver_preimage: Digest,
    pub auth_path_aocl: MmrMembershipProof<Hash>,
    pub target_chunks: ChunkDictionary,
}

I am trying to figure out why it is being handled as an expected UTXO, and if there would be any problem changing it to an announced one instead. I am also curious why it is treated differently from other UTXO's that I send to my own wallet.

Transaction size in the block is an important consideration here. Currently, the announced UTXOs use a lot of space in the block because they are using asymmetric cryptography. Using an announcement scheme with symmetric cryptography for e.g. the change transactions, would allow the sender to save a good amount of data. See #5.

If we could use announce mechanism for all, then it would seem the expected_utxos mechanism of the wallet could be removed and the code simplified. Or is this mechanism used elsewhere somehow?

I think the opposite suggestion describes a future where Neptune is a successful blockchain better: All UTXOs will be announced offchain, and the on-chain transactions will contain no broadcast information in order to save on transaction fees.

[Sword-Smith]

By running the test models::state::wallet::address::generation_address::test_generation_addresses::scan_for_announced_utxos_test you can see how big the public announcement is, if you add a single println!() statement to it. A public announcement for a generation address, using asymmetric cryptography, is 347 BFieldElements, which is $8 * 347 = 2776$ bytes.

Alternatively you can run ct test_encrypt_decrypt -- --nocapture.

[dan-da]

Ok, got it. Short answer then is that using expected_utxos saves 2776 bytes of blockchain space per UTXO vs using a public announcement.

Which still leaves my followup question: why don't other UTXOS that I send to my own wallet also use expected_utxos?

I believe the answer probably is: they should, but that optimization has not been coded yet. Ideally create_transaction() would return a Vec and the caller would add all to wallet's expected_utxos.

@Sword-Smith please let me know if you agree. If so, I will proceed to update this PR with that solution.

Using an announcement scheme with symmetric cryptography for e.g. the change transactions, would allow the sender to save a good amount of data

wait a sec, Is this intended to replace the expected_utxos mechanism? If so, then it would seem we don't need to optimize for that mechanism now and should impl #5 instead. now I'm a bit confused again.