Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

habibitcoin
Copy link
Contributor

@habibitcoin habibitcoin commented Nov 4, 2024

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
Copy link

coveralls commented Nov 4, 2024

Pull Request Test Coverage Report for Build 12128140103

Details

  • 20 of 74 (27.03%) changed or added relevant lines in 5 files are covered.
  • 17 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 40.839%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapfreighter/chain_porter.go 0 1 0.0%
tapfreighter/wallet.go 0 4 0.0%
tapdb/assets_store.go 19 33 57.58%
rpcserver.go 0 35 0.0%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 1 0.0%
tappsbt/create.go 2 53.22%
tapgarden/caretaker.go 4 68.5%
tapdb/assets_store.go 10 63.29%
Totals Coverage Status
Change from base Build 12125319192: -0.03%
Covered Lines: 25788
Relevant Lines: 63145

💛 - Coveralls

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
tapdb/assets_store.go Outdated Show resolved Hide resolved
@habibitcoin
Copy link
Contributor Author

@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

@habibitcoin habibitcoin requested a review from guggero November 8, 2024 02:07
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
tapfreighter/wallet.go Outdated Show resolved Hide resolved
tapfreighter/interface.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
@lightninglabs-deploy
Copy link

@habibitcoin, remember to re-request review from reviewers when ready

@habibitcoin
Copy link
Contributor Author

@guggero I decided to modify the testAssetBalances itest, as it seemed to be a suitable candidate to test this functionality.

I am having trouble however testing both the FundVirtualPSBT RPC with a valid PrevId, release the input lease, and then trying again without any PrevId specified, confirming we succeed

I was trying to use tapgarden.NewMockWalletAnchor(), but that doesn't seem to work. I was also seeing if maybe I could create a taprootassets.NewLndRpcWalletAnchor(), but wasn't sure what to use as the input. Curious if you had any recommendations there? I think once that is fixed, all the tests will pass and this should be ready to ship!


	// 4. Succeed if no Inputs are provided.
	// Unlock the input so that it can be spent again.
	walletAnchor := tapgarden.NewMockWalletAnchor()
	err = walletAnchor.UnlockInput(ctxb, *out)
	require.NoError(t.t, err)

	_, err = aliceTapd.FundVirtualPsbt(
		ctxt, &wrpc.FundVirtualPsbtRequest{
			Template: &wrpc.FundVirtualPsbtRequest_Raw{
				Raw: &wrpc.TxTemplate{
					Recipients: recipients,
				},
			},
		},
	)
	require.NoError(t.t, err)
	
	```

@guggero
Copy link
Member

guggero commented Nov 29, 2024

Yeah, that sounds good as an integration test.

You can't use a mock in itests, as that will literally not do anything. What you'll want to call into instead is assetwalletrpc.RemoveUTXOLease to release the asset-level input lease.
It's possible that any BTC on-chain funds added to pay for the fees then also remain leased. Those you need to release on the lnd RPC with walletrpc.ReleaseOutput.
Or you just make sure the lnd that's backing Alice has enough on-chain UTXOs so it doesn't become an issue in the first place.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the update and test!
Looks very close, just a couple of suggestions and fixes.

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
itest/assets_test.go Outdated Show resolved Hide resolved
itest/assets_test.go Outdated Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, LGTM 🎉

@habibitcoin
Copy link
Contributor Author

@dstadulis just wanted to bump this before it gets stale

@guggero guggero requested a review from gijswijs January 23, 2025 08:03
Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I made some comments and a clarifying question and a single change request. Very nice to see this addition.

This also needs a rebase, before we can merge.

return nil, fmt.Errorf("unable to query commitments: %w", err)
}

// If we want to restrict on specific inputs, we do the filtering now.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding: We filter here after the queryCommitments call, because there is no way to make it part of the assetFilter. Witnesses are added to the result after querying the assets. See https://github.com/habibitcoin/taproot-assets/blob/a1ec777fca91cd7ac250345d5da87bd9186ef04c/tapdb/assets_store.go#L953-L957
So we can only filter on those properties after the fact.

// returns the correct balances, taking into account the `include_leased` flag.
// testAssetBalances validates the balance retrieval and virtual PSBT funding
// functionality for issued assets. The test performs the following steps:
// 1. Mints two batches of assets and verifies that the tapcli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Love these clear descriptions of the itest.

Comment on lines +761 to +762
{
name: "Succeed if a valid Outpoint is provided",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would add a test case here that fails if a valid outpoint isn't found.

diff --git a/itest/assets_test.go b/itest/assets_test.go
index 00633c65..944f01d0 100644
--- a/itest/assets_test.go
+++ b/itest/assets_test.go
@@ -758,6 +758,21 @@ func testAssetBalances(t *harnessTest) {
                        expectError:    true,
                        expectedErrMsg: "invalid script key",
                },
+               {
+                       name: "Fail if valid, unfindable Outpoint is provided",
+                       inputs: []*wrpc.PrevId{
+                               {
+                                       Outpoint: &taprpc.OutPoint{
+                                               Txid:        anchorTxid,
+                                               OutputIndex: anchorVout + 1,
+                                       },
+                                       Id:        assetId,
+                                       ScriptKey: scriptKey,
+                               },
+                       },
+                       expectError:    true,
+                       expectedErrMsg: "failed to find coin(s)",
+               },
                {
                        name: "Succeed if a valid Outpoint is provided",
                        inputs: []*wrpc.PrevId{

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

6 participants