SealBatch RPC endpoint accepts signed group PSBT

[ffranr]

This PR modifies the SealBatch RPC endpoint such that it accepts a complete signed PSBT directly, instead of requiring the witness to be extracted and passed separately.

The goal of this PR is to simplify the UX for the external key minting procedure. It's a pretty radical simplification because it replaces the bitcoin-cli decodepsbt call from the docs/external-group-key.md doc. I think we should include something along these lines in the next release. (cc @dstadulis )

---

This work isn't complete. I ran into a problem: the signed PSBT provided by chantools does not contain the right flags/metadata (such as tapscript root) to be able to map the PSBT to a group key request. The PSBT seems to only contain the witness.

So we might need to include the asset ID mapping in the RPC request. Thoughts on this?

I think I'm going to go with requiring the asset ID to signed PSBT mapping passed in as an arg. Perhaps like this:

message WitnessStack {
    // The witness stack for the asset group.
    repeated bytes witness = 1;
}

message GroupSeal {
    // The asset ID of the pending asset that should be assigned this asset
    // group witness.
    bytes genesis_id = 1;

    oneof group_seal {
        // The serialized witness stack for the asset group.
        WitnessStack witness = 2;

        // The base64 encoded signed group virtual PSBT.
        string signed_group_virtual_psbt = 3;
    }
}

message SealBatchRequest {
    /*
    If true, then the assets currently in the batch won't be returned in the
    response. This is mainly to avoid a lot of data being transmitted and
    possibly printed on the command line in the case of a very large batch.
    */
    bool short_response = 1;

    // This field is deprecated. Use the `seals` field instead.
    // The assetID, witness pairs that authorize asset membership in a group.
    repeated taprpc.GroupWitness group_witnesses = 2 [deprecated = true];

    // Group seals that authorize asset membership in a group.
    repeated GroupSeal seals = 3;
}

I think it's wise to require the genesis asset ID because we never know what might be included in the signed PSBT.

[coveralls]

Pull Request Test Coverage Report for Build 12865228219

Details

• 1 of 104 (0.96%) changed or added relevant lines in 4 files are covered.
• 40 unchanged lines in 9 files lost coverage.
• Overall coverage decreased (-0.04%) to 40.793%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
cmd/tapcli/assets.go	0	2	0.0%
itest/utils.go	0	7	0.0%
rpcserver.go	0	16	0.0%
tapgarden/planter.go	1	79	1.27%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.22%
tapchannel/aux_invoice_manager.go	2	85.28%
fn/option.go	3	43.3%
tapchannel/aux_leaf_signer.go	3	43.43%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.5%
asset/asset.go	4	77.01%
asset/mock.go	6	92.27%
universe/interface.go	12	51.95%

Totals
Change from base Build 12830976373:	-0.04%
Covered Lines:	26650
Relevant Lines:	65330

---

💛 - Coveralls

[guggero]

I think we should have designed the API like this in the first place. Not having the asset ID in there and only relying on the order of things is suboptimal.
But now that we've already have that in place it's a bit unfortunate that we have to deprecate things and look in both places for a few version.

My initial idea was to do everything in the CLI only: Call ListBatches, extract the PSBTs, then compare the previous outpoints of the inputs to match the signed PSBTs with the unsigned ones to get the correct order.
But perhaps that's too ugly. So I'm okay with your approach.

But in any case, the previous outpoint should be the thing that can be matched against, as that should be unique per asset.

[ffranr]

Needs rebase.

[guggero]

Nice work!

[ZZiigguurraatt]

I think I'm going to go with requiring the asset ID to signed PSBT mapping passed in as an arg

Instead of requiring the asset id to be passed, if it can't be included in the PSBT, what about just looking through and testing all unsealed assets and see if the witness is valid for any of them? Might be slower, but would be simpler for the user.

[guggero]

Changed the base branch to main, needs a rebase.

[ffranr]

I think I'm going to go with requiring the asset ID to signed PSBT mapping passed in as an arg

