Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method; valid for single PrevId at first #1172

habibitcoin · 2024-11-04T22:35:47Z

Related to #1164

This PR simply implements the plumbing required to utilize the PrevIds payload for FundVirtualPSBT. Similar to how we can only handle 1 address for a raw input, this PR also only implements support for a single PrevId in the PrevIds payload.

At a high level, we perform the following:

Update FundAddressSend() to take in an optional array of PrevID pointers
If we see the PrevID pointers array is provided, and the length is 1, we add these values to the CommitmentConstraints object. CommitmentConstraints is also updated as a struct to hold an array of PrevIDs
We update constraintsToDbFilter to utilize the prevID if the length is 1:

if len(query.PrevIDs) == 1 {
			prevID := query.PrevIDs[0]
			encodedOutpoint, err := encodeOutpoint(prevID.OutPoint)
			if err == nil {
				assetFilter.AnchorPoint = encodedOutpoint
				assetFilter.TweakedScriptKey = prevID.ScriptKey.CopyBytes()
			}
			// If there's an error, we simply skip setting the fields
			// TODO, maybe log error or return it?
		}

Return an error to the RPC if an attempt is made to send more than 1 input

To-dos:

For some reason, my outpoint is not being encoded properly, but during testing if I simply comment out assetFilter.AnchorPoint = encodedOutpoint, everything else works as expected and is serving our needs in our demo environment
itest

coveralls · 2024-11-04T22:44:54Z

Pull Request Test Coverage Report for Build 11734743383

Details

7 of 59 (11.86%) changed or added relevant lines in 5 files are covered.
28 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.03%) to 40.682%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapfreighter/chain_porter.go	0	1	0.0%
tapfreighter/wallet.go	0	4	0.0%
rpcserver.go	0	23	0.0%
tapdb/assets_store.go	6	30	20.0%

Files with Coverage Reduction	New Missed Lines	%
commitment/tap.go	3	84.7%
tapdb/universe.go	4	80.91%
tapgarden/caretaker.go	4	68.87%
tapdb/multiverse.go	7	60.32%
universe/interface.go	10	51.12%

Totals
Change from base Build 11720128932:	-0.03%
Covered Lines:	24701
Relevant Lines:	60718

💛 - Coveralls

guggero

Thanks for the PR! This is definitely something we want as well and just didn't get to yet.
See inline comment for alternative and slightly more generic approach.

tapfreighter/interface.go

tapdb/assets_store.go

habibitcoin · 2024-11-08T02:07:06Z

@guggero addressed! Any thoughts on modifying or adding a new itest? I was thinking maybe to modify

