monero-serai: make it clear that not providing a change address is fingerprintable #472
Conversation
When no change address is provided, all change is shunted to the fee. This PR makes it clear to the caller that it is fingerprintable when the caller does this.
| } | ||
| change.address.is_subaddress() |
There was a problem hiding this comment.
Will an InternalPayment::Change exist if change.address is none?
There was a problem hiding this comment.
It shouldn't, I went with your suggestion in the other comment to not use the Option for InternalPayment::Change
coins/monero/src/wallet/send/mod.rs
Outdated
| @@ -624,7 +627,7 @@ impl SignableTransaction { | |||
| // regarding it's view key | |||
| payment = if !modified_change_ecdh { | |||
| if let InternalPayment::Change(change, amount) = &payment { | |||
| InternalPayment::Payment((change.address, *amount)) | |||
| InternalPayment::Payment((change.address.unwrap(), *amount)) | |||
There was a problem hiding this comment.
Prior comment asked about this, here unwraps.
We could also make InternalPayment::Change not internally use the Change struct, avoiding the Option entirely and offering compile-time safety.
There was a problem hiding this comment.
We could also make InternalPayment::Change not internally use the Change struct, avoiding the Option entirely and offering compile-time safety.
I thought of doing this when working on it too and decided not to. I like it though. Done in the latest.
coins/monero/tests/runner.rs
Outdated
| @@ -298,7 +298,10 @@ macro_rules! test { | |||
| rpc.publish_transaction(&signed).await.unwrap(); | |||
| mine_until_unlocked(&rpc, &random_address().2.to_string(), signed.hash()).await; | |||
| let tx = rpc.get_transaction(signed.hash()).await.unwrap(); | |||
| check_weight_and_fee(&tx, fee_rate); | |||
| if stringify!($name) != "spend_one_input_to_two_outputs_no_change" { | |||
| // Skip weight and fee check for the above test | |||
There was a problem hiding this comment.
Mind having the comment explain why?
" as if there's no change, any would-be change will be shunted to the fee" or so?
processor/src/networks/monero.rs
Outdated
| @@ -376,6 +376,12 @@ impl Monero { | |||
| }) | |||
| .collect::<Vec<_>>(); | |||
|
|
|||
| let change = Change::fingerprintable(if change.is_some() { | |||
| Some(change.clone().unwrap().into()) | |||
There was a problem hiding this comment.
if let Some(change) = change {}, to avoid the unwrap
There was a problem hiding this comment.
Did that suggestion, then the compiler warned it's equivalent to Option::map and suggested change.as_ref().map(|change| change.clone().into()). See the latest. I was wondering what the cleanest way to write this was, cool.
|
Thanks :) |
When a signable transaction is constructed without a change address specified, all change is added to the tx fee (this helps Serai avoid accumulating dust outputs that spam the chain). This PR makes it clearer to the caller that it is fingerprintable when the caller does not provide a change address, since the caller must explicitly initialize the change param with
Change::fingerprintable(None)(as is done in the added test). A tx created this way is fingerprintable because the fee is distinguishable from a fee created with wallet2.