assets: Add shared asset tooling - #1225
Conversation
c206f0d to
2814bf6
Compare
hieblmi
added
asset loop out
hieblmi
changed the title
4283082 to
50595de
Compare
|
/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.
|
Follow-up commit 818876e also adds documentation to the unexported production and test helpers listed in the general review. Local verification passed: go test ./..., go test -race ./assets/..., go vet ./..., golangci-lint (0 issues), plus a Nautilus #1712 itest-package compile with integration tags. |
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.
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.
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.
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.
818876e to
81e876c
Compare
| return d.anchorRootFromProofCommitment(depositProof) | ||
| } | ||
|
|
||
| // VerifyProofFile asks tapd to fully verify a proof file, binds its terminal |
There was a problem hiding this comment.
Could we make this method enforce full provenance rather than leaving that distinction to callers?
VerifyProofResponse.Valid does not necessarily mean that a proof history is rooted at issuance. In Taproot Assets v0.8.3:
- tapd's
VerifyProofcallsFile.Verifyand does not bindGenesisPoint(rpcserver.go:2159). File.Verifystarts the first proof withprev == nil(verifier.go:1565).- In that state,
Proof.Verifyaccepts a non-nilChallengeWitnessas an ownership proof (verifier.go:1227). This is intentional behavior (proof_test.go:975).
Because the deposited asset uses an OP_TRUE script key, satisfying that ownership challenge is trivial. An attacker could anchor a leaf with the target genesis identity and claimed amount under the expected public Bitcoin contract. The current chain-inclusion, commitment, outpoint, amount, internal-key and sibling checks could then pass without proving legitimate issuance or transition history.
I think VerifyProofFile should provide the safe abstraction promised by its name:
- Snapshot
RawProofFileonce and verify and decode that same immutable content. - Reject empty files and non-nil
ChallengeWitnessvalues. - Require the first asset to satisfy
IsGenesisAsset(). - Apply those requirements recursively to every proof file in
AdditionalInputs. - Call tapd to verify the genesis reveal, group keys, chain inclusion, commitments and state transitions, then perform the existing terminal contract bindings.
IsGenesisAsset() is only a structural precondition; tapd's cryptographic verification is still required. Conversely, tapd verification alone permits the ownership-proof shortcut. Please add regressions for a standalone ownership proof and an ownership-rooted AdditionalInputs file.
Confirmation depth and current unspent status are separate live-chain properties. If callers should have one safe go/no-go operation, a higher-level VerifyDeposit can encapsulate those checks while keeping VerifyProofFile focused on provenance and exact contract binding.
I would also consider removing or unexporting the commitment-only VerifyProof before merge (kit.go:515). Keeping an attractively named unsafe alternative makes it easy for Loop server to bypass the safe path accidentally.
| // LegacyDepositV0 preserves the asset deposit HTLC contract that predates | ||
| // this shared package. | ||
| LegacyDepositV0 |
There was a problem hiding this comment.
What policy should asset loop-in use?
Do we keep LegacyDepositV0 for drafts written before?
| proofFileCopy := &taprpc.ProofFile{ | ||
| RawProofFile: append([]byte(nil), proofFile.RawProofFile...), | ||
| GenesisPoint: proofFile.GenesisPoint, | ||
| } | ||
| verifyResponse, err := verifier.VerifyProof(ctx, proofFileCopy) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to verify deposit proof file: %w", err) | ||
| } | ||
| if verifyResponse == nil || !verifyResponse.Valid { | ||
| return nil, fmt.Errorf("invalid deposit proof file") | ||
| } | ||
|
|
||
| decodedFile, err := proof.DecodeFile(proofFile.RawProofFile) |
There was a problem hiding this comment.
The method creates a private copy and sends that to tapd, but afterward decodes the caller-owned original at line 497. Mutation while the RPC is in flight can therefore make Loop consume bytes tapd did not verify.
Make one immutable snapshot and both verify and decode that exact snapshot. Add a verifier mock that mutates the caller’s original buffer to prevent regression.
| funderKey *btcec.PublicKey | ||
| coSignerKey *btcec.PublicKey |
There was a problem hiding this comment.
The deposit kit collapses four independent key roles into two.
Kit stores only funderKey and coSignerKey. The same pair is then:
- aggregated into the deposit’s MuSig2 internal key at
assets/deposit/kit.go:103-110; - used for the deposit refund script at
assets/deposit/kit.go:138-150; - passed to the HTLC as its sender and receiver keys at
assets/deposit/kit.go:273-285; - aggregated again as the HTLC internal key at
assets/htlc/swapkit.go:285-301; - used directly in the HTLC success and timeout leaves at
assets/htlc/swapkit.go:261-283.
This cannot represent the intended four direction-neutral roles:
SenderInternalPubKeyandReceiverInternalPubKeyform the cooperative MuSig2 internal key.SenderScriptKeyauthorizes the unilateral deposit refund and HTLC timeout paths.ReceiverScriptKeyauthorizes the HTLC preimage-success path.
“Sender” and “receiver” are defined by the on-chain asset direction: for Asset Loop In, Loop is the sender and Loop server is the receiver; for Asset Loop Out, the mapping is reversed. This is also the model already used by Bitcoin HTLCs in loopdb/loop.go:13-35 and swap/htlc.go:535-615.
There is no correct way for callers to map four keys into the current two-key constructor. Passing the internal keys makes the unilateral scripts use the wrong keys; passing the script keys makes the cooperative aggregate use the wrong keys. This changes the deposit address, HTLC output, control blocks, signing responsibilities, and recovery paths.
Please preserve all four roles as separate immutable constructor inputs. Aggregate only the sender and receiver internal keys, use the sender script key in refund/timeout leaves, and use the receiver script key in the preimage-success leaf. The shared SwapKit policy should derive and validate that same mapping for both Loop In and Loop Out.
| // validateSweep binds a PSBT input and witness UTXO to the HTLC proof and | ||
| // reconstructs the exact Taproot output being spent. | ||
| func (s *SwapKit) validateSweep(assetProof *proof.Proof, | ||
| sweepPacket *psbt.Packet) (*validatedSweep, error) { |
There was a problem hiding this comment.
Witness methods will sign asset-burning outputs.
Both SwapKit.validateSweep and deposit.Kit.validateSweep verify only that the selected input is the expected asset anchor. assets/htlc/swapkit.go:618-708 and assets/deposit/kit.go:583-665 return without inspecting any transaction output or binding the PSBT to a validated Taproot Asset transition.
The PSBT is then signed by CreatePreimageWitness at assets/htlc/swapkit.go:812-815, CreateTimeoutWitness at assets/htlc/swapkit.go:883-886, and deposit CreateTimeoutWitness at assets/deposit/kit.go:730-733. SIGHASH_DEFAULT commits to all outputs, but that does not make them asset-safe—the helper signs whatever outputs the caller supplied.
Bitcoin consensus validates the anchor spend, not Taproot Asset conservation. A transaction can therefore spend the expected P2TR anchor while omitting or replacing the destination asset commitment. Once confirmed, this destroys the active asset’s valid transition history and can also burn passive assets co-anchored in the same UTXO.
The tests currently demonstrate this gap: the HTLC fixture’s only output is a bare OP_TRUE output at assets/htlc/swapkit_test.go:744-747, and the deposit fixture does the same at assets/deposit/kit_test.go:353-355; both are nevertheless signed successfully.
Because this kit is intended to hide Taproot Asset internals from callers, output correctness should not be an implicit caller obligation. Before releasing a signature or witness, the shared API must bind the exact PSBT to the exact validated virtual transition and verify:
- the active asset ID and amount;
- the intended receiver asset script key;
- the anchor output index, value, internal key, sibling, commitment root, and resulting pkScript;
- all required change and passive-asset outputs;
- that the Bitcoin outputs being signed are exactly those produced by the validated Taproot Asset transition.
Tests should construct a real asset-preserving transition, then verify that mutations to the destination output, amount, keys, sibling, commitment, or passive-asset carry-forward are rejected before the signer is invoked.
| htlcAddr, err := client.NewAddr(ctx, &taprpc.NewAddrRequest{ | ||
| AssetId: d.assetID[:], | ||
| Amt: amount, | ||
| ScriptKey: rpcutils.MarshalScriptKey(tapScriptKey), | ||
| InternalKey: &taprpc.KeyDescriptor{ | ||
| RawKeyBytes: btcInternalKey.SerializeCompressed(), | ||
| }, | ||
| TapscriptSibling: siblingBytes, | ||
| }) |
There was a problem hiding this comment.
Medium — NewHtlcAddr and CreateHtlcVpkt describe different asset commitments.
This request leaves both version fields at their protobuf zero values. With Taproot Assets v0.8.3, that selects Asset V0 and leaves the address version unspecified; tapd currently resolves the latter to Address V1. In contrast, CreateHtlcVpkt explicitly creates VPacket V1 and Asset V1 outputs at assets/htlc/swapkit.go:373-397.
The asset version is serialized into the asset leaf, so changing it changes the Taproot Asset commitment root and ultimately the Bitcoin output key. The address returned here and the VPacket returned by the same SwapKit therefore do not construct the same HTLC output. Relying on tapd's unspecified address-version default also leaves part of the contract outside the immutable policy.
Please freeze every relevant version in the policy—at minimum the address, asset, VPacket, and TapCommitment versions—and use those values consistently in both address requests and VPacket construction. Proof-file and transition-proof versions accepted by validation should be frozen there as well. Validate the versions returned by tapd, and add a test showing that the decoded address and the funded VPacket derive the same output key.
(also applies to assets/deposit/kit.go:212)
| if err != nil { | ||
| return nil, fmt.Errorf("invalid receiver public key: %w", err) | ||
| } | ||
| if senderKey.IsEqual(receiverKey) { |
There was a problem hiding this comment.
Low — distinct-key validation compares full points even though the contracts use x-only keys.
btcec.PublicKey.IsEqual distinguishes P from -P, but BIP340/Taproot serializes both points to the same x-only public key. A caller can therefore provide opposite-parity encodings of the same key and bypass the "keys must differ" check, even though one underlying signing key controls both nominal roles in the resulting contract.
Please compare schnorr.SerializePubKey encodings after parsing and apply the same x-only distinctness rule to every policy role that must be independently controlled. Add a regression test using P and -P.
| proofFileCopy := &taprpc.ProofFile{ | ||
| RawProofFile: append([]byte(nil), proofFile.RawProofFile...), | ||
| GenesisPoint: proofFile.GenesisPoint, | ||
| } |
There was a problem hiding this comment.
Low — VerifyProofFile duplicates oversized proofs before enforcing Taproot Assets' size limit.
The first operation on the raw proof is an unbounded append copy. Tapd's VerifyProof RPC applies proof.CheckMaxFileSize, but that happens only after this allocation and after the buffer crosses the pluggable verifier boundary. An oversized untrusted proof can therefore force a second allocation of the full payload before it is rejected.
Please capture the input slice, call proof.CheckMaxFileSize before allocating the immutable snapshot, and then use that one snapshot for both verification and decoding. Oversized input should be rejected before invoking the verifier.
| } | ||
|
|
||
| return client.NewAddr(ctx, &taprpc.NewAddrRequest{ | ||
| AssetId: d.assetID[:], |
There was a problem hiding this comment.
Low — the RPC request exposes mutable storage from the supposedly immutable deposit kit.
d.assetID[:] is a slice backed directly by the array stored in Kit. Because AddressProofClient is a pluggable interface, an implementation or interceptor can retain and mutate request.AssetId, changing d.assetID after construction. On the HTLC path this can also make the address response disagree with the SwapKit that was created before the RPC call.
Please copy AssetId into request-owned storage before crossing the interface boundary in both address methods. A mock that mutates and retains the request slice should demonstrate that subsequent kit operations remain unchanged.
| // CreateHtlcVpkt creates the version-one virtual packet for the HTLC. The | ||
| // split-root and HTLC output indices and their interactive flags are consensus | ||
| // with the existing server implementation. |
There was a problem hiding this comment.
Non-blocking documentation request — clarify that CreateHtlcVpkt returns a funding template.
The zero-valued interactive split-root placeholder at assets/htlc/swapkit.go:381-388 is correct for tapd's funding pipeline, but it is not valid as a final VPacket: PrepareOutputAssets rejects interactive zero-valued outputs. FundVirtualPsbt first converts the template to allocations, replaces the placeholder amount with actual change, and omits the local output entirely for a full-value interactive transfer.
Could the method documentation state that its result must pass through FundVirtualPsbt and must not be validated or committed as a final VPacket? Renaming it to something like CreateHtlcFundingTemplate would make the contract even clearer, but documentation is sufficient here. A funded-template test covering both exact-value and change cases would preserve this subtle dependency.
| // CreateOpTrueLeaf creates the legacy Taproot Asset script key whose only | ||
| // script path is OP_TRUE beneath the public Taproot Assets NUMS key. |
There was a problem hiding this comment.
Low — the policy permanently fixes a shared OP_TRUE asset script key.
CreateOpTrueLeaf has no contract-specific input, so all deposits and HTLCs use the same asset script key. This matches the historical prototype and is valid for individual swaps, but it prevents multiple outputs for the same asset from being constructed in one transaction and precludes script-key-addressed proof delivery without another disambiguation mechanism.
Please document whether those limitations are intentional for this immutable policy. If same-transaction batching or script-key-addressed proof delivery is required later, a new policy will need a deterministic contract-specific discriminator while retaining OP_TRUE as its spendable script path.
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