// testPsbtExternalCommit tests the ability to fully customize the BTC level of
// an asset transfer using a PSBT. This exercises the CommitVirtualPsbts and
// PublishAndLogTransfer RPCs. The test case moves some assets into an output
// that has a hash lock tapscript.
func testPsbtExternalCommit(t *harnessTest) {

Or copy + update the scenario to include an array of 2 inputs perhaps

guggero

Thanks a lot for the updates!
Mostly nits remaining. Would be nice to perhaps add an integration test? Do you have time to do that?

guggero · 2024-11-08T08:44:09Z

rpcserver.go

 		if len(raw.Inputs) > 0 {
-			return nil, fmt.Errorf("template inputs not yet " +
-				"supported")
+			for _, input := range raw.Inputs {


Can remove a whole level of indentation by removing the if len(raw.Inputs) > 0 { since for ... range loops are nil safe.

guggero · 2024-11-08T08:45:48Z

rpcserver.go

-			return nil, fmt.Errorf("template inputs not yet " +
-				"supported")
+			for _, input := range raw.Inputs {
+				// Create a new chainhash.Hash and set its bytes


nit: comments should always be full sentences and end with punctuation.

guggero · 2024-11-08T08:46:00Z

rpcserver.go

+			for _, input := range raw.Inputs {
+				// Create a new chainhash.Hash and set its bytes
+				var hash chainhash.Hash
+				err := hash.SetBytes(input.Outpoint.Txid)


nit: Should use chainhash.NewHash() instead.

guggero · 2024-11-08T08:46:23Z

rpcserver.go

+						"invalid Txid length: %w", err,
+					)
+				}
+				// Decode the input into an asset.PrevID


nit: if a comment isn't at the beginning of a new block/indentation, we usually add an empty line before to give some breathing space.

guggero · 2024-11-08T08:47:31Z

rpcserver.go

+				var hash chainhash.Hash
+				err := hash.SetBytes(input.Outpoint.Txid)
+				if err != nil {
+					return nil, fmt.Errorf(


nit: Multi-line calls to formatting function should follow the different form to more easily distinguish them from non-error/non-logging calls. See https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md#exception-for-log-and-error-message-formatting.

guggero · 2024-11-08T08:47:45Z

tapfreighter/wallet.go

@@ -69,6 +69,7 @@ type Wallet interface {
 	// selected assets.
 	FundAddressSend(ctx context.Context,
 		coinSelectType tapsend.CoinSelectType,
+		prevIDs []asset.PrevID,


nit: seems to fit on previous line, here and below.

guggero · 2024-11-08T08:48:50Z

tapfreighter/interface.go

@@ -35,6 +35,9 @@ type CommitmentConstraints struct {
 	// to satisfy the constraints.
 	MinAmt uint64

+	// PrevIDs are the set of inputs allowed to be used


nit: missing full stop.

guggero · 2024-11-08T08:49:20Z

rpcserver.go

+			for _, input := range raw.Inputs {
+				// Create a new chainhash.Hash and set its bytes
+				var hash chainhash.Hash
+				err := hash.SetBytes(input.Outpoint.Txid)


We should probably check that input.Outpoint is not nil.

guggero · 2024-11-08T08:49:59Z

rpcserver.go

@@ -2049,9 +2049,31 @@ func (r *rpcServer) FundVirtualPsbt(ctx context.Context,

 	case req.GetRaw() != nil:
 		raw := req.GetRaw()
+		prevIDs := []asset.PrevID{}


nit: var prevIDs []asset.PrevID style is preferred over empty slice initialization.

habibitcoin mentioned this pull request Nov 4, 2024

[bug/feature]: Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method #1164

Open

guggero self-requested a review November 5, 2024 08:06

dstadulis assigned habibitcoin Nov 5, 2024

dstadulis added the psbt label Nov 5, 2024

guggero reviewed Nov 6, 2024

View reviewed changes

tapfreighter/interface.go Outdated Show resolved Hide resolved

tapdb/assets_store.go Outdated Show resolved Hide resolved

habibitcoin added 2 commits November 7, 2024 21:04

taprpc+tapfreighter: update FundAddressSend to accept prevIds

baed202

tapdb+tapsend+tapfreighter:add prevIds to constraints we can filter on

483eb84

habibitcoin force-pushed the template-input branch from 100f726 to 483eb84 Compare November 8, 2024 02:05

habibitcoin requested a review from guggero November 8, 2024 02:07

guggero reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method; valid for single PrevId at first #1172

Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method; valid for single PrevId at first #1172

habibitcoin commented Nov 4, 2024 •

edited

Loading

coveralls commented Nov 4, 2024 •

edited

Loading

guggero left a comment

habibitcoin commented Nov 8, 2024

guggero left a comment

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

guggero Nov 8, 2024

Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method; valid for single PrevId at first #1172

Are you sure you want to change the base?

Implement PrevId[] UTXO selection for FundVirtualPSBT RPC method; valid for single PrevId at first #1172

Conversation

habibitcoin commented Nov 4, 2024 • edited Loading

coveralls commented Nov 4, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11734743383

Details

💛 - Coveralls

guggero left a comment

Choose a reason for hiding this comment

habibitcoin commented Nov 8, 2024

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

habibitcoin commented Nov 4, 2024 •

edited

Loading

coveralls commented Nov 4, 2024 •

edited

Loading