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

Add affiliation id to coinjoin notification header #10407

Merged
merged 1 commit into from Apr 4, 2023

Conversation

onvej-sl
Copy link
Contributor

This pull request adds affiliation id to the coinjoin notification header. The header is part of the data that is signed by the coordinator.

This pull request makes coinjoin notifications non-transferable from one affiliate server to another. In other words, an affiliate server that receives a coinjoin notification can eventually prove that the coinjoin notification was addressed to them.

Since the new format of the header is not compatible with the old one, deployment of this change requires coordinated deployment of affiliate servers as well (unless affiliate servers temporarily accept both formats).

@onvej-sl onvej-sl marked this pull request as ready for review March 31, 2023 15:08
@lontivero
Copy link
Collaborator

This makes a lot of sense and I would take the opportunity to add some extra info.

@onvej-sl
Copy link
Contributor Author

onvej-sl commented Apr 3, 2023

This makes a lot of sense and I would take the opportunity to add some extra info.

Do you have something specific you want to add?

@lontivero
Copy link
Collaborator

Do you have something specific you want to add?

Yes. There are at least two things it would simplify things:

  • Add Transaction Hash (id). That would allows us to find the coinjoin transaction easier.
  • Add Inputs' amount. This is something affsrv can verify but zksnacks can simply trust.

@andrewkozlik
Copy link

Yes. There are at least two things it would simplify things:

  • Add Transaction Hash (id). That would allows us to find the coinjoin transaction easier.
  • Add Inputs' amount. This is something affsrv can verify but zksnacks can simply trust.

The reason why we didn't include the TXID in the signed data is that it is basically redundant. It duplicates the information about the inputs and outputs, and by doing so it creates the potential for some inconsistency. In a way it violates the DRY principle. The TXID does include some additional information inside itself, like sequences, lock time and version. But these pieces of data are not relevant for the coinjoin (as long as all clients are synced on the values to use when signing the transaction), so there is no reason to commit to them.

Similarly the amounts can be obtained from the blockchain based on the outpoints. So again this creates the potential for some inconsistency.

How about adding this information (TXID, amounts), but not sign it? So it's for informational purposes only.

@lontivero
Copy link
Collaborator

lontivero commented Apr 3, 2023

How about adding this information (TXID, amounts), but not sign it? So it's for informational purposes only.

The idea is to enrich the requests with information that we have available at the moment of building them so we can simplify the verification procedure. If the request is signed by us then we know the info was produced by us and then we (not you) can trust it is correct.

Having the whole coinjoin notification request signed doesn't worsen anything for you, after all you aren't/won't be using it, but makes things much easier for us and for BTCPayServer plugin.

@lontivero
Copy link
Collaborator

This is how the coinjoin notification request would like like:

{
  "body": {
    "fee_rate": 300000,
    "inputs": [
      {
        "amount": 100000000,
        "is_affiliated": true,
        "is_no_fee": false,
        "prevout": {
          "hash": "e5b7e21b5ba720e81efd6bfa9f854ababdcddc75a43bfa60bf0fe069cfd1bb8a",
          "index": 0
        },
        "script_pubkey": "5120b3a2750e21facec36b2a56d76cca6019bf517a5c45e2ea8e5b4ed191090f3003"
      },
      {
        "amount": 200000000,
        "is_affiliated": true,
        "is_no_fee": false,
        "prevout": {
          "hash": "f982c0a283bd65a59aa89eded9e48f2a3319cb80361dfab4cf6192a03badb60a",
          "index": 1
        },
        "script_pubkey": "51202f436892d90fb2665519efa3d9f0f5182859124f179486862c2cd7a78ea9ac19"
      }
    ],
    "min_registrable_amount": 5000,
    "no_fee_threshold": 1000000,
    "outputs": [
      {
        "amount": 50000,
        "script_pubkey": "5120e0458118b80a08042d84c4f0356d86863fe2bffc034e839c166ad4e8da7e26ef"
      },
      {
        "amount": 50000,
        "script_pubkey": "5120bdb100a4e7ba327d364642dc653b9e6b51783bde6ea0df2ccbc1a78e3cc13295"
      },
      {
        "amount": 7202065,
        "script_pubkey": "5120c5c7c63798b59dc16e97d916011e99da5799d1b3dd81c2f2e93392477417e71e"
      },
      {
        "amount": 49010,
        "script_pubkey": "512062fdf14323b9ccda6f5b03c5c2c28e35839a3909a2e14d32b595c63d53c7b88f"
      },
      {
        "amount": 36945,
        "script_pubkey": "76a914a579388225827d9f2fe9014add644487808c695d88ac"
      }
    ],
    "slip44_coin_type": 1,
    "timestamp": 0,
    "transaction_id": "0000000000000000000000000000000000000000000000000000000000000001"
  },
  "header": {
    "affiliation_id": "WalletWasabi",
    "title": "coinjoin notification",
    "version": 1
  }
}

@lontivero
Copy link
Collaborator

@Kukks I want to merge this PR. The only thing that changes for you is this:

- Payload payload = new(Header.Instance, request.Body);
+ Payload payload = new(Header.Create("btcpayserver"), request.Body);

@andrewkozlik
Copy link

How about adding this information (TXID, amounts), but not sign it? So it's for informational purposes only.

The idea is to enrich the requests with information that we have available at the moment of building them so we can simplify the verification procedure. If the request is signed by us then we know the info was produced by us and then we (not you) can trust it is correct.

Having the whole coinjoin notification request signed doesn't worsen anything for you, after all you aren't/won't be using it, but makes things much easier for us and for BTCPayServer plugin.

OK

@Kukks
Copy link
Contributor

Kukks commented Apr 4, 2023

@Kukks I want to merge this PR. The only thing that changes for you is this:

- Payload payload = new(Header.Instance, request.Body);
+ Payload payload = new(Header.Create("btcpayserver"), request.Body);

Ok, but please give me a heads up before it is deployed.

@andrewkozlik
Copy link

Ok, but please give me a heads up before it is deployed.

We all need to coordinate on the deployment. So let's agree beforehand.

@lontivero lontivero merged commit 9d3dbd4 into zkSNACKs:master Apr 4, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants