Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CoinJoin requests #2564

Closed
wants to merge 7 commits into from
Closed

Conversation

andrewkozlik
Copy link
Contributor

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 I think 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 into SignTx, which is what I did here.

In the future we might want to add other types of transaction requests. In that case we should be able to change

optional CoinJoinRequest coinjoin_request = 13;

to

oneof TransactionRequest {
    CoinJoinRequest coinjoin_request = 13;
    AbcRequest abc_request = 14;
}

and maintain binary compatibility, see https://developers.google.com/protocol-buffers/docs/proto3#oneof.

The CoinJoinRequest contains two bitfields required bytes remixed_inputs and required bytes signable_inputs. The size of each of these fields is proportional to the number of inputs, e.g. 300 inputs implies ceil(300/8) = 38 bytes. In terms of scalability it seems more correct to put this information into TxInput as two fields optional bool coinjoin_remix and optional bool coinjoin_signable or merged into a single field optional int coinjoin_flags with possible values 0, 1, 2, 3. These fields should then be present if and only if a CoinJoinRequest is present in SignTx. The current solution on the other hand puts all the information in one place.

I'd like to hear opinions about the protobuf design mainly from @matejcik:

  1. Are we bloating the SignTx with too much information by adding this? We could devise a way to send the information in a separate message.
  2. Which of these options is most preferable?
    • CoinJoinRequest.remixed_inputs and CoinJoinRequest.signable_inputs
    • TxInput.coinjoin_remix and TxInput.coinjoin_signable
    • TxInput.coinjoin_flags

I am still considering implementing this via the existing payment request mechanism, in which case we'd need to use one of the two latter options in Question 2 above and either hard-code plebs_dont_pay_threshold or send it in SignTx. Any opinions on the dedicated CoinJoinRequest mechanism proposed here vs. reusing the payment request mechanism?

@matejcik
Copy link
Contributor

quick note: our protobuf tooling doesn't actually have oneof support so even if we use oneof as a specification, it's internally equivalent to just dumping the fields into top level and checking uniqueness manually

@andrewkozlik
Copy link
Contributor Author

Closed in favor of #2577.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants