Kukks
commented
4 years ago Hey @AdamISZ , thanks for the feedback! The specification document covers the code in quite a detailed manner( most of the technicalities were written following the code line by line) so it's fine to skip code review, especially if you don't know how NBitcoin (and C# syntax in general) works in detail.
The spec does not mention input/output ordering. This is mentioned in the original BIP79 but I think could be clearer. I can see an argument for just enforcing BIP69 everywhere, as it makes it easy for both sides to reason about.
BIP69 is applied from what I remember. We tried to only mention where we deviate from the original BIP to keep it short. We'll probably go into more detail if we decide to propose an amendment to the original BIP or create a new BIP entirely.
I already mentioned this in earlier discussions, and people don't seem to agree with me, but just for the record: in JM i have the sender specify a (payjoin) version range and the receiver responds with a choice from within that range (in JM only a version 01 exists for now). Although one can retrofit versioning (no version = version 0 so to speak), I don't see an advantage in not being explicit.
As this payjoin protocol uses BIP21, we can easily expand on this later, if needed. We're currently using the key pj to signal payjoin support with the value being the url of the endpoint.
If for some reason, we need to commit breaking changes to the protocol after it has been sufficiently adopted, we could always try one of the following options:
- switch to a different key, such as
pj2 - add another key, to indicate version, such as
pjv=1(payjoin version 1)
If for some reason, we notice servers start offering partial support of the specification, there are the following options:
- leave as is and return a status code indicating failure due to lack of capability.
- add a response header with capabilities options
- add a new key specifying capabilities (or specifying capabilities disabled) such as
pjc=psbt,rawtx,etcwhere each key is described in the spec as needed. (psbt would mean PSBT support, rawtx would mean raw transaction support, etc). We can obviously expand this to signal anything, such as the server not handling fee bumps, not supporting p2sh, etc.
You don't mention the UIH1 or UIH2 heuristics that were discussed here. Does that mean the receiver utxo selection doesn't pay attention to that? To be clear, I don't think that's necessarily a problem, but one can argue it's useful to address it. In the JM implementation the approach can be summarized "try to avoid UIH2 but go ahead anyway even if you can't". I guess if you had included this in the code it would have been mentioned here.
We currently have a very basic implementation of utxo selection based on UIH1 & UIH2 with a limitation that it has only been checked against the contributed input. I'm working on making it a proper check on all inputs and should come soon. That said, if a desirable input ( or inputs, as we can maybe find multiple utxos that do fit better than a single utxo) is not found, we grab a utxo deterministic manner. Here is the code comment on that deterministic selector:
/// This comparer sorts utxo in a deterministic manner
/// based on a random parameter.
/// When a UTXO is locked because used in a coinjoin, in might be unlocked
/// later if the coinjoin failed.
/// Such UTXO should be reselected in priority so we don't expose the other UTXOs.
/// By making sure this UTXO is almost always coming on the same order as before it was locked,
/// it will more likely be selected again.The snowball effect and whether the receiver chooses utxos "greedily" or not: in extreme a merchant can end up with only having 1 utxo if they do payjoin with every tx, because they always consume the last payment they received in the next payment. Obviously this is not a realistic scenario but notice how it's undesirable, almost certainly: first, because the growing size of the utxo flags what is happening, and second, it's undesirable if every transaction is a payjoin, since that makes the blockchain analyst's job easier, at least potentially, than if there is a mixture of payjoin/non-payjoin. So I guess this is just the same as my UIH comment above. To the extent you are deciding on a standard, you don't want to be completely restrictive on utxo selection algorithms, but it may be worth specifying what should be avoided in utxo selection, at least.
It is very unlikely that a payjoin will happen so often to cause such an obvious snowball, due to the many restrictive factors of doing a payjoin:
- limited wallet support
- wallet format must match the server ( though this can be fixed later by having a merchant store utxos of many formats. Don't hold your breath, it is hard and will take time :) ) ( this can also be potentially fixed by another idea I described here)
- constant movement of funds from hot to cold wallet to mitigate risk of stole funds
Will fix the typos for the rest, thanks!
NicolasDorier
AdamISZ
6102bitcoin
nopara73
LaurentMT
The following refers to this version of the doc.
First a thanks to @Kukks for bringing this to my attention, and to the whole project for implementing this, it looks like a major step forward. I wanted to review a little bit by comparing with what I did in Joinmarket a year or two back. Also apologies for not reviewing the code itself, but that would be very difficult for me.
Here's what I came up with:
There are several additional points you make here that I didn't consider, that are excellent, but no need to detail them I guess :)