Instead of requiring the asset id to be passed, if it can't be included in the PSBT, what about just looking through and testing all unsealed assets and see if the witness is valid for any of them? Might be slower, but would be simpler for the user.

Oli suggested another way which didn't require asset ID in psbt or passed in via arg, we can match on prevout.

[ffranr]

I haven't tried out the CLI arg or the new doc modification. I can do that when I'm online later.

[ZZiigguurraatt]

I just tested this and I'm getting unable to parse signed group virtual PSBT: unexpected EOF .

[guggero]

tACK, very nice!

Just a couple user experience questions.

[guggero]

So the idea is that the old way and new way could even be mixed? Since both seem to be parsed and then merged (at least with a collision check, so nothing is overwritten).

Not sure if that's more confusing to the user rather than helpful? I think just from an API perspective it makes sense to keep the old way (as that's perhaps easier to use programmatically). But perhaps we should disallow using both at the same time, to avoid some confusion?

[ZZiigguurraatt]

the old way still works for me with this branch, but as mentioned above, the new way fails

[guggero]

Did you switch between branches? Or use my Ledger branch before I rebased it on top of this one?
Not being able to parse the PSBT probably means it's because of the bump in the psbt library that isn't in this branch. So can you try again with the current version of #1301 that includes this PR?

[ZZiigguurraatt]

Still getting unable to parse signed group virtual PSBT: unexpected EOF with cold-group-key-pedersen. I'm using chantools to generate the PSBT, by the way. Maybe I'm not supposed to use that?

[guggero]

Hmm, weird. Can you paste the PSBT here please?

[ZZiigguurraatt]

You can give it a try here: https://github.com/lightninglabs/tapdvalidation/tree/sealbatch-signed-group-psbt-test . Follow README.md, except you'll have to add an extra step to checkout this branch before you run ./start_mini_META.

[ZZiigguurraatt]

You can see here the difference between the old way that worked and the new way that doesn't work: https://github.com/lightninglabs/tapdvalidation/commit/e95ee5b432443570632cc36202c14fb902b4cc9a.

[ZZiigguurraatt]

Okay, I figured out my issue. It has been fixed here: https://github.com/lightninglabs/tapdvalidation/commit/2aa7f2f05eb43c7f3e11eb1381759f240057c231 . The problem is I passed a string as signed_group_virtual_psbts instead of an array of strings.

Can we get a more useful test and error message for this mistake? unable to parse signed group virtual PSBT: unexpected EOF is super uninformative. I'm a little confused why gRPC accepts a single string instead of require an array for "repeated" fields. I'm not sure why it doesn't enforce this, but I've made this mistake before and the error messages never actually lead me to understand the mistake in a meaningful way.

[ffranr]

SealBatchRequest in mintrpc is given as

type SealBatchRequest struct {
    state                   protoimpl.MessageState
    sizeCache               protoimpl.SizeCache
    unknownFields           protoimpl.UnknownFields
    ShortResponse           bool                   `protobuf:"varint,1,opt,name=short_response,json=shortResponse,proto3" json:"short_response,omitempty"`
    GroupWitnesses          []*taprpc.GroupWitness `protobuf:"bytes,2,rep,name=group_witnesses,json=groupWitnesses,proto3" json:"group_witnesses,omitempty"`
    SignedGroupVirtualPsbts []string               `protobuf:"bytes,3,rep,name=signed_group_virtual_psbts,json=signedGroupVirtualPsbts,proto3" json:"signed_group_virtual_psbts,omitempty"`
}

Are you using JSON and Python? I think in golang the type checker would have caught this.

unable to parse signed group virtual PSBT: unexpected EOF is super uninformative

I'll modify the error message to include the signed PSBT as seen by tapd.

[ZZiigguurraatt]

Using Python.

It seems to do more clear error messages if I mix up a string , bytes, or int, but for some reason does not check arrays very well.

[GeorgeTsagk]

LGTM!
🔥 this was def needed