-
Notifications
You must be signed in to change notification settings - Fork 39
Intent proof of ownership rework #722
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
Conversation
WalkthroughReplaces BIP-0322 proof/signature types and flows with a PSBT-based "Intent" model across OpenAPI, protobufs, core services, libraries, wallet signing, tx-builder, parsers, and tests; deletes the bip322 package and introduces the intent package and helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant GRPC as gRPC Handler
participant Parser
participant Service
participant Wallet
participant Intent as intent.Proof
participant Script as FinalizeVtxoScript
rect rgb(241,245,249)
Note over Client,GRPC: RegisterIntent (intent proof flow)
Client->>GRPC: RegisterIntent(req: v1Intent)
GRPC->>Parser: parseRegisterIntent(req.intent)
Parser->>Intent: decode PSBT + message
Parser-->>GRPC: (proof, registerMessage)
end
GRPC->>Service: RegisterIntent(ctx, proof, registerMessage)
rect rgb(240,253,244)
Service->>Intent: Verify(proofB64, message)
Service->>Wallet: SignTransactionTapscript(psbt)
Wallet-->>Service: signed PSBT
Service->>Script: FinalizeVtxoScript(signed PSBT, inputIndex)
Script-->>Service: FinalScriptWitness set
Service-->>GRPC: intentID
GRPC-->>Client: intentID
end
sequenceDiagram
autonumber
participant Client
participant GRPC
participant Parser
participant Service
participant Intent
Note over Client,GRPC: DeleteIntentsByProof
Client->>GRPC: DeleteIntent(req: v1Intent)
GRPC->>Parser: parseDeleteIntent(req.intent)
Parser->>Intent: decode PSBT + delete message
Parser-->>GRPC: (proof, deleteMessage)
GRPC->>Service: DeleteIntentsByProof(ctx, proof, deleteMessage)
Service->>Intent: Verify(proofB64, message)
Service-->>GRPC: ok
GRPC-->>Client: ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
pkg/arkd-wallet/core/application/wallet/service.go (2)
458-466
: Bounds check to avoid panic on malformed PSBT prevout index.Accessing tx.TxOut[previousOutPoint.Index] without checking can panic on bad data.
- prevout := tx.TxOut[previousOutPoint.Index] + if int(previousOutPoint.Index) >= len(tx.TxOut) { + return "", fmt.Errorf("prevout index %d out of range (outputs: %d)", previousOutPoint.Index, len(tx.TxOut)) + } + prevout := tx.TxOut[previousOutPoint.Index]
657-691
: changeAmount underflow/overflow when actualFee > available change. Add guard and optional reselect.If actualFee exceeds allowance from the initial rough estimate (2 inputs), changeAmount underflows (uint64), later overflowing fee adjustment.
- actualFee := w.estimateWithdrawFee(feeRate, len(selectedUtxos), 2) // 2 outputs: destination + change - changeAmount := totalInputValue - amount - actualFee + actualFee := w.estimateWithdrawFee(feeRate, len(selectedUtxos), 2) // 2 outputs: destination + change + // Ensure inputs cover amount+fee; if not, try one reselect with updated fee to avoid underflow. + if totalInputValue < amount+actualFee { + reselected, _, err := w.SelectUtxos(ctx, amount+actualFee, true) + if err != nil { + return "", fmt.Errorf("insufficient funds after fee recalculation: %w", err) + } + if len(reselected) > 0 { + selectedUtxos = reselected + inputs = inputs[:0] + nSequences = nSequences[:0] + totalInputValue = 0 + for _, utxo := range selectedUtxos { + hash, err := chainhash.NewHashFromStr(utxo.Txid) + if err != nil { + return "", fmt.Errorf("failed to parse txid: %w", err) + } + inputs = append(inputs, &wire.OutPoint{Hash: *hash, Index: utxo.Index}) + totalInputValue += utxo.Value + nSequences = append(nSequences, wire.MaxTxInSequenceNum) + } + actualFee = w.estimateWithdrawFee(feeRate, len(selectedUtxos), 2) + } + if totalInputValue < amount+actualFee { + return "", fmt.Errorf("insufficient funds (need %d, have %d)", amount+actualFee, totalInputValue) + } + } + changeAmount := totalInputValue - amount - actualFeeinternal/infrastructure/tx-builder/covenantless/builder.go (2)
954-959
: Fee loop math bug when change ≤ dustYou zero newChange before adding it to exceedingValue, so nothing is added.
- if newChange <= dustLimit { - newChange = 0 - exceedingValue += newChange + if newChange <= dustLimit { + exceedingValue += newChange + newChange = 0 } else {
433-437
: Dust check compares wrong variableMessage says “output amount is dust” but code compares inputAmount.
- if inputAmount < dustAmount { + if outputAmount < dustAmount { return nil, fmt.Errorf( "forfeit tx output amount is dust, %d < %d", inputAmount, dustAmount, ) }api-spec/openapi/swagger/ark/v1/service.swagger.json (2)
54-57
: Update wording: BIP-322 terminology is stale here.Replace references to “BIP-322 signature/message” with the new “intent proof (base64 PSBT) and message” phrasing to match the new API and server behavior. Provide minimal guidance that the PSBT is base64-encoded.
- "summary": "DeleteIntent removes a previously registered intent from the server. -The client should provide the BIP-322 signature and message including any of the vtxos used in -the registered intent to prove its ownership. -The server should delete the intent and return success.", + "summary": "DeleteIntent removes a previously registered intent. +The client must provide an intent proof (base64 PSBT) and message that includes at least one input +from the registered intent to prove ownership. The server verifies the PSBT and deletes the intent.",
131-139
: Update wording for RegisterIntent.Align summary with the new PSBT-based intent proof.
- "summary": "RegisterIntent allows to register a new intent that will be eventually selected by the server -for a particular batch. -The client should provide a BIP-322 message with the intent information, and the server should -respond with an intent id.", + "summary": "RegisterIntent registers a new intent for selection by the server. +The client provides an intent proof (base64 PSBT) plus an encoded message; the server verifies and +returns an intent id.",internal/core/application/service.go (4)
1066-1071
: Wrong formatter for string; use %s not %x.%x on a Go string hex-encodes its bytes, not the intended hex string value.
- return "", fmt.Errorf("invalid cosigner pubkeys: %x is used by us", pubkey) + return "", fmt.Errorf("invalid cosigner pubkeys: %s is used by us", pubkey)
1040-1055
: Guard against short PkScript when extracting x-only pubkey.Ensure PkScript is long enough for both P2TR and sub-dust OP_RETURN encoding before slicing.
- hasOffChainReceiver = true - rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) + hasOffChainReceiver = true + if len(output.PkScript) < 34 { + return "", fmt.Errorf("invalid offchain receiver script (too short)") + } + rcv.PubKey = hex.EncodeToString(output.PkScript[2:])
1337-1339
: Fix stale error message (BIP322).Modernize the wording.
- return fmt.Errorf("no matching intents found for BIP322 proof") + return fmt.Errorf("no matching intents found for intent proof")
333-336
: Decode base64 PSBT input before parsing
psbt.NewFromRawBytes
expects raw PSBT bytes, but we’re passing base64-encoded strings (fromB64Encode
) directly. Introduce a helper, e.g.func parsePSBTB64(s string) (*psbt.Packet, error) { r := base64.NewDecoder(base64.StdEncoding, strings.NewReader(s)) return psbt.NewFromRawBytes(r, true) }and replace all
psbt.NewFromRawBytes(strings.NewReader(...))
call sites that receive B64 strings.
🧹 Nitpick comments (25)
pkg/arkd-wallet/core/application/wallet/service.go (3)
551-590
: Finalize tapscript using the leaf matching the provided LeafHash (not always index 0).When multiple TaprootLeafScript entries exist, picking [0] may mismatch the signed leaf/control block.
- if isTaproot && len(in.TaprootLeafScript) > 0 { - closure, err := script.DecodeClosure(in.TaprootLeafScript[0].Script) + if isTaproot && len(in.TaprootLeafScript) > 0 { + // Prefer the leaf matching TaprootScriptSpendSig.LeafHash (if present). + leaf := in.TaprootLeafScript[0] + if len(in.TaprootScriptSpendSig) > 0 { + want := in.TaprootScriptSpendSig[0].LeafHash + for _, l := range in.TaprootLeafScript { + th := txscript.NewBaseTapLeaf(l.Script).TapHash() + if bytes.Equal(th[:], want) { + leaf = l + break + } + } + } + closure, err := script.DecodeClosure(leaf.Script) if err != nil { return "", err } @@ - witness, err := closure.Witness(in.TaprootLeafScript[0].ControlBlock, args) + witness, err := closure.Witness(leaf.ControlBlock, args)
810-814
: Fee estimate: assume P2TR outputs (or actual dest script) to avoid underestimation.Using P2WPKH for outputs underestimates weight when the real outputs are P2TR.
- for i := 0; i < numOutputs; i++ { - dummyAddr, _ := btcutil.NewAddressWitnessPubKeyHash(make([]byte, 20), w.chainParams()) - dummyScript, _ := txscript.PayToAddrScript(dummyAddr) - weightEstimator.AddOutput(dummyScript) - } + for i := 0; i < numOutputs; i++ { + // Conservative: assume P2TR outputs by default. + dummyTap, _ := btcutil.NewAddressTaproot(make([]byte, 32), w.chainParams()) + dummyScript, _ := txscript.PayToAddrScript(dummyTap) + weightEstimator.AddOutput(dummyScript) + }
233-259
: Tiny readability nit: avoid shadowing imported package name.Local var named
script
shadows the importedscript
package elsewhere in this file; rename topkScript
for clarity.- script, err := txscript.ParsePkScript(in.WitnessUtxo.PkScript) + pkScript, err := txscript.ParsePkScript(in.WitnessUtxo.PkScript) if err != nil { return 0, err } - switch script.Class() { + switch pkScript.Class() { case txscript.PubKeyHashTy: weightEstimator.AddP2PKHInput() case txscript.WitnessV0PubKeyHashTy: weightEstimator.AddP2WKHInput() case txscript.WitnessV1TaprootTy: @@ - default: - return 0, fmt.Errorf("unsupported script type: %v", script.Class()) + default: + return 0, fmt.Errorf("unsupported script type: %v", pkScript.Class())api-spec/protobuf/ark/v1/types.proto (1)
51-53
: Use bytes for PSBT proof payload to avoid double-encoding.Since the proof will carry a signed PSBT, prefer
bytes
overstring
to store raw binary and let clients choose hex/base64 encodings at boundaries.Apply:
-message IntentProof { - string proof = 1; - string message = 2; -} +message IntentProof { + bytes proof = 1; // raw PSBT bytes + string message = 2; +}internal/interface/grpc/handlers/arkservice.go (1)
74-85
: Guard against nil/empty intent before parsing.Add an early check for
req.GetIntent()
(and optionally empty fields) to return InvalidArgument before calling the parser.func (h *handler) RegisterIntent( ctx context.Context, req *arkv1.RegisterIntentRequest, ) (*arkv1.RegisterIntentResponse, error) { - proof, message, err := parseRegisterIntent(req.GetIntent()) + if req.GetIntent() == nil { + return nil, status.Error(codes.InvalidArgument, "missing intent") + } + proof, message, err := parseRegisterIntent(req.GetIntent()) if err != nil { return nil, status.Error(codes.InvalidArgument, err.Error()) } intentId, err := h.svc.RegisterIntent(ctx, *proof, *message)Also ensure
parseRegisterIntent
never returns nil pointers on success.internal/infrastructure/tx-builder/covenantless/builder.go (1)
1278-1284
: Redundant re-decode and wrong net param (nil)You already have forfeitAddr; re-decoding with nil network can fail. Use it directly.
- addr, err := btcutil.DecodeAddress(forfeitAddr.String(), nil) - if err != nil { - return nil, err - } - - return txscript.PayToAddrScript(addr) + return txscript.PayToAddrScript(forfeitAddr)api-spec/openapi/swagger/ark/v1/admin.swagger.json (1)
465-475
: Add field descriptions/examples for v1IntentProofConsider documenting encoding expectations (base64 PSBT) and example payloads for proof/message to aid integrators.
api-spec/protobuf/ark/v1/service.proto (2)
16-25
: Update comments to remove BIP‑322 wordingThese lines still mention “BIP‑322 message”. Please align terminology with IntentProof (PSBT-based intent proof).
-// The client should provide a BIP-322 message with the intent information, and the server should -// respond with an intent id. +// The client should provide an IntentProof (PSBT-based) containing the intent information. +// The server responds with an intent id.
27-36
: Same here: DeleteIntent comment still mentions BIP‑322Refresh to reference the IntentProof structure.
-// The client should provide the BIP-322 signature and message including any of the vtxos used in -// the registered intent to prove its ownership. +// The client should provide an IntentProof including any of the inputs of the intent +// to prove ownership of that intent.internal/interface/grpc/handlers/parser.go (5)
20-33
: Be explicit in error text and accept only expected encodingsMinor: “missing intent” -> “missing intent proof” improves clarity. Also, if only base64 PSBT is supported, say so in the error; if hex may appear, consider a fallback parse.
- if i == nil { - return nil, fmt.Errorf("missing intent") + if i == nil { + return nil, fmt.Errorf("missing intent proof") } @@ - return nil, fmt.Errorf("missing intent proof") + return nil, fmt.Errorf("missing intent proof (expected base64-encoded PSBT)")
35-51
: Validate register message beyond JSON decodeAfter Decode, consider a lightweight Validate() (e.g., non-empty inputs/receivers, sane timestamps) to fail fast at the edge.
I can add a Validate() on intent.RegisterMessage and wire it here if desired.
53-69
: Same validation advice for delete intent messageAdd basic checks (e.g., presence of at least one referenced input, non-expired ExpireAt if used).
130-138
: Handle errors in toP2TR to avoid nil deref on malformed keysIgnoring parse errors risks panics if a 33-byte compressed key slips through. Fail closed or return empty script on error.
func toP2TR(pubkey string) string { - // nolint - buf, _ := hex.DecodeString(pubkey) - // nolint - key, _ := schnorr.ParsePubKey(buf) - // nolint - outScript, _ := script.P2TRScript(key) - return hex.EncodeToString(outScript) + buf, err := hex.DecodeString(pubkey) + if err != nil || len(buf) != 32 { + return "" + } + key, err := schnorr.ParsePubKey(buf) + if err != nil { + return "" + } + outScript, err := script.P2TRScript(key) + if err != nil { + return "" + } + return hex.EncodeToString(outScript) }
233-243
: Proof omitted in IntentInfo response — confirm intentIntentInfo in the API now has a proof field, but toProto doesn’t populate it. If intentional (to keep list lean), fine; otherwise, wire it through from application layer.
pkg/ark-lib/script/finalizer.go (1)
41-46
: Consider validating signature fields before processingThe loop processes
TaprootScriptSpendSig
entries without validating thatXOnlyPubKey
andSignature
fields are properly populated.Consider adding validation:
for _, sig := range in.TaprootScriptSpendSig { + if len(sig.XOnlyPubKey) != 32 { + return fmt.Errorf("invalid XOnlyPubKey length: expected 32, got %d", len(sig.XOnlyPubKey)) + } + if len(sig.Signature) != 64 { + return fmt.Errorf("invalid signature length: expected 64, got %d", len(sig.Signature)) + } args[hex.EncodeToString(sig.XOnlyPubKey)] = EncodeTaprootSignature(api-spec/openapi/swagger/ark/v1/service.swagger.json (2)
477-481
: Good ref: field now points to v1IntentProof. Consider example and clearer description.Add examples and clarify proof/message encoding to reduce client ambiguity.
"proof": { "$ref": "#/definitions/v1IntentProof", - "description": "an intent proof that includes any of the inputs of the intent to be deleted to prove the -ownership of that intent." + "description": "Intent proof including at least one input from the intent to prove ownership." }
601-611
: Add formats/descriptions to v1IntentProof fields.Declare base64 for the PSBT and UTF‑8 JSON for the message to improve client interop.
"v1IntentProof": { "type": "object", "properties": { "proof": { - "type": "string" + "type": "string", + "format": "byte", + "description": "Base64-encoded PSBT containing the signed intent proof" }, "message": { - "type": "string" + "type": "string", + "description": "UTF-8 JSON-encoded intent message (RegisterMessage/DeleteMessage)" } } },pkg/ark-lib/intent/proof.go (2)
292-299
: Copying Unknowns from input #1 to #0 is insufficient to verify input #0.Without TaprootLeafScript (and a sig bound to input index 0), input #0 can’t be finalized via script spend. If you keep executing input #0, also copy the TaprootLeafScript and rely on the signer to produce a distinct sig for input #0, or adopt the loop-skip above.
17-28
: Unused errors (ErrIncompletePSBT, ErrMissingArkFields).Remove or wire them in to reduce noise.
pkg/ark-lib/intent/message.go (3)
17-21
: Domain-separation tag: make the literal stable and documented.Consider defining a string const (e.g., const tag = "ark-intent-proof-message") and derive the []byte from it to avoid accidental drift and ease cross-language reuse. Also add a brief comment stating this tag is consensus-critical.
-var tagHashMessage = []byte("ark-intent-proof-message") +const intentTagString = "ark-intent-proof-message" // consensus-critical domain tag +var tagHashMessage = []byte(intentTagString)
23-38
: Validate fields on Decode (indexes, timestamps, cosigners).Decode currently only checks Type. Add lightweight validation:
- OnchainOutputIndexes: non-negative, unique, in-range of proof outputs.
- ValidAt/ExpireAt: ExpireAt == 0 or ExpireAt > ValidAt (if ValidAt != 0).
- CosignersPublicKeys: if any offchain outputs exist, require at least one key.
48-58
: Type guard is good; consider defaulting Type when empty.If a client omits Type (zero-value), set it to IntentMessageTypeRegister before returning an error, or return a clearer error indicating the required value.
test/e2e/e2e_test.go (3)
1889-1896
: Constructors over literals improve safety.Optional: provide an intent.NewRegisterMessage(...) helper that sets BaseMessage.Type and sensible defaults to avoid mistakes across tests.
1960-1964
: Prefer PSBT proprietary keys over ad-hoc Unknown keys.Using a raw ASCII key makes the key type 0x74 ('t'). Prefer the proprietary key type (0xFC) with an "ark" prefix to avoid collisions and follow BIP-174 conventions.
- intentProof.Inputs[realIdx].Unknowns = []*psbt.Unknown{{ - Value: tapTree, - Key: txutils.VTXO_TAPROOT_TREE_KEY, - }} + // Proprietary key: prefix "ark", subtype 0x01, keydata = VTXO_TAPROOT_TREE_KEY + prop := (&psbt.ProprietaryKey{ + Prefix: []byte("ark"), + SubType: 0x01, + KeyData: txutils.VTXO_TAPROOT_TREE_KEY, + }).Serialize() + intentProof.Inputs[realIdx].Unknowns = []*psbt.Unknown{{ + Key: append([]byte{psbt.ProprietaryType}, prop...), + Value: tapTree, + }}If ProprietaryKey isn’t available in your psbt version, I can adapt this to the equivalent raw serialization.
1965-1966
: Remove redundant decode/re-encode; use signedIntentProof directly.signedIntentProof is already base64 PSBT. Re-parsing only to re-encode is unnecessary.
- unsignedIntentProof, err := intentProof.B64Encode() + unsignedIntentProof, err := intentProof.B64Encode() require.NoError(t, err) @@ - signedIntentProofPsbt, err := psbt.NewFromRawBytes(strings.NewReader(signedIntentProof), true) - require.NoError(t, err) - require.NotNil(t, signedIntentProofPsbt) - - encodedIntentProof, err := signedIntentProofPsbt.B64Encode() - require.NoError(t, err) + encodedIntentProof := signedIntentProofAlso applies to: 1971-1975
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api-spec/protobuf/gen/ark/v1/admin.pb.go
is excluded by!**/*.pb.go
,!**/gen/**
api-spec/protobuf/gen/ark/v1/service.pb.go
is excluded by!**/*.pb.go
,!**/gen/**
api-spec/protobuf/gen/ark/v1/types.pb.go
is excluded by!**/*.pb.go
,!**/gen/**
go.sum
is excluded by!**/*.sum
pkg/ark-cli/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (22)
api-spec/openapi/swagger/ark/v1/admin.swagger.json
(2 hunks)api-spec/openapi/swagger/ark/v1/service.swagger.json
(3 hunks)api-spec/protobuf/ark/v1/admin.proto
(1 hunks)api-spec/protobuf/ark/v1/service.proto
(1 hunks)api-spec/protobuf/ark/v1/types.proto
(1 hunks)go.mod
(1 hunks)internal/core/application/admin.go
(1 hunks)internal/core/application/service.go
(14 hunks)internal/core/application/types.go
(2 hunks)internal/infrastructure/tx-builder/covenantless/builder.go
(1 hunks)internal/interface/grpc/handlers/arkservice.go
(1 hunks)internal/interface/grpc/handlers/parser.go
(1 hunks)pkg/ark-cli/go.mod
(1 hunks)pkg/ark-lib/bip322/proof.go
(0 hunks)pkg/ark-lib/bip322/signature.go
(0 hunks)pkg/ark-lib/bip322/signature_test.go
(0 hunks)pkg/ark-lib/intent/message.go
(5 hunks)pkg/ark-lib/intent/proof.go
(1 hunks)pkg/ark-lib/note/note.go
(2 hunks)pkg/ark-lib/script/finalizer.go
(1 hunks)pkg/arkd-wallet/core/application/wallet/service.go
(1 hunks)test/e2e/e2e_test.go
(5 hunks)
💤 Files with no reviewable changes (3)
- pkg/ark-lib/bip322/signature_test.go
- pkg/ark-lib/bip322/proof.go
- pkg/ark-lib/bip322/signature.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/infrastructure/tx-builder/covenantless/builder.go
internal/core/application/service.go
test/e2e/e2e_test.go
🧬 Code graph analysis (9)
internal/core/application/admin.go (2)
api-spec/protobuf/gen/ark/v1/types.pb.go (6)
Vtxo
(133-151)Vtxo
(166-166)Vtxo
(181-183)Outpoint
(23-30)Outpoint
(45-45)Outpoint
(60-62)internal/core/application/types.go (2)
Outpoint
(145-145)VOut
(99-99)
internal/infrastructure/tx-builder/covenantless/builder.go (1)
pkg/ark-lib/script/finalizer.go (1)
FinalizeVtxoScript
(13-61)
pkg/ark-lib/script/finalizer.go (3)
pkg/ark-lib/script/closure.go (1)
DecodeClosure
(31-69)pkg/ark-lib/txutils/psbt.go (2)
GetConditionWitness
(67-75)CONDITION_WITNESS_KEY_PREFIX
(17-17)pkg/ark-lib/script/script.go (1)
EncodeTaprootSignature
(132-138)
pkg/ark-lib/intent/proof.go (3)
pkg/ark-lib/note/note.go (1)
NoteClosure
(158-160)pkg/ark-lib/txutils/psbt.go (1)
GetConditionWitness
(67-75)pkg/ark-lib/script/finalizer.go (1)
FinalizeVtxoScript
(13-61)
pkg/ark-lib/note/note.go (3)
pkg/ark-lib/txutils/taptree.go (1)
TapTree
(14-14)pkg/ark-lib/script/script.go (1)
P2TRScript
(112-117)pkg/ark-lib/txutils/psbt.go (2)
CONDITION_WITNESS_KEY_PREFIX
(17-17)VTXO_TAPROOT_TREE_KEY
(19-19)
internal/core/application/service.go (3)
pkg/ark-lib/intent/proof.go (2)
Proof
(41-43)Verify
(53-128)pkg/ark-lib/intent/message.go (2)
RegisterMessage
(23-38)DeleteMessage
(60-65)pkg/ark-lib/txutils/psbt.go (1)
GetTaprootTree
(38-50)
internal/interface/grpc/handlers/parser.go (3)
api-spec/protobuf/gen/ark/v1/types.pb.go (3)
IntentProof
(458-465)IntentProof
(480-480)IntentProof
(495-497)pkg/ark-lib/intent/proof.go (1)
Proof
(41-43)pkg/ark-lib/intent/message.go (2)
RegisterMessage
(23-38)DeleteMessage
(60-65)
internal/core/application/types.go (2)
pkg/ark-lib/intent/proof.go (1)
Proof
(41-43)pkg/ark-lib/intent/message.go (2)
RegisterMessage
(23-38)DeleteMessage
(60-65)
test/e2e/e2e_test.go (4)
pkg/ark-lib/intent/message.go (3)
RegisterMessage
(23-38)BaseMessage
(19-21)IntentMessageTypeRegister
(13-13)pkg/ark-lib/intent/proof.go (2)
New
(136-160)Input
(46-50)pkg/ark-lib/txutils/taptree.go (1)
TapTree
(14-14)pkg/ark-lib/txutils/psbt.go (1)
VTXO_TAPROOT_TREE_KEY
(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (17)
pkg/arkd-wallet/core/application/wallet/service.go (2)
512-519
: Correct: PSBT tapscript sigs must be 64 bytes (no sighash byte). Add a small length guard.Stripping the trailing sighash byte is right for PSBT_IN_TAPROOT_SCRIPT_SIG. Consider guarding against unexpected lengths to avoid slicing panics.
leafHash := tapLeaf.TapHash() - ptx.Inputs[inputIndex].TaprootScriptSpendSig = append(ptx.Inputs[inputIndex].TaprootScriptSpendSig, &psbt.TaprootScriptSpendSig{ - Signature: signature[:64], // remove the last byte (sig hash type) because signature is already encoded + if len(signature) < 64 { + return "", fmt.Errorf("invalid tapscript signature length: %d", len(signature)) + } + ptx.Inputs[inputIndex].TaprootScriptSpendSig = append(ptx.Inputs[inputIndex].TaprootScriptSpendSig, &psbt.TaprootScriptSpendSig{ + Signature: signature[:64], // strip appended sighash byte; SigHash is stored separately XOnlyPubKey: schnorr.SerializePubKey(signingKey.PubKey()), LeafHash: leafHash[:], SigHash: input.SighashType, })
496-505
: Leaf-path signing always uses SignerKey; confirm intent vs. signMode.The prior (commented) code suggested LP should sign leaf-path with forfeitPrvkey. Current logic unconditionally uses SignerKey and silently skips when it’s nil. Is that intentional for the new intent/PSBT flow?
If not, consider restoring signMode-based key selection:
- if len(input.TaprootLeafScript) > 0 { - if w.SignerKey == nil { - continue - } - - signingKey := w.SignerKey + if len(input.TaprootLeafScript) > 0 { + var signingKey *btcec.PrivateKey + switch signMode { + case application.SignModeSigner: + if w.SignerKey == nil { + return "", ErrSignerDisabled + } + signingKey = w.SignerKey + case application.SignModeLiquidityProvider: + if w.keyMgr == nil { + return "", ErrWalletLocked + } + signingKey = w.keyMgr.forfeitPrvkey + default: + continue + }internal/core/application/admin.go (1)
200-214
: The verification is in progress; fetching the implementation details ofIntentProofInput
and the definition ofWitnessUtxo
.internal/infrastructure/tx-builder/covenantless/builder.go (1)
214-214
: Good consolidation to script.FinalizeVtxoScriptThis simplifies Taproot vtxo witness finalization and centralizes correctness.
Please add/extend a test that ensures FinalizeAndExtract correctly finalizes inputs with CONDITION_WITNESS present (covers closure-conditional paths).
api-spec/protobuf/ark/v1/admin.proto (1)
146-147
: Type swap to IntentProof looks good; confirm wire-compatAssuming IntentProof keeps two string fields at tags 1 and 2 like the old type, this remains wire-compatible. Please confirm generated code and any mixed-version clients remain interoperable.
api-spec/openapi/swagger/ark/v1/admin.swagger.json (1)
445-446
: Reference update aligns with protov1IntentInfo.proof -> v1IntentProof is consistent with protobuf changes.
api-spec/protobuf/ark/v1/service.proto (1)
146-149
: Request/response schema updates look correctRegisterIntentRequest.intent and DeleteIntentRequest.proof now use IntentProof.
Also applies to: 154-158
internal/core/application/types.go (1)
8-8
: LGTM! Clean migration to intent-based typesThe interface changes from
bip322
tointent
types are consistent and well-structured. The method signatures properly align with the new intent-based flow introduced across the codebase.Also applies to: 15-17, 35-39
pkg/ark-lib/note/note.go (1)
110-154
: Well-structured migration to PSBT-based intent proofsThe refactored
IntentProofInput()
method properly constructs PSBT inputs with witness data and taproot tree information embedded in the Unknowns field. Error handling is comprehensive and the return structure aligns well with the new intent-based flow.pkg/ark-lib/intent/message.go (5)
40-46
: LGTM: Encode is straightforward.
60-65
: LGTM: DeleteMessage DTO is clear.
67-73
: LGTM: Encode.
75-85
: LGTM: Decode + type check.
87-90
: hashMessage helper is fine; keep tag stable.Returning tagged[:] is correct; ensure all verifiers use the same tag to avoid mismatches.
Would you like a small cross-test asserting the hash for a fixed sample message so changes to the tag are caught?test/e2e/e2e_test.go (3)
22-22
: LGTM: intent import switch.
1923-1944
: LGTM: Proof.New inputs/outputs wiring confirmedbuildToSignTx adds the anchor/toSpend input at index 0 and each provided UTXO sequentially starting at index 1.
1952-1954
: Guard against input index assumptions; set data on the real input only.Accessing Inputs[1] assumes a specific layout. Use the last PSBT input as the real UTXO and attach scripts/unknowns there. This avoids out-of-range panics if layout changes.
- intentProof.Inputs[0].TaprootLeafScript = []*psbt.TaprootTapLeafScript{tapLeafScript} - intentProof.Inputs[1].TaprootLeafScript = []*psbt.TaprootTapLeafScript{tapLeafScript} + realIdx := len(intentProof.Inputs) - 1 + intentProof.Inputs[realIdx].TaprootLeafScript = []*psbt.TaprootTapLeafScript{tapLeafScript} @@ - intentProof.Inputs[1].Unknowns = []*psbt.Unknown{{ - Value: tapTree, - Key: txutils.VTXO_TAPROOT_TREE_KEY, - }} + intentProof.Inputs[realIdx].Unknowns = []*psbt.Unknown{{ + Value: tapTree, + Key: txutils.VTXO_TAPROOT_TREE_KEY, // consider proprietary PSBT key, see next comment + }}Also applies to: 1960-1964
⛔ Skipped due to learnings
Learnt from: louisinger PR: arkade-os/arkd#691 File: internal/core/application/service.go:557-562 Timestamp: 2025-08-19T10:58:41.042Z Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api-spec/protobuf/gen/ark/v1/service.pb.go
is excluded by!**/*.pb.go
,!**/gen/**
📒 Files selected for processing (4)
api-spec/openapi/swagger/ark/v1/service.swagger.json
(3 hunks)api-spec/protobuf/ark/v1/service.proto
(1 hunks)internal/core/application/service.go
(15 hunks)pkg/ark-lib/intent/proof.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api-spec/protobuf/ark/v1/service.proto
- api-spec/openapi/swagger/ark/v1/service.swagger.json
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-19T10:58:41.042Z
Learnt from: louisinger
PR: arkade-os/arkd#691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.
Applied to files:
internal/core/application/service.go
pkg/ark-lib/intent/proof.go
📚 Learning: 2025-09-29T14:33:52.834Z
Learnt from: louisinger
PR: arkade-os/arkd#722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.834Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern. A common bug is manually base64-decoding the string and then passing b64=true, which causes a double-decode error.
Applied to files:
pkg/ark-lib/intent/proof.go
📚 Learning: 2025-09-29T14:33:52.834Z
Learnt from: louisinger
PR: arkade-os/arkd#722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.834Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern.
Applied to files:
pkg/ark-lib/intent/proof.go
🧬 Code graph analysis (2)
internal/core/application/service.go (3)
pkg/ark-lib/intent/proof.go (2)
Proof
(41-43)Verify
(53-128)pkg/ark-lib/intent/message.go (2)
RegisterMessage
(23-38)DeleteMessage
(60-65)pkg/ark-lib/txutils/psbt.go (1)
GetTaprootTree
(38-50)
pkg/ark-lib/intent/proof.go (3)
pkg/ark-lib/note/note.go (1)
NoteClosure
(158-160)pkg/ark-lib/txutils/psbt.go (1)
GetConditionWitness
(67-75)pkg/ark-lib/script/finalizer.go (1)
FinalizeVtxoScript
(13-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit tests
- GitHub Check: integration tests
- GitHub Check: Build and Scan
🔇 Additional comments (13)
pkg/ark-lib/intent/proof.go (7)
1-28
: LGTM!The import statements and error variable definitions are well-structured and comprehensive. The error messages are descriptive and cover the expected validation failure scenarios.
30-50
: LGTM!The constants and struct definitions are well-designed for the BIP322-style intent proof system. The fake outpoint constants correctly implement the BIP322 specification, and the struct definitions are clean and appropriate.
130-159
: LGTM!The proof construction logic is well-implemented with proper input validation. The function correctly builds the BIP322-style transaction structure and returns a properly initialized Proof struct.
162-181
: LGTM!Both methods correctly handle the BIP322 transaction structure.
GetOutpoints
properly skips the first input (toSpend transaction), andContainsOutputs
correctly identifies when the proof includes actual outputs beyond the empty OP_RETURN placeholder.
183-282
: LGTM!The helper functions correctly implement the BIP322 transaction building pattern. The
buildToSpendTx
function properly creates the virtual transaction with the message hash, andbuildToSignTx
creates the appropriate PSBT structure. TheintentProofPrevoutFetcher
wrapper correctly handles the special case of the virtual toSpend transaction.
284-303
: LGTM!The finalization logic correctly handles the PSBT extraction process. The copying of unknowns from the second input to the first input enables proper condition witness handling for the BIP322 structure.
305-351
: LGTM!The input finalization logic properly handles both note closures and regular vtxo scripts. The note-specific handling correctly validates the condition witness structure and extracts the preimage, while falling back appropriately to the existing vtxo script finalization for regular inputs.
internal/core/application/service.go (6)
18-18
: LGTM!The addition of the intent package import and the updated
RegisterIntent
method signature correctly align with the PR's objective to replace BIP322-based proofs with the new intent proof system.Also applies to: 814-816
839-969
: LGTM!The updated input validation logic properly handles the new intent proof structure. The indexing correctly accounts for the toSpend input at index 0, and the direct validation of witness UTXOs from PSBT inputs provides better consistency checking than the previous approach. The validation covers both boarding inputs and existing vtxo inputs appropriately.
1000-1077
: LGTM!The output handling correctly accesses transaction outputs via
proof.UnsignedTx.TxOut
. The logic for distinguishing between onchain and offchain outputs and their respective validation remains appropriate and comprehensive.
1201-1203
: LGTM!The updated
DeleteIntentsByProof
method signature correctly uses the new intent proof types, aligning with the overall PR objectives.
986-993
: Remove sensitive data from log statements.These log statements expose sensitive proof data that can leak preimages, scripts, and partial signatures. Only log the error message without the proof/message content.
- if err := intent.Verify(signedProof, encodedMessage); err != nil { - log. - WithField("unsignedProof", encodedProof). - WithField("signedProof", signedProof). - WithField("encodedMessage", encodedMessage). - Tracef("failed to verify intent proof: %s", err) + if err := intent.Verify(signedProof, encodedMessage); err != nil { + log.Tracef("failed to verify intent proof: %s", err) return "", fmt.Errorf("invalid intent proof: %s", err) }
1310-1317
: Remove sensitive data from log statements in Delete flow too.This has the same sensitive data logging issue as in the RegisterIntent method.
- if err := intent.Verify(signedProof, encodedMessage); err != nil { - log. - WithField("unsignedProof", encodedProof). - WithField("signedProof", signedProof). - WithField("encodedMessage", encodedMessage). - Tracef("failed to verify intent proof: %s", err) + if err := intent.Verify(signedProof, encodedMessage); err != nil { + log.Tracef("failed to verify intent proof: %s", err) return fmt.Errorf("invalid intent proof: %s", err) }
This PR reworks the BIP322 intent proof used to register coins in batches.
BIP322 PSBT
We keep BIP322 transaction format, but instead of finalizing client-side, we directly send the signed PSBT to
RegisterIntent
. The server is responsible to finalize and execute the input witnesses in order to verify the proof.PSBT ark "unknown" fields
Having a PSBT instead of a raw tx allows specifying ark fields associated with spent inputs. Thus, the intent PSBT can reveal extra witness conditions (like preimage) and taproot vtxo tree inside the input's unknown PSBT field. It makes the intent transaction closer to offchain transaction spec. Because now we can reveal taptrees in inputs, we remove them from the proof's message.
Operator counter-sign intent
RegisterIntent
also try to counter-sign the intent tx before finalizing it, letting to later implement collab path registration (#613)@altafan @Kukks please review
Summary by CodeRabbit
New Features
Refactor
Tests
Chores