assets: Add shared asset tooling - #1225
Conversation
f805e70 to
c206f0d
Compare
Move the existing server asset HTLC contract into Loop so both sides can derive and spend the same commitment. Freeze the legacy vectors and bind witness construction to verified proofs, prevouts, and input indices. Reserve future policies so Loop Asset Out can choose its contract explicitly instead of inheriting the deposit key path.
c206f0d to
2814bf6
Compare
hieblmi
added
asset loop out
hieblmi
changed the title
a33d139 to
4283082
Compare
Consolidate the remaining deposit and OP_TRUE virtual-packet helpers behind Loop-owned packages. Replace positional sweep assumptions with proof-bound input selection, complete prevout validation, and explicit signature verification.
4283082 to
50595de
Compare
Reject non-block deposit expiries. This prevents BIP68 flags from changing or disabling the intended block delay. Canonicalize OP_TRUE keys, populate and validate every virtual input witness, and reject unsupported addresses and duplicate anchor inputs.
Return the complete anchor Merkle root from proof verification. This lets MuSig2 spends reproduce the output's Taproot tweak.
|
/gateway review |
|
👀 gateway review starting… |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Style — ~18 unexported funcs and ~14 test helpers have no godoc, against the convention. Non-test: canonicalAddressParams, cloneAddressParams (both copies), validateScripts, genBtcControlBlock, validateAsset (both), findUniqueInput (both), validateSweep (both), validateSequence, verifyTapscriptSignature (both), timeoutPathSibling, encodedTimeoutPathSibling, newHtlcSwapKit, validateProof.
| } | ||
|
|
||
| sweepVpkt, err := tappsbt.FromProofs( | ||
| proofs, addr.ChainParams, tappsbt.V1, |
There was a problem hiding this comment.
Above we accept both V0 and V1, but map both to V1. V0 addresses accepted, but built as a V1/interactive send.
- Upstream's own mapping (
tappsbt/create.go:39-45) is V0→V0, V1→V1, andtappsbt.CommitmentVersion(tappsbt/interface.go:198-210) turnsV1into a mandatoryTapCommitmentV2whileV0leaves it open. A V0 address therefore gets an output commitment it doesn't describe. - The other half is worse: upstream builds V0/V1 address sends as non-interactive with a split-root tombstone, with an explicit comment that V0/V1 receivers need that to predict the anchor output key. Loop builds a single
Interactive: truefull-value output. So for V0 and V1 addresses the interactivity is wrong too.
address.V0 == 0, so an unsetVersionsilently takes this path — and that's exactly what the fixtures do (assets/tapkit_test.go:64,113constructaddress.Tapwith noVersion, paired withTapCommitmentV2proofs). The only success-path test that sets a version usesaddress.V1. The V0 path has no coverage and fails silently.
Fix: reject non-V1.
| if assetProof == nil { | ||
| return nil, fmt.Errorf("asset proof %d is nil", idx) | ||
| } | ||
| if _, err := assetProof.VerifyProofs(); err != nil { |
There was a problem hiding this comment.
malformed proofs panic inside VerifyProofs
Loop's exposure by call ordering: assets/tapkit.go:66 calls VerifyProofs before its script-key nil check at :73 (exposed to both); exported GenTaprootAssetRootFromProof (swapkit.go:489) validates nothing (exposed to both); deposit/kit.go:353 and swapkit.go:614 guard the script key first but check the internal key only afterwards at :357 — or never (exposed to the internal-key panic).
| if !bytes.Equal(d.assetID[:], proofID[:]) { | ||
| return fmt.Errorf("asset proof ID does not match deposit") | ||
| } | ||
| if depositAsset.Amount == 0 { |
There was a problem hiding this comment.
validateAsset only rejects Amount == 0. Kit stores no expected amount even though NewAddr takes one, so VerifyProof succeeds for any positive amount of the right asset at the right script key. A caller reading it as "the deposit is funded" would accept an underfunded deposit. (GroupKey is also unchecked, unlike the swapkit path.)
| return nil, err | ||
| } | ||
|
|
||
| tapCommitment, err := depositProof.VerifyProofs() |
There was a problem hiding this comment.
VerifyProofs() is a commitment-structure check, not a proof-validity check, and VerifyProof exposes its result as a standalone verdict.
Proof.VerifyProofs() verifies only the inclusion proof, the split-root proof (when the asset has a split commitment witness), the exclusion proofs, and that the inclusion/exclusion commitment versions are consistent. It deliberately stops there — compare it with VerifyProofIntegrity, which additionally checks the proof version, Asset.Validate(), TxSpendsPrevOut, and runs HeaderVerifier + MerkleVerifier plus the genesis/group reveals; and with Proof.Verify, which on top of that runs verifyAssetStateTransition (previous witnesses and amount conservation through the asset VM).
So what we establish here is: "some Taproot Asset commitment containing this asset leaf is committed to by this anchor output, and the sibling and internal key are the ones this deposit expects." What we do not establish:
- that the anchor transaction is in a block (no header or merkle verification);
- that the asset has valid provenance back to genesis, or a valid group key;
- amount conservation from the previous witnesses;
- that the anchor outpoint is confirmed, or still unspent.
A single *proof.Proof cannot give us provenance at all — the chain back to genesis lives in the proof.File.
Why this call site specifically
The other VerifyProofs() callers in this branch are fine as-is. validateSweep (kit.go:467) and htlc.SwapKit.validateSweep / GetPkScriptFromProof use the returned commitment to reconstruct a pkScript that is then compared against the prevout being spent (kit.go:514-536). A forged proof there just yields a script that does not exist on chain, and the sweep fails — no trust decision is taken.
VerifyProof (kit.go:389) is different: it returns the anchor root and nothing else, and its name and doc invite a caller to read it as "this deposit is real." It is the one place where a commitment-only result escapes as a verdict someone can act on. Once Nautilus or Asset Loop In consumes this package, that is the line where a counterfeit or unconfirmed asset state would be accepted.
Suggested fix
Split the two concerns so the trust decision cannot be reached by accident:
- Keep today's cheap reconstruction, renamed to say what it does (e.g.
AnchorRootFromProofCommitment), with a doc note that it establishes nothing about the asset's validity or on-chain existence. - Add one explicitly-named verification entry point that takes a
*proof.Fileplus an expected amount. Options, roughly in order of fit for a client:- Delegate to tapd.
taprpc.TaprootAssets.VerifyProof(ProofFile) → {Valid, DecodedProof}already exists in our pinnedtaprpc v1.2.0, and tapd has a fully wiredVerifierCtxagainst its own chain backend.AddressProofClient(kit.go:33-38) already narrows tapd toNewAddr/ExportProof; addingVerifyProofis a small extension in the existing style and needs no dependency bump. - Verify in-process via
proof.Verifywith a realVerifierCtx, backingHeaderVerifier/MerkleVerifierwithlndclient'sChainKit.GetBlockHeader/GetBlock(available in our pinned lndclient; loop does not use ChainKit today, so this is new plumbing). - For the unconfirmed case — which is what Asset Loop In actually needs, since HTLC funding has to be validated before the anchor confirms —
proof.VerifyProofSuffix. It runs proof integrity, inclusion/exclusion, anchor input spending checks and the asset VM against the supplied input files, skipping only the suffix's own header, merkle proof and timelock checks. Note it does not exist in v0.8.1; it is new on taproot-assets main, so it would require bumping the pin.
- Delegate to tapd.
Whichever we pick, these remain our job at the kit level, because no verifier does them for us:
- proof outpoint equals the outpoint we expect, and its pkScript equals the one we derived ourselves from this
Kit(we already do this invalidateSweep;VerifyProofdoes not); - amount equals the expected amount, and asset ID / group key match;
- confirmation depth, from our own chain source;
- the anchor outpoint is still unspent — no proof can ever tell us this.
Given the package has no callers yet, this does not have to block landing the foundation, but it should be settled before anything treats these helpers as a validation boundary.
| muSig2Key, err := input.MuSig2CombineKeys( | ||
| input.MuSig2Version100RC2, | ||
| []*btcec.PublicKey{funderKeyCopy, coSignerKeyCopy}, sortKeys, | ||
| &input.MuSig2Tweaks{TaprootBIP0086Tweak: true}, |
There was a problem hiding this comment.
TaprootBIP0086Tweak: true here is inert, and it is a footgun to leave in place.
Only PreTweakedKey is ever read from muSig2Key (kit.go:198, :359, :419, :429, :529), and btcd snapshots that value before applying any tweak (schnorr/musig2/keys.go:395-399; the taproot branch at :403 normalizes a local copy and mutates only finalKeyJ → FinalKey). So this flag cannot change any key this kit produces, parity included.
I think this is residue from adapting staticaddr/script/script.go:62-99, which is the same shape — two keys plus a CSV timeout leaf — but computes rootHash from the script tree up front and keeps both PreTweakedKey and FinalKey. That does not work for an asset anchor: the root is tapCommitment.TapscriptRoot(siblingHash) and is unknown until the deposit lands and we hold a proof, which is exactly why VerifyProof returns it. TaprootBIP0086Tweak is the only tweak variant that needs no root hash, so it satisfies the struct without data we do not have yet.
Worth removing rather than just tidying, because MuSig2Tweaks is also the signing-time config (ToContextOptions / MuSig2CreateContext) and TaprootBIP0086Tweak takes precedence over TaprootTweak — lnd ignores the script root when the BIP86 flag is set (input/musig2.go:220-229, :358-363). If someone later stores this struct on the Kit and adds the real anchor root for the MuSig2 key-path spend without spotting the flag, the root is silently discarded and they get BIP86 key-spend-only signatures for an output that has both a CSV leaf and an asset commitment.
Suggest matching htlc.GetAggregateKey (swapkit.go:289), which passes an empty tweak set and documents the intent:
&input.MuSig2Tweaks{},plus a comment noting that the BIP341 tweak is applied later at signing time, using the anchor root from VerifyProof.
| return nil, err | ||
| } | ||
| sweepTx := sweepPacket.UnsignedTx.Copy() | ||
| sweepTx.TxIn[sweep.assetInputIndex].Sequence = d.csvExpiry |
There was a problem hiding this comment.
Divergent contracts: deposit.CreateTimeoutWitness sets the CSV sequence itself and writes it back into the caller's PSBT (kit.go:591,631); htlc.CreateTimeoutWitness requires the caller to have set it and rejects any mismatch. Easy to get wrong.
| func cloneAddressParams(params address.ChainParams) *address.ChainParams { | ||
| paramsCopy := params | ||
| if params.Params != nil { | ||
| bitcoinParams := *params.Params |
There was a problem hiding this comment.
This derefs params.Params unguarded; the htlc twin guards it. Safe via NewKit today, a landmine if reused.
Align taprpc and LND with the Taproot Assets v0.8.3 dependency graph. Raise the minimum Go build version to 1.25.13 and refresh module sums for both the main and client RPC modules.
3 participants
Summary
SwapKit, legacy deposit kit, and generic OP_TRUE virtual-packet sweephelper
LegacyDepositV0policy, pinned by golden script, key, anchor, witness, andvirtual-packet vectors
with Go 1.25.13 and classic btcd; this prototype does not add tap-sdk
Security and correctness boundaries
The shared kit verifies proofs before using them and binds every Bitcoin
spend to the proof's exact anchor outpoint, output value, script, commitment
root, and unique PSBT input. It supplies every prevout to the signer, applies
the required CSV sequence to the matched input without mutating the caller's
PSBT on failure, rejects malformed signer responses, verifies the returned
Schnorr signature, and returns the matched input index to the caller.
The OP_TRUE helper rejects empty, nil, invalid, non-OP_TRUE, mixed-asset,
overflowing, and amount-mismatched proof sets. It derives the network from the
destination address and validates the prepared output and split-root witness
before attaching the asset witness.
Network validation distinguishes shared testnet HRPs by Bitcoin network
magic. Simnet explicitly accepts both btcd's native BIP-0044 coin type 115 and
lnd's testnet-compatible coin type 1 without relying on mutable global state.
Feature state machines remain responsible for trusted proof import,
canonical-chain and confirmation tracking, reorg handling, destination
validation, quote and fee limits, and durable recovery. This PR adds no Asset
Loop Out RPC, funding flow, persistence, or state machine, and does not alter
conventional Loop In or Asset Loop In.
This is prototype infrastructure, not a release or rollout change.
Verification
GOTOOLCHAIN=go1.25.13 go test ./... -count=1 -timeout=10mGOTOOLCHAIN=go1.25.13 go test -race ./assets/... -count=1GOTOOLCHAIN=go1.25.13 go vet ./assets/...GOTOOLCHAIN=go1.25.13 CGO_ENABLED=0 go build -tags=dev ./cmd/loop ./cmd/loopdGOTOOLCHAIN=go1.25.13 go mod verifyin the root andlooprpcmodulesGOTOOLCHAIN=go1.25.13 go mod tidy -diffin the root,looprpc, andswapserverrpcmodulesGOTOOLCHAIN=go1.25.13 go test ./... -count=1in thelooprpcandswapserverrpcmodulesmake commitmsg-lint range=origin/master..HEADgit diff --check origin/master..HEAD