PSBT BIP 174 implementation

[AdamISZ]

See next.

[AdamISZ]

This is intended to be an implementation of most of the functionality in BIP 174, a few details:

• No implementation of the Combiner role as of yet; but in the cases where this is applicable, one can simply apply the Updater role multiple times.
• For the Signer role, a caller/user of this API simply uses a call to PsbtUpdater.AddPartialSignature, and this function implements the important checks laid out in the BIP on signature validity w.r.t the transaction it's signing.
• The validation checks in finalizing (which constructs final scriptSig and witness fields from the data fields) assumes that p2sh non-witness is multisig, and p2wsh is multisig (though it allows any M of N). Generalizing that scriptSig/witness construction to any custom script is an obviously desirable improvement.
• Use of BIP32 derivation fields is still an open question; they are serialized and can be added with PsbtUpdater.AddInBip32Derivation, but there is no use of them in validation here, I presume that in some cases they will be used by callers (such as hardware wallets, other constrained environments) to re-derive signing keys / check them.

It is functional however, and I'm "dogfooding" it a bit by using it in the CoinJoin rpc implementation in my fork here.

Thanks for any review (btw, if anyone could let me know how to activate travis to test the PR, that'd be helpful also).

[afk11]

Nice one, this looks good! Will try give this PR a review soon

[afk11]

Why parse into PsbtKeyValuePairs instead of structures containing the actual types? (wire.Tx, wire.TxOut, etc). Less chance of deserializing a lot of garbage if you can do sanity checks while parsing.

[AdamISZ]

Why parse into PsbtKeyValuePairs instead of structures containing the actual types? (wire.Tx, wire.TxOut, etc). Less chance of deserializing a lot of garbage if you can do sanity checks while parsing

I feel like I could interpret this a couple of different ways; one thing is, it probably makes a lot of sense to have a struct for PsbtInput (I note that the Core implementation does this), rather than having it be a list of kv-pairs. Not that I think the difference would be that huge, but it should probably be somewhat cleaner. I definitely considered it.

On the other hand, I think you're actually asking about the idea of having separate structs for each k-v type (like a struct for PsbtInWitnessUtxo, PsbtInNonWitnessUtxo, ... etc.). I actually didn't consider that; it sounds like a sound idea, not sure whether I'd get a bit tripped up with not having in-built inheritance (golang doesn't do inheritance in quite the same simple way as other langs.).

[AdamISZ]

By the way, to answer an earlier Q I had, apparently travis is not active on this repo atm.

[afk11]

I feel like I could interpret this a couple of different ways; one thing is, it probably makes a lot of sense to have a struct for PsbtInput

Yes, this is what I meant mainly! Works with separate types for GlobalKeyType+PSBT global / InputKeyType + PSBT input / etc

On the other hand, I think you're actually asking about the idea of having separate structs for each k-v type (like a struct for PsbtInWitnessUtxo, PsbtInNonWitnessUtxo, ... etc.)

Hmm, might be useful for the Unknown / Derivations / PartialSig fields since they're a map type.

[AdamISZ]

Just dropping a quick note here, in case anyone reads, that I'm in the process of updating much as per @afk11 's comments (separating out the serialization from the logical manipulation using structs for PsbtInput etc.). Expect to post a big refactoring of this here in the next few days, probably.

[AdamISZ]

Finished above-mentioned refactoring.

There's no change to the description as per this comment.

[RHavar]

Trying to test this, but can't get it working. I'm using the transaction:

00000000000101e165f072311e71825b47a4797221d7ae56d4b40b7707c540049aee43302448a40000000000feffffff0212f1126a0000000017a9143e836801b2b15aa193449d815c62d6c4b6227c898780778e060000000017a914ba4bdb0b07d67bc60f59c1f4fe54170565254974870000000000

loading it with psbt.NewPsbtFromUnsignedTx then printing it with transaction.B64Encode()

which gives:

cHNidP8BAHYAAAAAAAEB4WXwcjEecYJbR6R5ciHXrlbUtAt3B8VABJruQzAkSKQAAAAAAP7///8CEvESagAAAAAXqRQ+g2gBsrFaoZNEnYFcYtbEtiJ8iYeAd44GAAAAABepFLpL2wsH1nvGD1nB9P5UFwVlJUl0hwAAAAAAAAAAAA==

