Proof-system-v1 #1453
Draft
Proof-system-v1 #1453
Conversation
This file contains hidden or 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 14384345530Details
💛 - Coveralls |
Roasbeef
requested changes
Mar 28, 2025
asset/asset.go
Outdated
| // IsTransferRoot returns true if this asset represents a root transfer. A root | ||
| // transfer is an asset that is neither a genesis asset nor contains split | ||
| // commitment witness data. | ||
| func (a *Asset) IsTransferRoot() bool { |
There was a problem hiding this comment.
So the split commitment witness distinction is important as the v2 proofs will only go in the anchor output?
There was a problem hiding this comment.
Correct. However, there is the todo of adding the v2 proofs to the SplitRootProof. You commented on that yourself as well: https://github.com/lightninglabs/taproot-assets/blob/af9066c4aeb34207f7be4ca3de28da92c4f42836/proof/append.go#L238-L239
| @@ -682,6 +682,8 @@ func checkOutputCommitments(t *testing.T, vPkt *tappsbt.VPacket, | |||
| default: | |||
| require.Fail(t, "unknown vPacket version") | |||
| } | |||
|
|
|||
| // TODO(jhb): Check for correct STXOs | |||
github-project-automation
bot
moved this from 🏗 In progress
to 👀 In review
in Taproot-Assets Project Board
gijswijs
force-pushed
the
proof-system-v2
branch
from
4eef445 to
f65456c
Compare
guggero
reviewed
Apr 4, 2025
| @@ -2026,15 +2038,6 @@ func ValidateAnchorInputs(anchorPacket *psbt.Packet, packets []*tappsbt.VPacket, | |||
| ) | |||
| } | |||
|
|
|||
| // Each input must have a valid set of AltLeaves at this point. | |||
There was a problem hiding this comment.
Since there might've already been alt leaves in the vPacket, should we still validate them? Or is this added back in another place in another commit? Still going through the diff.
There was a problem hiding this comment.
I'm not sure if I follow. This is a sanity check that makes sure we have at least a valid set of AltLeaves. If we don't have them here during ValidateAnchorInputs we can fail early.
Yes, we do check them during FundPacket as well, so it should not be possible to have invalid AltLEaves at this point in the code, but it doesn't hurt to check here. Also, future flows might be different and warrant a check here even more.
| for idx, assets := range assetsByOutput { | ||
| for idx := range assets { | ||
| if !assets[idx].HasSplitCommitmentWitness() { | ||
| stxoAssetsByOutput := make(map[uint32][]asset.AltLeaf[asset.Asset]) |
There was a problem hiding this comment.
Should we also add some negative tests where we expect a failure?
There was a problem hiding this comment.
That's easier said than done. My commit only "fixes" the TestCreateProofSuffix itest, which tests the successful creation of suffix proofs for a given anchor transaction. It's probably easier to create a new test that tests for a failure condition.
There was a problem hiding this comment.
Yeah, that would be good.
Can assist with that test, have a couple of ideas.
gijswijs
force-pushed
the
proof-system-v2
branch
3 times, most recently
from
9c4d5c9 to
0055fb3
Compare
gijswijs
force-pushed
the
proof-system-v2
branch
from
0055fb3 to
7af5271
Compare
gijswijs
force-pushed
the
proof-system-v2
branch
from
7af5271 to
e0e4bf8
Compare
Because we retrieve AltLeaves from the proof during anchor input validation, we don't need to store them in the vInputs struct. This commit removes the AltLeaves field from the vInputs struct and the associated methods.
This bool accidentally was set to `true`. It does not affect the logic, but it does affect the debug logging in `deriveCommitmentKeys`. By correcting the value to `false`, we ensure that the debug logging mentions the correct prooftype: Exclusion.
Since we're only ever passing nil (and we use the previous witness in the script/burn key), we just remove the witness parameter completely.
This commit stores the STXOs in the output commitments using AltLeaves.
Co-authored-by: Gijs van Dam <gijs@lightning.engineering>
Instead of using alt leaves from the inputs, we use the alt leaves from the proof. We also remove the deduplication func `AddLeafKeysVerifyMerge` because it's not needed anymore.
This commit adds the STXO exclusion proof to the proof parameters. It stores them in `params.ExclusionProofs[].CommitmentProof.STXOProofs[]` where they are later encoded when creating the actual proofs.
`verifyExclusionProofs` now checks if the stxo exclusion proofs are present in the `ExclusionProofs[idx].CommitmentProof` and if so will verify those in preference of the old style proofs.
This commit adds an itest that test whether the stxo exclusion proof is added to the proof suffix and whether the proof is actually a valid exclusion proof for the minimal asset we expect to see in the stxo set.
This commit removes the calls to AssertInputsUnique. We can now rely on stxo exclusion proof verification instead of relying on the uniqueness check.
This commit adds v2 inclusion proof verification.
The proof version is bumped to indicate the usage and support of stxo style proofs.
guggero
force-pushed
the
proof-system-v2
branch
from
e0e4bf8 to
c971e08
Compare
|
Extracted some commits into #1465 to ease review on this one. Pushed up a rebase. |
guggero
reviewed
Apr 11, 2025
| @@ -191,6 +232,7 @@ func CreateTransitionProof(prevOut wire.OutPoint, | |||
| return nil, fmt.Errorf("root asset mismatch") | |||
| } | |||
|
|
|||
| // TODO(jhb): add STXO inclusion proof for root prevIDs | |||
There was a problem hiding this comment.
I think we can remove the TODO, since only the root assets need to have an inclusion proof, since the split outputs themselves should be covered by the split commitment and exclusion proof.
But wanted to confirm with both of you first...
Comment on lines
+384
to
+404
| // Find the exclusion proofs for this output. | ||
| var eProof *proof.TaprootProof | ||
| for idx := range params.ExclusionProofs { | ||
| e := params.ExclusionProofs[idx] | ||
| if e.OutputIndex == outIndex { | ||
| eProof = ¶ms.ExclusionProofs[idx] | ||
| break | ||
| } | ||
| } | ||
| if eProof == nil { | ||
| return fmt.Errorf("no exclusion proof for "+ | ||
| "output %d", outIndex) | ||
| } | ||
|
|
||
| // There aren't any assets in that output, we can skip | ||
| // creating exclusion proofs for it. | ||
| if eProof.CommitmentProof == nil { | ||
| continue | ||
| } | ||
|
|
||
| commitmentProof := eProof.CommitmentProof |
There was a problem hiding this comment.
It looks to me like this part is independent of the stxoAssets. So it should perhaps be moved out of the loop? Also the map nil check and initialization below.
| // asset that is being created. We do this by confirming | ||
| // that the exclusion proof for the newly created asset | ||
| // is already present. | ||
| _, err = eProof.DeriveByAssetExclusion( |
There was a problem hiding this comment.
I don't see what this actually checks... AFAIK this wouldn't error out if this was an inclusion proof for example. I think it would just create an invalid proof but not return an error.
Could be wrong though.
| // Gather all P2TR outputs in the on-chain transaction. | ||
| p2trOutputs := make(map[uint32]struct{}) | ||
| // There is nothing to check so return early. | ||
| if p.ExclusionProofs == nil { |
There was a problem hiding this comment.
I think this is incorrect. Because that would mean I can just create a proof with no exclusion proof and I wouldn't have to prove anything.
The code below proves that we actually have an exclusion proof per p2tr output. So the exit early would only be if len(p2trOutputs) == 0.
| } | ||
|
|
||
| // All proofs must have similar versions. | ||
| firstVersion := maps.Values(commitVersions)[0][0] |
There was a problem hiding this comment.
Move this extraction of the first version into the verifySTXOVersions, since it's done the same way twice?
| p.ExclusionProofs[0].CommitmentProof != nil && | ||
| len(p.ExclusionProofs[0].CommitmentProof.STXOProofs) > 0 | ||
|
|
||
| if hasV1Proofs { |
There was a problem hiding this comment.
This whole logic is pretty hard to follow. And because of that, it's hard to reason about not removing any existing verification.
Can we perhaps change the flow that we always verify the V0 exclusion proofs.
And then in verifyV1ExclusionProofs we would only verify the STXO proofs?
Here's a quick diff of what I mean:
diff --git a/proof/verifier.go b/proof/verifier.go
index 166e81a0..f25e0e0e 100644
--- a/proof/verifier.go
+++ b/proof/verifier.go
@@ -83,6 +83,8 @@ type BaseVerifier struct {
// transaction.
type P2TROutputsSTXOs = map[uint32]fn.Set[asset.SerializedKey]
+type CommittedVersions = map[uint32][]commitment.TapCommitmentVersion
+
// Verify takes the passed serialized proof file, and returns a nil
// error if the proof file is valid. A valid file should return an
// AssetSnapshot of the final state transition of the file.
@@ -232,11 +234,6 @@ func (p *Proof) verifySplitRootProof() error {
func (p *Proof) verifyExclusionProofs() (*commitment.TapCommitmentVersion,
error) {
- // There is nothing to check so return early.
- if p.ExclusionProofs == nil {
- return nil, nil
- }
-
// Gather all P2TR outputs in the on-chain transaction. The STXO proofs
// are tracked using a set of serialized keys. We ignore that set when
// verifying V0 exclusion proofs.
@@ -252,6 +249,14 @@ func (p *Proof) verifyExclusionProofs() (*commitment.TapCommitmentVersion,
p2trOutputs.Add(uint32(i))
}
+ commitVersions := make(CommittedVersions)
+
+ p2trOutputCopy := fn.NewSet(p2trOutputs.ToSlice()...)
+ err := p.verifyV0ExclusionProofs(p2trOutputCopy, commitVersions)
+ if err != nil {
+ return nil, err
+ }
+
// Early pass to determine if we're dealing with v0 or v1 proofs.
hasV1Proofs := p.IsVersionV1() &&
len(p.ExclusionProofs) > 0 &&
@@ -259,54 +264,66 @@ func (p *Proof) verifyExclusionProofs() (*commitment.TapCommitmentVersion,
len(p.ExclusionProofs[0].CommitmentProof.STXOProofs) > 0
if hasV1Proofs {
- return p.verifyV1ExclusionProofs(p2trOutputs)
+ p2trOutputCopy = fn.NewSet(p2trOutputs.ToSlice()...)
+ err := p.verifyV1ExclusionProofs(p2trOutputCopy, commitVersions)
+ if err != nil {
+ return nil, err
+ }
+ }
+
+ // If no commitments in proofs, no version to return.
+ if len(commitVersions) == 0 {
+ return nil, nil
}
- return p.verifyV0ExclusionProofs(p2trOutputs)
+ // All proofs must have similar versions.
+ firstVersion := maps.Values(commitVersions)[0][0]
+ return verifySTXOVersions(firstVersion, commitVersions)
}
// verifyV0ExclusionProofs verifies all version 1 exclusion proofs.
-func (p *Proof) verifyV0ExclusionProofs(p2trOutputs fn.Set[uint32]) (
- *commitment.TapCommitmentVersion, error) {
-
- commitVersions := make(map[uint32][]commitment.TapCommitmentVersion)
+func (p *Proof) verifyV0ExclusionProofs(p2trOutputs fn.Set[uint32],
+ commitVersions CommittedVersions) error {
// Verify all of the encoded v0 exclusion proofs.
for idx := range p.ExclusionProofs {
exclusionProof := p.ExclusionProofs[idx]
- err := p.handleBasicExclusionProof(
- &exclusionProof, p2trOutputs, commitVersions,
+
+ derivedCommitment, err := verifyTaprootProof(
+ &p.AnchorTx, &exclusionProof, &p.Asset, false,
)
if err != nil {
- return nil, err
+ return fmt.Errorf("error verifying STXO proof: %w", err)
+ }
+
+ outputIdx := exclusionProof.OutputIndex
+ delete(p2trOutputs, exclusionProof.OutputIndex)
+
+ // Store commitment version if Taproot Asset commitment present.
+ if derivedCommitment != nil {
+ commitVersions[outputIdx] = append(
+ commitVersions[outputIdx],
+ derivedCommitment.Version,
+ )
}
}
// If any outputs are missing a proof, fail.
if len(p2trOutputs) > 0 {
- return nil, ErrMissingExclusionProofs
- }
-
- // If no commitments in proofs, no version to return.
- if len(commitVersions) == 0 {
- return nil, nil
+ return ErrMissingExclusionProofs
}
- // All proofs must have similar versions.
- firstVersion := maps.Values(commitVersions)[0][0]
- return verifySTXOVersions(firstVersion, commitVersions)
+ return nil
}
// verifyV1ExclusionProofs verifies all version 2 exclusion proofs.
-func (p *Proof) verifyV1ExclusionProofs(
- p2trOutputs fn.Set[uint32]) (*commitment.TapCommitmentVersion, error) {
-
- commitVersions := make(map[uint32][]commitment.TapCommitmentVersion)
+func (p *Proof) verifyV1ExclusionProofs(p2trOutputs fn.Set[uint32],
+ commitVersions CommittedVersions) error {
// Collect the STXOs from the new asset.
stxoAssets, err := asset.CollectSTXO(&p.Asset)
if err != nil {
- return nil, fmt.Errorf("error collecting STXO assets: %w", err)
+ return fmt.Errorf("error collecting STXO assets: %w", err)
}
// Create a P2TROutputsSTXOs from p2trOutputs and map STXOs by
@@ -327,68 +344,22 @@ func (p *Proof) verifyV1ExclusionProofs(
}
for idx := range p.ExclusionProofs {
- baseProof := p.ExclusionProofs[idx]
- if baseProof.TapscriptProof != nil &&
- baseProof.TapscriptProof.Bip86 {
-
- // This is a BIP-0086 wallet output (likely a change
- // output) that does not commit to any script or Taproot
- // Asset root. There is no special stxo proof handling
- // needed, so we default to `handleBasicExclusionProof`.
- err := p.handleBasicExclusionProof(
- &baseProof, p2trOutputs, commitVersions,
- )
- if err != nil {
- return nil, err
- }
+ exclusionProof := p.ExclusionProofs[idx]
+ if exclusionProof.TapscriptProof != nil &&
+ exclusionProof.TapscriptProof.Bip86 {
+
continue
}
_, err := verifySTXOProofSet(
- &p.AnchorTx, baseProof, assetMap, p2trOutputsSTXOs,
+ &p.AnchorTx, exclusionProof, assetMap, p2trOutputsSTXOs,
commitVersions, false,
)
if err != nil {
- return nil, err
+ return err
}
}
- err = p.verifyRemainingOutputs(p2trOutputsSTXOs)
- if err != nil {
- return nil, err
- }
-
- if len(commitVersions) == 0 {
- return nil, nil
- }
-
- // All proofs must have similar versions.
- firstVersion := maps.Values(commitVersions)[0][0]
- return verifySTXOVersions(firstVersion, commitVersions)
-}
-
-// handleBasicExclusionProof handles a basic (non-STXO) exclusion proof.
-func (p *Proof) handleBasicExclusionProof(baseProof *TaprootProof,
- p2trOutputs fn.Set[uint32],
- commitVersions map[uint32][]commitment.TapCommitmentVersion) error {
-
- derivedCommitment, err := verifyTaprootProof(
- &p.AnchorTx, baseProof, &p.Asset, false,
- )
- if err != nil {
- return fmt.Errorf("error verifying STXO proof: %w", err)
- }
-
- outputIdx := baseProof.OutputIndex
- delete(p2trOutputs, baseProof.OutputIndex)
-
- // Store commitment version if Taproot Asset commitment present.
- if derivedCommitment != nil {
- commitVersions[outputIdx] = append(
- commitVersions[outputIdx], derivedCommitment.Version,
- )
- }
-
return nil
}
@@ -450,33 +421,6 @@ func MakeSTXOProof(baseProof TaprootProof,
}
}
-// verifyRemainingOutputs verifies any remaining outputs are BIP-86 outputs.
-func (p *Proof) verifyRemainingOutputs(p2trOutputs P2TROutputsSTXOs) error {
- if len(p2trOutputs) == 0 {
- return nil
- }
-
- // Check remaining outputs are BIP-86 outputs.
- for outIdx := range p2trOutputs {
- for _, proof := range p.ExclusionProofs {
- if proof.OutputIndex != outIdx {
- continue
- }
- if proof.TapscriptProof != nil &&
- proof.TapscriptProof.Bip86 {
-
- delete(p2trOutputs, outIdx)
- }
- }
- }
-
- if len(p2trOutputs) > 0 {
- return ErrMissingExclusionProofs
- }
-
- return nil
-}
-
// verifySTXOVersions verifies all STXO versions match.
func verifySTXOVersions(firstVersion commitment.TapCommitmentVersion,
versions map[uint32][]commitment.TapCommitmentVersion) (
diff --git a/tapsend/proof.go b/tapsend/proof.go
index 018857fd..4773ef9a 100644
--- a/tapsend/proof.go
+++ b/tapsend/proof.go
@@ -366,6 +366,33 @@ func addSTXOExclusionProofs(outputs []*tappsbt.VOutput,
continue
}
+ // Find the exclusion proofs for this output.
+ var eProof *proof.TaprootProof
+ for idx := range params.ExclusionProofs {
+ e := params.ExclusionProofs[idx]
+ if e.OutputIndex == outIndex {
+ eProof = ¶ms.ExclusionProofs[idx]
+ break
+ }
+ }
+ if eProof == nil {
+ return fmt.Errorf("no exclusion proof for "+
+ "output %d", outIndex)
+ }
+
+ // There aren't any assets in that output, we can skip
+ // creating exclusion proofs for it.
+ if eProof.CommitmentProof == nil {
+ continue
+ }
+
+ commitmentProof := eProof.CommitmentProof
+ if commitmentProof.STXOProofs == nil {
+ commitmentProof.STXOProofs = make(
+ map[asset.SerializedKey]commitment.Proof,
+ )
+ }
+
tapTree := outputCommitments[outIndex]
for idx := range stxoAssets {
@@ -381,28 +408,6 @@ func addSTXOExclusionProofs(outputs []*tappsbt.VOutput,
return err
}
- // Find the exclusion proofs for this output.
- var eProof *proof.TaprootProof
- for idx := range params.ExclusionProofs {
- e := params.ExclusionProofs[idx]
- if e.OutputIndex == outIndex {
- eProof = ¶ms.ExclusionProofs[idx]
- break
- }
- }
- if eProof == nil {
- return fmt.Errorf("no exclusion proof for "+
- "output %d", outIndex)
- }
-
- // There aren't any assets in that output, we can skip
- // creating exclusion proofs for it.
- if eProof.CommitmentProof == nil {
- continue
- }
-
- commitmentProof := eProof.CommitmentProof
-
// Confirm that we are creating the stxo proofs for the
// asset that is being created. We do this by confirming
// that the exclusion proof for the newly created asset
@@ -417,13 +422,6 @@ func addSTXOExclusionProofs(outputs []*tappsbt.VOutput,
"stxo proofs: %w", err)
}
- //nolint:lll
- if commitmentProof.STXOProofs == nil {
- commitmentProof.STXOProofs = make(
- map[asset.SerializedKey]commitment.Proof,
- )
- }
-
commitmentProof.STXOProofs[identifier] = *exclusionProof
}
}Or maybe it's easier as a wip commit: guggero@a2a1633
| return nil, err | ||
| } | ||
|
|
||
| err = p.verifyRemainingOutputs(p2trOutputs) |
There was a problem hiding this comment.
I don't think we need to call into this. We only added a single output index to the map and we expect that to be empty now, after the STXO verification. So we just need to check for map emptiness.
| for idx, assets := range assetsByOutput { | ||
| for idx := range assets { | ||
| if !assets[idx].HasSplitCommitmentWitness() { | ||
| stxoAssetsByOutput := make(map[uint32][]asset.AltLeaf[asset.Asset]) |
There was a problem hiding this comment.
Yeah, that would be good.
Can assist with that test, have a couple of ideas.
| @@ -252,7 +253,8 @@ func (p *Proof) verifyExclusionProofs() (*commitment.TapCommitmentVersion, | |||
| } | |||
|
|
|||
| // Early pass to determine if we're dealing with v0 or v1 proofs. | |||
| hasV1Proofs := len(p.ExclusionProofs) > 0 && | |||
| hasV1Proofs := p.IsVersionV1() && | |||
There was a problem hiding this comment.
If this is a V1 proof and there are no STXO proofs, should that result in an error?
Same for the inclusion proofs above.
gijswijs
force-pushed
the
proof-system-v2
branch
from
c971e08 to
da7b212
Compare
guggero
force-pushed
the
extract-refactors-from-proof-v1
branch
2 times, most recently
from
171f7ae to
c8a7971
Compare
|
Sorry, I don't think I made things easier for you as an author by splitting the PR into two. And now I noticed that the commit c9b1d54 does need to go into this one instead... |
gijswijs
force-pushed
the
proof-system-v2
branch
from
da7b212 to
6badb61
Compare
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.
This pull request introduces several enhancements and refactors related to inclusion and exclusion proof handling. The key changes include:
AltLeaves.proof/verifier.goConsiderations
multi: remove AssertInputsUnique. Although with this proof system we are able to parse a proof for non-unique inputs, older nodes cannot. So for now this check should probably remain in place.