[tapsend]: Enforce unique script keys #1181
Open
+478
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 11838616455Details
💛 - Coveralls |
|
I'll look into the failed itest on Monday, looks like a test is using duplicate keys. |
Roasbeef
reviewed
Nov 8, 2024
tapsend/send.go
Outdated
| for idx := range vPkt.Outputs { | ||
| vOut := vPkt.Outputs[idx] | ||
|
|
||
| // Check if the script key has already been used in this |
There was a problem hiding this comment.
Isn't it ok for us to have a script key duplicate across Bitcoin transaction outputs?
There was a problem hiding this comment.
I think a duplicate script key for the same asset would be an issue, even without those assets being children of an asset split. If that occurred, the exclusion proof would be a proof of inclusion, but for a different leaf vs. the existing model where the exclusion proof terminates in a nil leaf.
There was a problem hiding this comment.
exclusion proof terminates in a nil leaf.
Yes, exactly. The way our exclusion proofs currently work is that they expect no leaf to be present at the exclusion location (which is defined by the script key).
There was a problem hiding this comment.
Hmm yeh, I do wonder if there's a small change we can make the the exclusion proofs here to patch this portion.
There was a problem hiding this comment.
That would be optimal, yes. But not sure what you had in mind here? Prove that the exact asset leave isn't in another output? But how would you define equality here so that some trickery with a slight modification (e.g. just change the script version) isn't possible?
Also, while brainstorming on this with @jharveyb, the question came up if this restriction could ever be an issue for HTLCs? Since it's possible that multiple MPP shards have the same script key (since they use the same payment hash).
| vPkt.SetInputAsset(0, &a) | ||
|
|
||
| for i, outputKey := range outputKeys { | ||
| vPkt.Outputs[i] = &tappsbt.VOutput{ |
There was a problem hiding this comment.
Shouldn't the output index be considered here when determining uniqueness?
There was a problem hiding this comment.
No, that's kind of the point of this fix. See comment(s) above.
jharveyb
approved these changes
Nov 11, 2024
tapsend/send_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| name: "collision, same group packets, same keys", |
There was a problem hiding this comment.
would be good to test collision based on asset ID + script key in addition to group key + script key.
There was a problem hiding this comment.
I would like to voice something similar. I think this test case should be added. It's a case that increased my understanding of the PR, and if IIUC it's also a edge case that should be tested.
{
name: "no collision, multi asset packets, same keys",
vPackets: []*tappsbt.VPacket{
makeVPacket(
assetID1, nil,
[]asset.ScriptKey{
scriptKey1,
},
),
makeVPacket(
assetID2, nil,
[]asset.ScriptKey{
scriptKey1,
},
),
},
},
There was a problem hiding this comment.
Thanks, added two more test cases.
| return c.AnchorPoint.String() | ||
| }, | ||
| ) | ||
| log.Infof("Identified %v eligible asset inputs for send of %d to %v: "+ |
There was a problem hiding this comment.
nit: this is counting anchor inputs and not asset inputs?
IIUC the anchor inputs may contain multiple eligible asset inputs, which we will select from later.
There was a problem hiding this comment.
I think if we have multiple eligible assets in the same anchor, we'd still get multiple *AnchoredCommitment structs returned. So IMO this is correct.
gijswijs
approved these changes
Nov 11, 2024
There was a problem hiding this comment.
Very nice!
I feel that the tapfreighter commits should be their own PR? They don't have anything to do with enforcing unique script keys. Same thing goes fee estimation itest.
Or maybe just mention it in the PR that a number of other fixes were implemented as well, just for future reference.
gijswijs
reviewed
Nov 11, 2024
tapsend/send.go
Outdated
| // Check if the script key has already been used in this | ||
| // transaction. | ||
| scriptKey := asset.ToSerialized(vOut.ScriptKey.PubKey) | ||
| if _, ok := perAssetMap[scriptKey]; ok { |
There was a problem hiding this comment.
Let me see if I understand this:
In theory it would be ok to reuse scriptKeys over different tapCommitmentKeys?
There was a problem hiding this comment.
Yes, since those would have different paths in the anchored tapCommitment.
I think this statement is correct:
"For a given transfer, across all anchor outputs, assets can share a TapCommitmentKey(), or an AssetCommitmentKey(), but not both."
If both are shared, for any two assets, then we can't generate valid exclusion proofs.
This test is very useful for debugging user-supplied proofs that failed for some reason. We expand the test to also verify the inclusion and exclusion proofs.
Actually, they kind of do. To be able to test the check introduced with the PR, we want to be able to try again. That's why we added the fix for releasing the leased inputs. Which then changes a bunch of assumptions in the itests. |
This helper method returns the asset specifier for the virtual packet. Because a single virtual packet is only supposed to carry inputs and outputs of a single asset ID, the TAP commitment key is valid for the whole vPacket.
To avoid asset leaves colliding on the same asset ID and script key.
Previously we only released/unlocked the BTC level outputs we leased from the lnd wallet when an error occurred. But if we're still in a state where nothing has been written to disk and the user can try again, we can safely release/unlock the asset level UTXOs as well to avoid them being locked for 10 minutes.
This test demonstrates that we get the correct error when we attempt to send to the same address twice in the same transaction. This also shows that we can re-try normally if a send fails before it was written to disk.
The fee estimation test assumed that UTXOs would be locked after an error and couldn't be used anymore. This is no longer the case and we need to adjust the test. We clean up the test a bit while we're at it.
Because we now check all active and passive packets for uniqueness in their commitment keys, this issue has surfaced. If we're spending multiple different assets from the same on-chain input commitment, we used to create duplicate passive assets. That wasn't a problem until now because within the trees everything would just be overwritten and collapsed back to a single asset. But because have the uniqueness check on the virtual packet slice level, this duplication became apparent.
guggero
force-pushed
the
enforce-unique-script-key
branch
from
6a69824
to
ae69e0a
Compare
|
Thanks to the failing integration test I noticed two things that I needed to fix (with the latest push):
|
May be a motivation to use the |
|
I was trying to find out if the issue fixed in this PR also affects HTLCs with the same payment hash (e.g. MPP shards). cc @Roasbeef Will investigate further next week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1168 (or at least prevents it from happening in the future).
This PR enforces script keys to be unique per top-level MS-SMT tree (assetID/groupKey) within a single transfer.
To be able to test this properly, we also make sure selected UTXOs are released by the freighter if the shipment of a parcel results in an error.
That change then impacts some assumptions in the integration tests, which requires additional commits to fix.