Implement CoinJoin requests #2577
Conversation
andrewkozlik
added
core
|
|
||
| if authorization.params.coin_name != tx.coin_name: | ||
| raise wire.DataError("Coin name does not match authorization.") |
There was a problem hiding this comment.
I removed this, because it's not needed. Already checked in approve_tx() by calling self.authorization.approve_sign_tx(tx_info.tx).
common/protob/messages-bitcoin.proto
Outdated
| * Signing request for a CoinJoin transaction. | ||
| */ | ||
| message CoinJoinRequest { | ||
| required uint32 fee_rate = 1; // coordination fee rate in units of 10^-8 percent |
There was a problem hiding this comment.
This overflows uint32 with 43 percent or more - not probable, but we might change it to uint64 as protection.
There was a problem hiding this comment.
reasonable.
(fwiw the firmware currently still ignores uint sizes so this is not an issue; however, that is going to be fixed at some point)
There was a problem hiding this comment.
Firstly, a 43 % fee is crazy huge. However, I understand that we might want to represent percentages in other protobuf messages in the future and it would be nice to use a consistent format. The present scheme already allows 8 decimal digits of a percent, which seems way more precise than we might ever need. So rather than doubling the size of the data type, which would result in a maximum value of 184 billion %, I'd opt for decreasing the precision. If we go with 10–7 or 10–6, then that can fit 429% or 4295% respectively and the precision is still great. We will probably never need more than 4 digits of percent precision anyway.
BTW note that the fee rate is specified in two places in protobuf AuthorizeCoinJoin.max_coordinator_fee_rate and CoinJoinRequest.fee_rate, so we definitely want to keep those consistent.
When CoinJoin was first implemented in July 2020, we had it at 10–9, see #1127. That means a maximum of 4.29 % could be represented. The meaning was also different (fee per anonymity unit vs. flat fee rate now). I recall that we discussed this at the standup back then and the argument was that 4 % (per anonymity unit) is huge.
In #2180 I changed the meaning to the flat fee rate and 8 digit precision to partially compensate the difference in meaning.
This is probably the last chance to easily change the precision, if we want to. It will also require changes in connect. After connect is deployed, it would probably be too complicated.
There was a problem hiding this comment.
I would vote for decreasing the precision to 10–6, it could also be better rememberable - "a million is one percent" (now it is "a BTC in satoshis is one percent").
There was a problem hiding this comment.
+1 for 10^-6 precision (ppm) - ppm is also the unit how fee rate is specified in lightning (LND). Let's not forget to change the max_coordinator_fee_rate too.
There was a problem hiding this comment.
@prusnak interesting point, but I think there is a difference. Here we are talking about precision of 10^-6 of a percent, whereas in LND I understand the precision to be 10^-6 of the whole. In other words if we want to follow LND, then we should be talking about 10^-4 of a percent here.
There was a problem hiding this comment.
So what will it be? 10^-4 (ppm, LND), 10^-6 (millionth of a percent), 10^-8 (current) etc.? To be honest, I have no strong preference. Anything between 4 and 8 is fine by me.
There was a problem hiding this comment.
10^-6 seems like a good middle ground.
Done in 52fca8c
[no changelog]
andrewkozlik
force-pushed
the
andrewkozlik/coinjoin-request2
branch
from
fa690b8
to
d48cc02
Compare
We decided to strengthen the security guarantees that will be provided by the signed affiliate data in CoinJoin. Due to these changes it is becoming a little more complicated to reuse the solution that is based on payment requests, although technically it could still be done if we wanted to. Payment requests are meant for individual outputs or sets of outputs in a transaction, whereas the affiliate data is now working with the entire set of inputs and outputs. I'd call this a "transaction request" and in this particular case a
CoinJoinRequest. Since there is only one such transaction request per transaction it makes sense to put it intoSignTx, which is what I did here.There is an optional commit d547584 that I could add, which adds a fee tolerance of 200 sats on Mainnet. The rationale behind it is to prevent Trezor from declining to sign the transaction in case of some sort of rounding error in the fee computation. That would ruin the CoinJoin for all participants. But I think this shouldn't occur. Opinions?
@onvej-sl please review and feel free to comment on the CoinJoin request digest algorithm and the mask computation. It's implemented in three places actually (core, unit tests and device tests).
make_coinjoin_request()intests/device_tests/bitcoin/payment_req.pyis pretty well legible.@matejcik you might want to glance at the protobuf changes in 1de8f10, but I think it's in line with what we discussed IRL.