WIP: Feat/vhtlc contract#157
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
bitcoin-coder-bob
changed the base branch from
master
to
feat/delegate-contract
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Aggressive Code Review — #157 "Feat/vhtlc contract"
Thorough review of the full diff (+5954/−273 across 36 files). This PR introduces a contract manager abstraction, a watcher, a delegate handler, and a VHTLC handler. It's a major architectural change that touches protocol-critical paths. Findings below, ordered by severity.
⚠️ PROTOCOL-CRITICAL — Requires human sign-off regardless of fixes
This PR modifies VTXO handling, spend path selection, forfeit paths, and transaction signing flows. Even if all findings below are resolved, a human must review and approve before merge per the protocol-critical rules.
🔴 P0 — Data races on a.contractManager (funds at risk)
client.go ~line 1485 (safeCheck), ~line 1501 (getAllBoardingUtxos), ~line 1741 (saveSendTransaction), send.go (getSpendableVtxos), funding.go (NewOffchainAddress, NewBoardingAddress, NewOnchainAddress):
All these methods read a.contractManager directly without holding cmMu. Meanwhile, Lock(), Reset(), and Stop() set a.contractManager = nil under cmMu.Lock().
The contractMgr() accessor exists and correctly uses cmMu.RLock(), but it's only used in ContractManager() and listenForArkTxs. Every other call site bypasses it.
TOCTOU scenario: safeCheck() observes a.contractManager != nil → concurrent Lock() sets it to nil → caller dereferences a nil pointer → panic or silent data corruption.
// safeCheck reads without lock:
if a.contractManager == nil { // ← DATA RACE
return ErrNotInitialized
}
// ... caller then uses a.contractManager without lockFix: All reads of a.contractManager must go through contractMgr() or hold cmMu.RLock(). Alternatively, safeCheck() should return the manager under lock so the caller can use the returned value safely.
🔴 P0 — serializeVHTLCOpts silently swallows BIP68 errors (timelock bypass)
contract/vhtlc_handler.go ~lines 151-154 (serializeVHTLCOpts):
claimSeq, _ := arklib.BIP68Sequence(opts.UnilateralClaimDelay)
refundSeq, _ := arklib.BIP68Sequence(opts.UnilateralRefundDelay)
noRcvrSeq, _ := arklib.BIP68Sequence(opts.UnilateralRefundWithoutReceiverDelay)If BIP68Sequence fails (e.g. invalid locktime type), the sequence is silently set to 0. A contract serialized with sequence 0 has no CSV timelock, meaning the unilateral exit path could be spent immediately. This defeats the entire security model of the VHTLC.
Fix: Propagate the error. serializeVHTLCOpts should return (map[string]string, error).
🔴 P1 — Public API break: GetAddresses return type change
sdk.go ~line 34:
// Before:
GetAddresses(ctx context.Context) (
onchainAddresses, offchainAddresses, boardingAddresses, redemptionAddresses []string, err error)
// After:
GetAddresses(ctx context.Context) (
onchainAddresses []string,
offchainAddresses, boardingAddresses, redemptionAddresses []clientTypes.Address, err error)This breaks all downstream consumers of the ArkClient interface. Confirmed consumers:
bancod/pkg/contract/taker.go:103—_, offchainAddrs, _, _, err := wp.Wallet().GetAddresses(ctx)demos/asset/golang/main.go:100asset-demos/asset/golang/main.go:100
These repos compile against go-sdk and will fail. Need coordinated PRs on downstream repos or a migration path.
🟡 P2 — NewVHTLC TOCTOU: duplicate event emission
contract/manager.go ~lines 1976-1998 (NewVHTLC):
m.mu.RLock()
existing, found := m.contracts[c.Script]
m.mu.RUnlock()
if found { return &existing, nil }
// ← Another goroutine can reach here with same script
c.CreatedAt = time.Now()
if err := m.persistAndCache(ctx, *c); err != nil { ... }
m.emit(Event{Type: "contract_created", Contract: *c})Unlike NewDefault and NewDelegate which hold dedicated create mutexes, NewVHTLC has no serialization. Two concurrent calls with identical params will both pass the existence check, both persist (benign due to upsert), and both emit contract_created events. The watcher will attempt to subscribe the same address twice.
Fix: Add a vhtlcCreateMu or use the script as the lock key.
🟡 P2 — Deprecated ripemd160 import
vhtlc/vhtlc.go line 11:
"golang.org/x/crypto/ripemd160" //nolint:staticcheckbtcutil.Hash160 (used in the tests) wraps the same logic. Using the deprecated package directly is unnecessary. Use btcutil.Hash160 in production code for consistency.
🟡 P2 — Inconsistent pubkey serialization: delegate key is 33 bytes, all others are 32
contract/delegate_handler.go ~line 149:
ParamDelegateKey: hex.EncodeToString(delegateKey.SerializeCompressed()),Every other pubkey param uses schnorr.SerializePubKey (32-byte x-only). The delegate key uses SerializeCompressed() (33-byte SEC). This inconsistency will cause confusion if any downstream code tries to deserialize it as a schnorr key or compare it against other params.
If the 33-byte form is intentional (e.g. to preserve the y-coordinate parity), add a comment explaining why. Otherwise, standardize on 32-byte x-only.
🟡 P2 — ListContracts SQL filter ignores IsOnchain and KeyID
store/sql/contract_store.go ListContracts:
The Filter struct has IsOnchain and KeyID fields, but ListContracts only applies Type, State, and Script filters. The IsOnchain and KeyID filters are silently ignored at the SQL layer (they only work in the in-memory GetContracts path in manager.go).
This means querying the persistent store directly (e.g. during Load()) won't honor these filters. Currently Load() always passes Filter{} so it's not a bug today, but it's a trap for future callers.
Fix: Add IsOnchain and KeyID (via JSON extract) conditions to the SQL query.
🟢 P3 — Nit: toBitcoinNetwork duplicated → now returns error instead of defaulting to mainnet
contract/default_handler.go vs old utils.go:
The old toBitcoinNetwork silently defaulted to mainnet for unknown networks. The new version returns an error. This is good — it's safer. Just noting the behavior change for awareness.
🟢 P3 — Opts() extracts preimage hash via slice offset [2:2+hash160Len]
vhtlc/vhtlc.go Opts() method:
PreimageHash: v.preimageConditionScript[2 : 2+hash160Len],This assumes the condition script layout is always OP_HASH160 <20-byte push> OP_EQUAL with the hash at offset 2. If the script builder ever changes the push encoding (e.g. for longer hashes), this will silently return wrong bytes. Consider parsing the script properly or at minimum adding a length assertion.
🟢 P3 — Good: VHTLC tapscript leaf ordering matches ts-sdk
Verified against /root/arkana/repos/ts-sdk/src/script/vhtlc.ts lines 110-117. The 6-leaf ordering is an exact match. Cross-SDK VHTLC interoperability is preserved. ✅
🟢 P3 — Good: Delegate closure ordering test
TestDelegateHandler_ClosureOrdering correctly verifies that forfeitClosures[0] is the 2-of-2 (owner+server) and not the 3-of-3 (owner+delegate+server). This is important because arkd uses forfeitClosures[0] for forfeit tx construction. ✅
🟢 P3 — Good: Test coverage
2,800+ lines of new tests covering:
- Handler derivation determinism
- Path selection for all contract types
- Manager concurrency (20-goroutine test for NewDelegate)
- Watcher lifecycle (subscribe, backoff, reconnect, dedup, stop)
- SQL store CRUD and filter round-trips
- VHTLC param round-trip serialization
Summary
| Severity | Count | Key Issues |
|---|---|---|
| 🔴 P0 | 2 | Data races on contractManager; silent BIP68 error swallowing |
| 🔴 P1 | 1 | Public API break (GetAddresses) with known downstream consumers |
| 🟡 P2 | 3 | VHTLC TOCTOU, deprecated ripemd160, SQL filter gap |
| 🟢 P3 | 4 | Nits, observations, positive callouts |
Requesting changes for the P0 data race and the P0 BIP68 silent error. The P1 API break needs a migration plan with downstream repos (bancod, demos).
The VHTLC tapscript layout, closure ordering, and cross-SDK compatibility all look correct. Architecture is solid. Fix the safety issues and this is ready for human sign-off.
bitcoin-coder-bob
force-pushed
the
feat/vhtlc-contract
branch
from
8cc20af to
a1c38fd
Compare
There was a problem hiding this comment.
🔄 Follow-up Review — #157 (post force-push)
PR was significantly scoped down (~1682 lines vs ~6000 previously). The client.go/send.go/funding.go refactor, the GetAddresses API change, and the SQL store changes are all gone. What remains is focused: VHTLC handler + vhtlc package + Manager interface extension.
Checked against my previous review — here's what's fixed, what's still open, and new findings.
✅ Previous findings resolved by scope reduction
- P0 data races on
a.contractManager— those files are no longer in the diff - P1
GetAddressesAPI break — removed from PR scope - P2 SQL filter gap — removed from PR scope
- P2 delegate key serialization — not changed in this diff
✅ Previous finding I retract
- P2 deprecated
ripemd160import — I was wrong.NewVhtlcScriptreceives a SHA-256 hash and needs raw RIPEMD160 (not HASH160 = RIPEMD160∘SHA256).btcutil.Hash160would double-hash. The directripemd160import is correct.
🔴 P0 — STILL OPEN: serializeVHTLCOpts silently swallows BIP68 errors (timelock bypass)
contract/vhtlc_handler.go lines 437-440:
claimSeq, _ := arklib.BIP68Sequence(opts.UnilateralClaimDelay)
refundSeq, _ := arklib.BIP68Sequence(opts.UnilateralRefundDelay)
noRcvrSeq, _ := arklib.BIP68Sequence(opts.UnilateralRefundWithoutReceiverDelay)Unchanged from previous review. If BIP68Sequence returns an error, the sequence defaults to 0, producing a serialized contract with no CSV timelock. A contract loaded from these params would allow immediate unilateral exit, defeating the security model.
Fix: serializeVHTLCOpts should return (map[string]string, error) and propagate BIP68 errors. Callers (DeriveContract at line 212) should check the error.
🟡 P2 — NEW: GetSpendablePaths returns CLTV-locked path without CLTV check
contract/vhtlc_handler.go lines 319-327 (GetSpendablePaths, sender collaborative):
if role == "sender" {
lt, _ := vhtlcRefundLocktime(c.Params)
sel, err := tapLeafSelection(tapscripts[2], nil, <)
...
paths = append(paths, *sel)
}Compare to SelectPath (line 259) which correctly gates on isVHTLCCltvSatisfied:
if role == "sender" && isVHTLCCltvSatisfied(c.Params, pctx) {GetSpendablePaths unconditionally returns leaf[2] (the CLTV-locked refundWithoutReceiver path) for senders in collaborative mode, regardless of whether the CLTV is satisfied. The docstring says "CSV timelocks are not checked" but this is a CLTV (absolute) lock, not CSV (relative). A caller using GetSpendablePaths could attempt to broadcast a transaction that miners will reject because nLockTime < refundLocktime.
Additionally, vhtlcRefundLocktime error is swallowed — lt defaults to 0 on parse failure, meaning nLockTime=0 would be set.
Fix: Either (a) check isVHTLCCltvSatisfied like SelectPath does, or (b) explicitly document that CLTV-locked paths are returned unchecked and callers must verify. Also propagate the vhtlcRefundLocktime error.
🟡 P2 — STILL OPEN: NewVHTLC TOCTOU / duplicate event emission
contract/manager.go lines 69-84:
existing, err := m.store.GetContractsByScripts(ctx, []string{c.Script})
...
if len(existing) > 0 { return &existing[0], nil }
c.CreatedAt = time.Now()
if err := m.store.AddContract(ctx, *c, 0); err != nil { ... }
m.emit(*c)No mutex serialization. Two concurrent NewVHTLC calls with the same params will both pass the existence check (neither has persisted yet), both persist (benign if upsert), and both emit contract_created. The watcher subscribes the same address twice.
Fix: Same as before — add a vhtlcCreateMu or reuse an existing create mutex.
🟡 P2 — NEW: SelectPath swallows vhtlcRefundLocktime error
contract/vhtlc_handler.go line 261:
lt, _ := vhtlcRefundLocktime(c.Params)If the stored refundLocktime param is corrupt, lt defaults to 0, setting nLockTime=0 on the spending transaction. While isVHTLCCltvSatisfied would have already parsed it successfully to reach this line, the error should still be propagated for defensive correctness.
🟢 P3 — GetRevealedTapscripts silently drops closures on Script() error
vhtlc/vhtlc.go lines 231-237:
if s, err := closure.Script(); err == nil {
scripts = append(scripts, hex.EncodeToString(s))
}If any closure's Script() fails, it's silently skipped. This would produce fewer than 6 tapscripts, and downstream code checking len(tapscripts) < 6 would get a confusing "got 5" error instead of the actual underlying issue. Consider returning an error or panicking (since these closures were validated at construction time).
✅ Positive observations
- Cross-SDK leaf ordering matches ts-sdk — verified against
ts-sdk/src/script/vhtlc.tslines 110-117. All 6 leaves, including pubkey ordering within each leaf, are identical. ✅ - Test coverage is strong — 469 lines covering derivation determinism, param round-trips, all path selection scenarios (7 for SelectPath, 5 for GetSpendablePaths), missing param validation, and wrong-length hash rejection. ✅
- Cross-repo impact is safe —
NewVHTLCadded toManagerinterface; only concrete impl is in go-sdk. External mocks in bancod use struct embedding and won't break at compile time. ✅ - ripemd160 usage is correct — the SHA256→RIPEMD160 path in
NewVhtlcScriptis intentional and correct. ✅
Summary
| Severity | Count | Status |
|---|---|---|
| 🔴 P0 | 1 | serializeVHTLCOpts BIP68 error swallowing — still open from previous review |
| 🟡 P2 | 3 | CLTV check missing in GetSpendablePaths (new), TOCTOU (still open), locktime error swallowed (new) |
| 🟢 P3 | 1 | GetRevealedTapscripts silent error |
Blocking on P0 (BIP68 error swallowing). The P2s should also be addressed but are lower risk in practice.
There was a problem hiding this comment.
✅ Follow-up Review #3 — #157 (commit 73cfa24d)
Reviewed diff a1c38fd8..73cfa24d ("fix vhtlc security review findings").
All previous findings resolved
| Previous Finding | Status |
|---|---|
🔴 P0 serializeVHTLCOpts BIP68 error swallowing |
✅ Fixed — returns (map[string]string, error), all 3 BIP68 errors propagated (vhtlc_handler.go:337-349) |
🟡 P2 GetSpendablePaths missing CLTV check |
✅ Fixed — now gates on isVHTLCCltvSatisfied (vhtlc_handler.go:216), matching SelectPath behavior |
🟡 P2 NewVHTLC TOCTOU race |
✅ Fixed — m.mu.Lock() now wraps check-then-insert (manager.go:334-353), unlock before emit is intentional and correct |
🟡 P2 SelectPath swallowed vhtlcRefundLocktime error |
✅ Fixed — error propagated (vhtlc_handler.go:154-156) |
🟡 P2 GetSpendablePaths swallowed vhtlcRefundLocktime error |
✅ Fixed — error propagated (vhtlc_handler.go:217-219) |
🟢 P3 GetRevealedTapscripts silent error drop |
✅ Fixed — now returns ([]string, error) (vhtlc.go:172), caller checks error (vhtlc_handler.go:88-90) |
Tests updated
GetSpendablePathssender collaborative test split into CLTV-satisfied and CLTV-not-satisfied cases (vhtlc_handler_test.go:413-441)
No new issues found
Code is clean. Approving.
Reviewed by Arkana 🔮 — 3rd pass, focused on commit 73cfa24d
Conflict in contract/types.go: incoming branch split NewDelegate out of Manager into a separate DelegateCreator interface; resolved by keeping DelegateCreator separate and retaining NewVHTLC in Manager.
There was a problem hiding this comment.
🔮 Arkana Review — 4th pass (merge commit d77095a6)
Focused on delta since last review (73cfa24d → d77095a6). This merge brings in feat/delegate-contract changes.
Changes reviewed
contract/types.go—DelegateCreatorinterface extracted fromManagercontract/manager.go— Dedup logic inNewDelegate+findDelegateContractByKeyhelpercontract/delegate_handler.go— Nil checks for owner/signer keys
✅ Cross-repo impact: CLEAR
Searched all repos under /root/arkana/repos/ — no external consumer imports go-sdk/contract or references contract.Manager directly. The NewDelegate → DelegateCreator extraction is safe. bancod, introspector-review, demos all consume go-sdk at the top-level ArkClient level only.
✅ Interface compliance: CLEAR
Both test mocks (watcherMockManager, fixContractManager) implement NewVHTLC to satisfy the updated Manager interface. Extra NewDelegate methods on mocks are harmless in Go.
✅ Nil checks in delegate_handler.go:84-89: GOOD
Defensive checks for key.PubKey and cfg.SignerKey before use. Prevents nil-pointer panics.
⚠️ Minor: Manual mutex in NewDelegate (manager.go:250-312)
NewDelegate has 9 separate m.mu.Unlock() calls across error paths. All paths are correctly covered today, but this is fragile — future edits could easily miss an unlock. Consider refactoring to use a pattern like:
result, err := func() (*types.Contract, error) {
m.mu.Lock()
defer m.mu.Unlock()
// ... all the logic
}()Or at minimum, extract the critical section into a helper that uses defer. Not blocking, but worth hardening.
✅ Dedup logic in findDelegateContractByKey (manager.go:565-577): CORRECT
Linear scan over delegate contracts by type. O(n) per call, called under mutex. Acceptable for current scale. If delegate contract count grows significantly, consider indexing by key in the store layer.
✅ CI: Passing
Verdict: Merge commit is clean. No protocol-critical changes, no security issues, no breakage. Previous review findings on the VHTLC code still apply.
Reviewed by Arkana 🔮 — 4th pass, focused on merge commit d77095a6
There was a problem hiding this comment.
🔮 Arkana Review — 4th pass (merge commit d77095a6)
Focused on delta since last review (73cfa24d → d77095a6). This merge brings in feat/delegate-contract changes.
Changes reviewed
contract/types.go—DelegateCreatorinterface extracted fromManagercontract/manager.go— Dedup logic inNewDelegate+findDelegateContractByKeyhelpercontract/delegate_handler.go— Nil checks for owner/signer keys
✅ Cross-repo impact: CLEAR
Searched all repos under /root/arkana/repos/ — no external consumer imports go-sdk/contract or references contract.Manager directly. The NewDelegate → DelegateCreator extraction is safe. bancod, introspector-review, demos all consume go-sdk at the top-level ArkClient level only.
✅ Interface compliance: CLEAR
Both test mocks (watcherMockManager, fixContractManager) implement NewVHTLC to satisfy the updated Manager interface. Extra NewDelegate methods on mocks are harmless in Go.
✅ Nil checks in delegate_handler.go:84-89: GOOD
Defensive checks for key.PubKey and cfg.SignerKey before use. Prevents nil-pointer panics.
⚠️ Minor: Manual mutex in NewDelegate (manager.go:250-312)
NewDelegate has 9 separate m.mu.Unlock() calls across error paths. All paths are correctly covered today, but this is fragile — future edits could easily miss an unlock. Consider refactoring to use a closure pattern:
result, err := func() (*types.Contract, error) {
m.mu.Lock()
defer m.mu.Unlock()
// ... critical section
}()Not blocking, but worth hardening.
✅ Dedup logic in findDelegateContractByKey (manager.go:565-577): CORRECT
Linear scan over delegate contracts by type. O(n) per call, called under mutex. Acceptable for current scale. If delegate contract count grows significantly, consider indexing by key in the store layer.
✅ CI: Passing
Verdict: Merge commit is clean. No protocol-critical changes, no security issues, no breakage. Previous review findings on the VHTLC code still apply.
Reviewed by Arkana 🔮 — 4th pass, focused on merge commit d77095a6
"Manual mutex in |
There was a problem hiding this comment.
🔮 Arkana Review — 5th pass (commit b8372708)
Focused on delta since last review (d77095a6 → b8372708).
✅ NewDelegate mutex refactor — looks good
The closure+defer m.mu.Unlock() pattern in NewDelegate is correct and addresses the manual-unlock feedback from pass 4. The isNew guard on m.emit() is a nice touch — avoids emitting for already-existing contracts returned from the cache path.
⚠️ One remaining nit: NewVHTLC still uses manual unlocks
contract/manager.go:NewVHTLC (~line 370–400) still does manual m.mu.Unlock() on each error path — the same pattern that was just cleaned up in NewDelegate. For consistency and safety, consider applying the same closure+defer treatment. Not a blocker, but it'd be inconsistent to fix one and leave the other.
No new protocol-correctness or security issues found in this delta.
Reviewed by Arkana 🔮 — 5th pass, focused on commit b8372708
There was a problem hiding this comment.
🔮 Arkana Review — 5th pass (commit b8372708)
Focused on delta since last review (d77095a6 → b8372708).
✅ NewDelegate mutex refactor — looks good
The closure+defer m.mu.Unlock() pattern in NewDelegate is correct and addresses the manual-unlock feedback from pass 4. The isNew guard on m.emit() is a nice touch — avoids emitting for already-existing contracts returned from the cache path.
⚠️ One remaining nit: NewVHTLC still uses manual unlocks
contract/manager.go:NewVHTLC (~line 370–400) still does manual m.mu.Unlock() on each error path — the same pattern that was just cleaned up in NewDelegate. For consistency and safety, consider applying the same closure+defer treatment. Not a blocker, but it'd be inconsistent to fix one and leave the other.
No new protocol-correctness or security issues found in this delta.
Reviewed by Arkana 🔮 — 5th pass, focused on commit b8372708
There was a problem hiding this comment.
🔮 Arkana Review — 6th pass (commit d0bd113)
Focused on delta since last review (b8372708 → d0bd113).
Single commit: d0bd113 — closure+defer mutex pattern for NewVHTLC
Mechanical refactor only. Changes:
NewVHTLCmutex handling moved into an anonymous closure withdefer m.mu.Unlock(), matching the pattern already used inNewDelegatesinceb8372708.- No logic changes. The lock/unlock boundaries, error paths, return values, and
m.emit()placement (outside the lock) are all preserved identically.
Verdict: ✅ Clean
No new findings. The refactor eliminates the risk of forgetting an m.mu.Unlock() on a new error path — strictly safer than the previous manual unlock style.
Previous findings from passes 1–5 still apply where not yet addressed.
Reviewed by Arkana 🔮 — 6th pass, focused on commit d0bd113
There was a problem hiding this comment.
🔮 Arkana Review — 6th pass (commit d0bd113)
Focused on delta since last review (b8372708 → d0bd113).
Single commit: d0bd113 — closure+defer mutex pattern for NewVHTLC
Mechanical refactor only. Changes:
NewVHTLCmutex handling moved into an anonymous closure withdefer m.mu.Unlock(), matching the pattern already used inNewDelegatesinceb8372708.- No logic changes. The lock/unlock boundaries, error paths, return values, and
m.emit()placement (outside the lock) are all preserved identically.
Verdict: ✅ Clean
No new findings. The refactor eliminates the risk of forgetting an m.mu.Unlock() on a new error path — strictly safer than the previous manual unlock style.
Previous findings from passes 1–5 still apply where not yet addressed.
Reviewed by Arkana 🔮 — 6th pass, focused on commit d0bd113
Resolve conflict in NewDelegate by taking the named-helper approach (newDelegateLocked + nil guard) from feat/delegate-contract.
1 participant
closes #150
work in progress