Repository navigation
fix(template): push a pubkey into a covenant as its x-only key - #3
Conversation
leafLookup already turns a pubkey constructor value into the 32-byte x-only key a taproot script uses, but covenantLookup passed every field through raw, so the same key reached a covenant as 33 compressed bytes. A covenant that compares a key with a taproot witness program (`scriptPubKey == key`) can then never succeed, and the derived address disagrees with the one the ts-sdk and arkadec's own e2e derive from the same artifact and params: both bind a pubkey with schnorr.SerializePubKey. Both lookups now share one conversion. DELEGATE_KEY keeps its hex spelling, since intent messages carry cosigner keys that way. No bundled artifact puts a pubkey in a covenant, so no existing template's address moves.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughContract covenant and leaf lookups now use a shared helper that converts ChangesPubkey normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: pushing public-key constructor values into covenants as 32-byte x-only keys. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review
Fix is correct. No changes required.
What I verified
keyField helper (contract.go:34–44)
Logic is sound. The if err != nil || v.typ != "pubkey" guard passes non-pubkey values and errors through unchanged; only pubkey-typed values are converted. btcec.ParsePubKey on a well-formed 33-byte compressed key (the only thing canonicalBytes allows through) will always succeed, so no silent truncation or panic surface is added.
covenantLookup change (contract.go:56)
Swapping field(name) → keyField(name) is the minimal correct fix. The DELEGATE_KEY branch is correctly left alone: intent messages carry cosigner keys as hex-encoded compressed bytes by protocol design, and that convention is unchanged.
leafLookup simplification (contract.go:109)
Removing the explicit v.typ == "pubkey" block and delegating to keyField produces identical behaviour. The TWEAK branch (contract.go:98–105) correctly keeps calling field directly — it needs the 33-byte compressed form to call btcec.ParsePubKey for the twist, so it must not go through keyField.
Address stability
I checked every bundled template (delegated_vtxo.json, boarding.json, renewal.json, vhtlc_claim.json, vhtlc_refund.json) and all pkg/template/testdata/ artifacts: none references a pubkey constructor field inside a covenant asm section. No deployed address changes.
Test (contract_test.go:67–79)
TestBuildCovenantPushesXOnlyPubkey directly reproduces the failure (21-byte push before, 20-byte after) and pins the correct output. Coverage is adequate for this targeted regression.
CI note
All CI jobs failed with "account payments have failed / spending limit" — a billing infrastructure issue unrelated to the code. The PR author reports go test ./pkg/template/, go vet, and go build ./... pass locally, which is consistent with the change.
Nits (non-blocking)
keyFieldis a small closure that only does one thing; no objection to keeping it inline as written. If this file grows further, a named package-level helper could be worth extracting, but that is out of scope here.- The test covenant (
<owner> OP_DROP OP_1) is intentionally minimal to isolate encoding behaviour — appropriate for this case.
LGTM.
…smatch The registration lands after receiveQuotes.bind --- which is the commit --- and before fillToResponse offers the lockup. It is a network call and has no business inside the transaction, and the ordering means the advance row exists when a mismatch throws, so the failure is diagnosable rather than invisible. The cost is the reverse: a mismatch strands one advance and its input reservations until the quote expires. Registering before bind would avoid that and instead leave a watch with no advance behind a race; watches are idempotent on their fingerprint and expire, so either is recoverable, and this ordering was the one asked for. It is gated on covenantVersion === 2 and on a configured delegatee, so a v1 fill never calls it and an unset TAXI_DELEGATEE_URL leaves v2 covenants unrenewed rather than refusing to quote. cli.ts registers the artifact and the template once at startup, before any quote can run; both calls are idempotent on their derived id. A mismatch means the two sides disagree about the taptree, and under D2 that covenant would never be renewed, so registerRenewal throws. That is not hypothetical: against delegatee master f12e97e the derivations differ, because contract.go:42-51 pushes a `pubkey` constructor argument into the arkade covenant raw, 33 compressed bytes, where contract.go:101-107 x-onlys the same argument for a tapscript leaf. arkade-os/executor#3 fixes it upstream; the bytes32 workaround was declined, so this gate is what stands between that bug and a covenant nobody renews.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass: 8 commits since 9931409, including feat: spend outpoint, parallel intent confirmation, pool-based output allocation, Go 1.26.9 + dependency bumps. 0 prior issues to close (previous review was LGTM with no open findings).
Prior findings
None were open. All closed trivially — no prior issues remain.
New findings
Low — poolOf nil dereference (unreachable, but latent panic)
pkg/template/template_outputs.go:174–182, called at pkg/template/outputs.go:51:
if pool := t.poolOf(slot); pool[0] == slot {poolOf returns nil when no output draws from slot. checkBalances (enforced at parse time) guarantees every input has exactly one remainder output, so this is unreachable through Parse(). But the call site dereferences without a nil guard. A defensive check costs nothing:
if pool := t.poolOf(slot); pool != nil && pool[0] == slot {Low — queuedKey uses only inputs[0]
internal/core/application/lanes.go:20–21:
func queuedKey(p *pendingIntent) string { return p.inputs[0].coin.Outpoint.String() }For the reconnection-recovery path in register(), two intents that share inputs[0] but differ on later inputs could map to the same queue key and the wrong queued entry would be reused. In the current design this can't happen: "duplicated input" from arkd means all of the intent's inputs are duplicated, so the queued entry is definitionally the right one. The assumption isn't documented. A comment on queuedKey to that effect would help the next reader.
Low — "duplicated input" string matching for arkd recovery
internal/core/application/renewal.go in register():
strings.Contains(err.Error(), "duplicated input")This couples the reconnection-recovery path to arkd's exact error text. If arkd changes that message, the service silently fails to recover the queued intent (it returns an error instead of reusing the session). Not security-critical, but a reliability regression on arkd updates. Worth extracting into a sentinel error on the arkd client interface, or at least a named constant string.
Design note — watch and spend racing on the same coin
internal/core/application/discovery.go:258: spends are excluded from boundOwners, so a watch that covers the same coin will still discover it and attempt renewal. TestWatchKeepsCoinsASpendBinds confirms this is intentional. The practical consequence: if both the spend and the watch run in the same cycle, the watch's renewal fails with a "coin already spent" error, and that failure is charged against the watch's template quota (only spend results are excluded from templateOutcomes, not watch failures on coins a spend consumed). No correctness bug — the watch retries, the coin is already gone, the delegation expires normally — but the first failure log will be confusing without spend context and may hit the template's refusal threshold on a tight quota. If this matters operationally, the watch's failure reason could check for "already spent" before counting it.
Things checked and found correct
spendMessageusesTaggedHash("delegatee/spend", fp_bytes || expires_unix_be_u64)— properly binds both the authorized scope and its lifetime;checkSpendSignatureverifies a Schnorr signature with that message against the x-only exit key. Sound.exitKeyOfEverySlot— requires a CSV single-pubkey closure per slot. Correctly rejects keys that are only in multi-key leaves or non-exit leaves.IsSpend()discriminator (Address == "" && ExpiresAt != nil) — consistent with how all three delegation kinds set those fields across the codebase; idempotent re-registration and the "outpoint bound by another spend" DB rejection are both tested.followBatchesgoroutine +refusedslice —refusedis written exclusively by the goroutine beforeclose(registered)and read only after<-registered. Happens-before is sound; no data race.- Parallel
ConfirmRegistrationinOnBatchStarted— errgroup, unconfirmed intents return topending, empty confirmation returnsskip=true. Correct; tested inTestBatchHandlerConfirmsInParallel. - Pool allocation in
routeSats— sums the whole pool, deducts fixed outputs, deducts fee (viaslices.Contains(pool, t.fee.from)), assigns remainder. Balanced;TestAllocatePooledpins the arithmetic. batch: lane{queued: map[string]*pendingIntent{}}inNewServiceWithKeys— nil map write was the pre-existing bug that the monitor-intent-queue commit fixed. Correct.expiresAtrange gate inRegisterSpend—After(now)andBefore(now+24h)— prevents stale and excessively long-lived spends; tested in "expired" and "beyond a day".- Dependency bumps —
x/net v0.60.0,x/crypto v0.57.0,x/sync v0.23.0and companion packages updated consistently across all threego.modfiles.
|
I checked the follow-up review against current master while refreshing this branch. The remaining PR diff is only the covenant/leaf x-only conversion and its regression test; the dependency updates and queue/spend changes discussed in the follow-up are now upstream. The suggested The queue identity/error-text and watch/spend quota notes remain reasonable upstream follow-ups. This refresh does not change these paths or claim to solve their design questions. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
5 commits since 56fb40b. Prior findings: 1 no longer applicable, 2 still open but out of scope for this diff, 1 partially addressed. No new blocking issues.
Prior findings — close-out
| # | Finding | Status |
|---|---|---|
| 1 | poolOf nil dereference (template_outputs.go) |
No longer applicable — @Kukks explicitly chose the checkBalances invariant over a defensive guard; template_outputs.go is unchanged in this delta. |
| 2 | queuedKey uses only inputs[0] without a documenting comment (lanes.go) |
Still open — lanes.go is not in this diff. Not worse. |
| 3 | "duplicated input" string-match coupling (renewal.go) |
Still open — renewal.go is not in this diff. Not worse. |
| 4 | Watch/spend quota design note (discovery.go) |
Partially addressed — the IsSpawned() sort in discovery.go now gives spawned watches priority over public watches on the same script, reducing the collision likelihood. The quota-charging concern (a failed watch costing a slot before a spend is visible) is not directly addressed, but the blast radius is smaller. |
Incremental findings
pkg/template/contract.go — new buildTree / fromTree / decodeTaptree / taptreeScript
Protocol logic is sound. Walk-through:
decodeTaptree(~line 175): 8 KB cap →txutils.DecodeTapTree→TapscriptsVtxoScript.Decode. Validates every leaf as an ark-acceptable closure before the delegatee accepts the tree. Zero-leaf guard is correct. The cap prevents a DoS-sized PSBT from stallingtrigger.Match.fromTree: Maps declared leaves to tree positions by hex-encoded script equality. Returns tree-ordered leaves (not declared-order), sobuildTreeassembles the same Merkle root astaptreeScriptcomputes —c.pkScriptwill match the matched output'sPkScriptwhen the trigger fires. Correct.- Unnamed-leaf guard (
buildTreeloop):names[i] == ([2]string{})skips proof generation for tree-only leaves. Correct and tested. taptreeScriptinsidetrigger.Match: Output is skipped whenerr != nilor the derived key doesn't matcho.PkScript. Soreactnever receives an output whose taptree failed validation.confineTaptrees: Prevents a taptree variable from appearing inside anasm/arguments/locking/ciphertextbinding. Correct — avoids the push-size cap and script injection.isTaptreeskipscanonicalBytesinInstantiate: Correct; the taptree byte slice is not pushed, so the per-element size limit does not apply. Validation is deferred todecodeTaptree.
Low / nit — tapLeaves silently swallows hex decode error (contract.go, tapLeaves function):
s, _ := hex.DecodeString(h)Every caller obtains h from decodeTaptree → txutils.DecodeTapTree, which hex-encodes the scripts, so the decode will always succeed in practice. Still, the silent _ is the only unchecked error in the file. A panic guard or a named error return would make the invariant explicit without changing behaviour for callers.
Low — duplicate script in the given taptree (fromTree):
for i, h := range scripts {
at[h] = i // last occurrence wins on duplicate
}If the taptree has two identical scripts, only the last index is recorded; the proof attached to that position is valid for either occurrence (both control blocks cover the same script). TestTaptreeContract covers this ("a duplicated leaf is still in the tree"), so behaviour is intentional and tested. Noting for future readers: the proof references the last-indexed position.
internal/core/application/stream.go (new file)
react is correct. Binding.Output comes from range tx.UnsignedTx.TxOut inside trigger.Match, so it is always a valid index into ptx.UnsignedTx.TxOut; no bounds issue.
setStreams / streamSet mutex discipline: streamSet returns a snapshot of the slice before releasing the lock; react iterates the snapshot, so concurrent setStreams calls do not corrupt the iteration. Correct.
readTransactions / watchStreams reconnect: error-suppression after the first warning, 1 s retry, streamsChanged channel with non-blocking send — all correct.
Low — undocumented invariant in triggerKeys (stream.go, triggerKeys function):
k := template.TriggerKeys{..., Delegate: s.cosigners[0].key.PubKey()}s.cosigners[0] is accessed without a len guard. Service initialisation enforces at least one cosigner, but the assumption is not expressed at this site. A comment cross-referencing that invariant (or a panic guard consistent with how similar assumptions are handled elsewhere) would make the site self-documenting.
internal/core/application/discovery.go
successor cap change: the old }, own) passed a bool as the max int parameter (false→0 = no cap, true→1 = cap at 1). New code:
limit := s.limits.MaxDelegations
if own { limit = 0 } // operator's own: no capSemantics are intentionally inverted relative to the old code: the operator's own successor discoveries bypass MaxDelegations (uncapped), while public ones are capped. Correct for the stream-spawned-watch use case and consistent with MaxStreamChildren in Limits.
IsSpawned() sort: spawned watches are sorted first, so a public watch on the same script cannot shadow a spawn in the same discovery cycle. Correct and intentional.
internal/core/application/spend.go
tmpl.Trigger() != nil → errStreamOnly guard is correctly placed before the slot-count check. RegisterSpend now uses s.limits.MaxDelegations instead of false (0); spend registrations now count against the cap. Correct fix.
Test coverage
TestTaptreeContract, TestTaptreeParse, TestNonInteractiveClaimAddress, TestNonInteractiveClaimNeedsTheClaimLeaf, TestNonInteractiveClaimCovenant directly exercise the new buildTree path, variable confinement, and failure modes. TestRegisterStreamDelegation, TestPublicRegistrationRefusesTriggers, TestScanHandsStreamsToTheWatcher cover the stream lifecycle. Coverage is adequate.
No blocking issues. The two low/nit items above are the only new findings.
|
Checked the nonblocking notes in the latest review against
No additional guards or upstream stream changes are included in this refresh. All seven current-head checks pass, including funded regtest/PostgreSQL coverage; this is separate from live Mutinynet and human protocol sign-off. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
1 commit since last pass ("cleaning" — extract closures to package-level functions); 0 prior findings fixed, 0 made worse, no new blocking issues.
Prior findings — close-out
| # | Finding | Status |
|---|---|---|
| 1 | poolOf nil dereference (template_outputs.go) |
No longer applicable — unchanged; @Kukks' checkBalances-invariant decision stands |
| 2 | queuedKey uses only inputs[0] without a documenting comment (lanes.go) |
Still open — not in this diff, not worse |
| 3 | "duplicated input" string-match coupling (renewal.go) |
Still open — not in this diff, not worse |
| 4 | Watch/spend quota design note (discovery.go) |
Still open — not in this diff, not worse |
| 5 | tapLeaves silent _ on hex decode (contract.go) |
Fixed — invariant comment (// DecodeTapTree hex-encodes) present in the file; @Kukks' response accepted |
| 6 | triggerKeys undocumented first-cosigner access (stream.go) |
Acknowledged — @Kukks documented the newCosigners invariant; not in this diff |
Incremental findings — pkg/template/contract.go @ 3141ef5
The "cleaning" commit does exactly one thing: it lifts the four closures that were defined inside buildTree (field, keyField, delegate, covenantLookup, tweak) into unexported package-level functions and passes the formerly-captured state as explicit parameters. All paths and all call sites are semantically identical to the prior head. No behaviour changed.
Nit — gofmt violations (two)
1. Leading space before covenantLookup declaration (contract.go, the func covenantLookup line):
func covenantLookup(keys Keys, args map[string]value, name string) (value, error) {There is a stray leading space before func. gofmt rejects this; make fmt would fix it.
2. Trailing space in struct literal (contract.go, c := &contract{…}):
covenants: map[string][]byte{}, Trailing whitespace on that line. Also caught by gofmt / golangci-lint.
Neither affects compilation or correctness. Running make fmt before merge is sufficient.
No protocol, security, or correctness issues in this diff. The two open upstream findings (items 2 and 3 above) are pre-existing and out of scope for this change.
Constructor public keys now use the same 32-byte x-only encoding in covenant and leaf lookups. A covenant previously received a 33-byte compressed key, so comparing it with a taproot witness program failed and its derived address disagreed with the SDK. Both lookups share the existing conversion.
DELEGATE_KEYandTWEAKretain their compressed-key conventions.TestBuildCovenantPushesXOnlyPubkeypins the original failure. No bundled template puts a pubkey constructor field in its covenant assembly, so their addresses remain stable.The branch includes current master (
78391fc) without rewriting its published history. The merge retains master's delegation stream, template/health API and gRPC 1.83.2 changes. The resulting PR diff contains only the x-only helper and its regression test; the earlier Go/HTTP/2 dependency updates are now on master.Validation
56fb40ba: all seven GitHub checks passed, including funded regtest and PostgreSQL integration.e3c3f66: local Linux Go 1.26.9 race/coverage suites pass: baseline 309 top-level tests /712 passing test events becomes 333 /740 after incoming master, with zero failures. Application build, root/template/API vet, standalone API tests and formatting of all 106 tracked Go files pass. Formatting used temporary Linux copies; Windows source line endings were preserved.e3c3f66, including the image security scan and funded regtest/PostgreSQL job. The integration log records 34 passing tests, zero failures and the existingTestLiveDelegateegate skip.Review
CodeRabbit reviewed the updated
e3c3f66contract and reported no actionable comments. The latest Arkana formal review also coverse3c3f66and reports no new blocking issues. Its low-priority taptree/cosigner invariants were checked against current source and answered on the PR. Earlier queue-recovery and watch/spend quota follow-ups concern unchanged upstream paths. This refresh preserves those contracts; human protocol approval and live Mutinynet validation remain separate gates.