I try feed this into core: bitcoin-cli decodepsbt $base64fromabove and get:

TX decode failed CDataStream::read(): end of data: unspecified iostream_category error.

The same transaction using core gives a psbt base64 of:

cHNidP8BAHMAAAAAAeFl8HIxHnGCW0ekeXIh165W1LQLdwfFQASa7kMwJEikAAAAAAD+////AhLxEmoAAAAAF6kUPoNoAbKxWqGTRJ2BXGLWxLYifImHgHeOBgAAAAAXqRS6S9sLB9Z7xg9ZwfT+VBcFZSVJdIcAAAAAAAAAAA==

which it's able to decode

[RHavar]

Also how come psbt's (Inputs / Outputs / Unknowns) is a pointer-to-a-slice and not just a slice?

I'm also not a huge fan of the memoized Raw thing in Psbt, it seems really error prone. I think it'd be cleaner (and likely more efficient) to remove it, and just compute it on demand when ever it's needed.

[AdamISZ]

First, thanks a lot for taking a look. Second: I'm trying to replicate your error with the given raw tx, using NewPsbtFromUnsignedTx . Running this:

serTx, err := hex.DecodeString("00000000000101e165f072311e71825b47a4797221d7ae56d4b40b7707c540049aee43302448a40000000000feffffff0212f1126a0000000017a9143e836801b2b15aa193449d815c62d6c4b6227c898780778e060000000017a914ba4bdb0b07d67bc60f59c1f4fe54170565254974870000000000")
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    tx := wire.NewMsgTx(2)
    err = tx.Deserialize(bytes.NewReader(serTx))
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    psbt1, err := NewPsbtFromUnsignedTx(tx)
    if err != nil {
        t.Fatalf("Error: %v", err)
    }
    fmt.Println(psbt1.B64Encode())

fails on the call to create psbt1 with "Invalid PSBT, raw transaction must be unsigned".

Edit: So I can see the transaction actually is unsigned! This indeed must be some kind of bug. Investigating.

Will keep looking at this and also address your design-related comments, which I suspect are quite right.

[AdamISZ]

Yes, it was a bug. On line 717 of psbt.go, replace tin.Witness != nil with len(tin.Witness) != 0 and it replicates the same base64 as Core.

(Bug was that I forgot tin.Witness is a slice, empty doesn't equal nil).

Will push fix once I've thought through the other issues you brought up.

[AdamISZ]

@RHavar re your second comment:

Yes, the Raw element is unnecessary, I agree.

Memo-ization as you say could be a justification, but def not worth it here, it's quite hard to imagine a situation where perf. would ever be an issue. Doesn't justify adding unnecessary code/state.

Re: pointer to slice instead of just slice, I also agree, that looks like a bit of a derp to be honest :)

[RHavar]

and it replicates the same base64 as Core.

Does it? Here's my full test file:

package main

import (
    "github.com/btcsuite/btcd/wire"
    "bytes"
    "fmt"
    "encoding/hex"
    "log"
    "github.com/btcsuite/btcutil/psbt"
)

func main() {
    serTx, err := hex.DecodeString("00000000000101e165f072311e71825b47a4797221d7ae56d4b40b7707c540049aee43302448a40000000000feffffff0212f1126a0000000017a9143e836801b2b15aa193449d815c62d6c4b6227c898780778e060000000017a914ba4bdb0b07d67bc60f59c1f4fe54170565254974870000000000")
    if err != nil {
        log.Fatalf("Error: %v", err)
    }
    tx := wire.NewMsgTx(2)
    err = tx.Deserialize(bytes.NewReader(serTx))
    if err != nil {
        log.Fatalf("Error: %v", err)
    }
    psbt1, err := psbt.NewPsbtFromUnsignedTx(tx)
    if err != nil {
        log.Fatalf("Error: %v", err)
    }
    fmt.Println(psbt1.B64Encode())
}

And it prints:

cHNidP8BAHYAAAAAAAEB4WXwcjEecYJbR6R5ciHXrlbUtAt3B8VABJruQzAkSKQAAAAAAP7///8CEvESagAAAAAXqRQ+g2gBsrFaoZNEnYFcYtbEtiJ8iYeAd44GAAAAABepFLpL2wsH1nvGD1nB9P5UFwVlJUl0hwAAAAAAAAAAAA==

