search

satoshilabs / slips

SatoshiLabs Improvement Proposals
Creative Commons Attribution Share Alike 4.0 International
1.51k stars 1.72k forks source link

SLIP-0019: already in use byte type for BIP174 extension #1513

Closed louneskmt closed 1 year ago

louneskmt commented 1 year ago

It looks like the byte type 0x0A for the added PSBT_IN_PROOF_OF_OWNERSHIP is already used by PSBT_IN_RIPEMD160 in BIP174.

AFAIK, Trezor doesn't use PSBTs so I guess we could change that byte easily, without breaking the SLIP-0019 implementation that is in production?

prusnak commented 1 year ago

@andrewkozlik What do you think?

andrewkozlik commented 1 year ago

We have never gotten around to implementing an application of SLIP-19 in PSBT, so we can certainly change the number easily.

SLIP-19 also defines PSBT_GLOBAL_PSBT_ID. I just noticed that there is a typo. I defined the value as 0x0A and then two lines later used 0x02. I meant to use 0x02, which was the next available global type value at the time of writing, but is taken now.

Also I am not sure whether to call it PSBT_GLOBAL_PSBT_ID or PSBT_GLOBAL_PROOF_OF_OWNERSHIP_COMMITMENT_DATA. More details on the usage can be found here. It could also be called PSBT_GLOBAL_COINJOIN_ROUND_ID, but that seems too specific to coinjoin and I'd prefer something more generic.

I was pretty much waiting for a need to arise for the use of SLIP-19 in PSBTs before we request adding the type definitions into BIP-174. Wasabi's coinjoin protocol doesn't use PSBTs, so we didn't need to deal with this yet. Do you have an application that needs ownership proofs in PSBTs @louneskmt ?

louneskmt commented 1 year ago

As I mentioned in a previous comment, @Sosthene00 and I are currently implementing SLIP-0019 and SLIP-0021 in the ColdCard firmware, with the goal of adding a new rule to their HSM (ckbunker) allowing the hardware wallet to sign any transaction that don't spend any coins. For this to work, a proof of ownership is needed for every input so the wallet cannot be tricked in thinking that it does not possess one of them while it does while computing the amount being sent (otherwise, you could make it sign the transaction several times, each time tricking the wallet to sign for only one input, even though there are several it has ownership of).

One of the following goal would be to be able to use the ColdCard HSM to automatically sign for remix transactions in Whirlpool (or any other protocol with free remixes), and thus being able to do coinjoins with hardware wallets.

So far, almost all the logic for the proof construction and verification is ready for the ColdCard. The next step is to integrate this to the HSM and implement the PSBT extension.

If this is working, we could look into doing the implementation in other wallets (preferably with HSM?).

Re the keytype name, I agree with you about PSBT_GLOBAL_COINJOIN_ROUND_ID being too specific to coinjoin, while it is only a usecase of these proofs. PSBT_GLOBAL_PSBT_ID sounds too generic and not relative to ownership, so I guess PSBT_GLOBAL_PROOF_OF_OWNERSHIP_COMMITMENT_DATA or similar would do.

andrewkozlik commented 1 year ago

Here is a possible update I prepared in a separate branch: https://github.com/satoshilabs/slips/blob/slip19/slip-0019.md#psbt-extension

louneskmt commented 1 year ago

Thanks! PSBT_GLOBAL_OWNERSHIP_COMMITMENT <valuedata> should be <bytes commitmentData> and PSBT_IN_OWNERSHIP_PROOF <valuedata> should be <bytes proofOfOwnership>.

Also, I just saw that there is a PSBT_IN_POR_COMMITMENT type for the Proof-of-Reserve commitment. Maybe we could follow the naming and change OWNERSHIP to POO in both our new keytypes?

andrewkozlik commented 1 year ago

Also, I just saw that there is a PSBT_IN_POR_COMMITMENT type for the Proof-of-Reserve commitment. Maybe we could follow the naming and change OWNERSHIP to POO in both our new keytypes?

I had the same thought, but calling it "poo" is a bit degrading :-).

louneskmt commented 1 year ago

Now that you say it... 🤣

andrewkozlik commented 1 year ago

I made some more adjustments to the section and pushed the update into the master branch. See https://github.com/satoshilabs/slips/blob/master/slip-0019.md#psbt-extension. We still need to get this update into BIP 174. Would you guys be willing to take care of that? It should just be a matter of copying and pasting the table rows and replacing the words "as defined above" with "as defined in SLIP 19".

louneskmt commented 1 year ago

Perfect, see https://github.com/bitcoin/bips/pull/1434. Thanks!

prusnak commented 1 year ago

How about PSBTv2 (BIP370)?

Do we need to add/update something there?

andrewkozlik commented 1 year ago

How about PSBTv2 (BIP370)?

Do we need to add/update something there?

I don't think so. There are a number of fields specified in BIP 174 that are not in BIP 370 and yet, when you look at "Versions Allowing Inclusion" they are clearly usable in PSBTv2. Namely PSBT_GLOBAL_XPUB = 0x01 and all of the first fourteen PSBT_IN_* key types.