which not equiv to what core gives:

cHNidP8BAHMAAAAAAeFl8HIxHnGCW0ekeXIh165W1LQLdwfFQASa7kMwJEikAAAAAAD+////AhLxEmoAAAAAF6kUPoNoAbKxWqGTRJ2BXGLWxLYifImHgHeOBgAAAAAXqRS6S9sLB9Z7xg9ZwfT+VBcFZSVJdIcAAAAAAAAAAA==

And if i run bitcoin-cli decodepsbt it works with the output of core, but fails with the output of this PR

[AdamISZ]

Did you apply the patch? I tested it as giving exactly that output after the patch.

[RHavar]

Did you apply the patch? I tested it as giving exactly that output after the patch.

Which patch? I just changedtin.Witness != nil to len(tin.Witness) != 0. But that shouldn't effect the result, just prevent it erroring out. (The reason I didn't hit it before was I was loading the transaction in a diff way that had actual nil witness)

[AdamISZ]

Ah, I get it now. The Serialize() call is failing so the Raw is not valid, so the fact that you get a weird result instead of an error is related to your point about Raw (that is, it being an error prone way of doing things).

I've basically finished all the changes (including that), am about to push. It'll be fixed then.

[AdamISZ]

So the above commit does those three things (fix validateUnsignedTx, remove Raw and use Serialize() only, change Inputs, Outputs and Unknowns in Psbt structs to be slices not pointers-to (arguably could be slices of pointers, but this seems fine).

Need to improve the tests probably, also create a decode (into json say) function seems like an obvious win. Exact nature of rpcs and where they go is open (as mentioned at the start, did a little bit of work on that in my lnd fork in lnwallet, but that's just speculative really).

[AdamISZ]

@RHavar Just in case it helps, when I said it tested correct, I meant this function which afaict is exactly the same calls as yours. It worked after that change (len(tin.Witness)). A bit confused tbh what was happening before but if it's still not operating correctly let me know.

[RHavar]

Looks good 👍

Confirming my concerns have been addressed, and my test is now working =)

Thanks!

[AdamISZ]

Just dropping a note here that this update to functionality that's been done in Core must be added here, too.

[AdamISZ]

Just to note on the above, I'd have preferred to do it a cleaner way involving having each type reuse the same code for duplicate checking, but I found it was a bit weird trying to do that with an interface; if there's a better idiom for it to remove that code duplication, let me know; what is here should work fine I guess, plus I added two tests for the deserialization of such cases.

[AdamISZ]

@bjarnemagnussen many thanks for your review. I'm sorry I have left this to one side for a few months, was busy with other stuff. I also notice you're doing a python implementation which may be of interest to me for Joinmarket, but that's off topic here.

Made a few comment responses above, I will try to address those points and update the PR shortly.

[AdamISZ]

So https://github.com/btcsuite/btcutil/pull/126/commits/021250ec3443533a7e9139f7ad1580488489bcf3 adds checks that a given witnessScript or redeemScript is actually multisig and that the provided pubkeys and signatures in the partial signatures match, in number, the policy of the multisig. Also a couple of extra tests in the psbt_tests to sanity check those functions.

This was achieved using txscript.CalcMultiSigStats in the standard.go module.

As mentioned above we're still restricted to redeemScripts/witnessScripts of type M of N multisig here, anything else will not be finalizable. It may need more work on my part (or others) to generalize that on this codebase. But at least we cover for now all of p2pkh, p2sh multisig, p2wpkh, p2sh-p2wpkh, p2sh-p2wsh (multisig).

I'll take a little look at linting and other code checks that are appropriate here, and probably add the new "POR" keytype that was added for inputs recently to BIP174, but after that there'll probably not be much more I can do to improve it.

[Roasbeef]

Has this PR been updated to account for these changes in bitcoind?

[AdamISZ]

I remember around the last time I was looking at this, something like May, there was at least a new field added (POR as mentioned above), and I haven't looked at it since then. So yes for full support, those additions would be needed (although with BIP174's design cross-compatibility should still be there even if other impls know about fields you don't). Also I'd have to review Core merges to know if there are further things to do.