multi: rebuild refresh output template against current operator key (#520) - #525
Conversation
|
@claude review this |
|
Claude finished @ellemouton's task in 2m 43s —— View job Code Review
Overall the fix is well-scoped and the design is sound: split the "rebuild the new output's template" concern cleanly from spend-time material, add a nil-safe fallback seam, wire it through the config chain, and cover it with a targeted regression test. A few findings below.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #520 by ensuring that VTXO refresh operations utilize the operator's current long-term public key rather than stale keys persisted in descriptors. Changes include introducing a CurrentOperatorKey lookup mechanism across the server, VTXO manager, and wallet subsystems, along with logic to rebuild policy templates for standard VTXO shapes. Feedback was provided regarding wallet/policy.go to improve error wrapping for better fallback behavior and to maintain implementation consistency with the vtxo package.
| if d == nil { | ||
| return nil, fmt.Errorf("wallet VTXO descriptor must be " + | ||
| "provided") | ||
| } | ||
|
|
||
| if currentOperatorKey == nil { | ||
| return nil, fmt.Errorf("current operator key must be provided") | ||
| } | ||
|
|
||
| if len(d.PolicyTemplate) == 0 { | ||
| return nil, fmt.Errorf("wallet VTXO descriptor policy " + | ||
| "template must be provided") | ||
| } | ||
|
|
||
| template, err := arkscript.DecodePolicyTemplate(d.PolicyTemplate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("decode stored policy template: %w", err) | ||
| } |
There was a problem hiding this comment.
The error returned when arkscript.DecodePolicyTemplate fails should be wrapped with ErrRefreshOperatorKeyUnsupported. This ensures that if the stored template is unparseable (and thus not a standard Ark VTXO shape), the logic falls back to the raw bytes instead of failing the refresh operation entirely. Additionally, the manual nil and empty checks can be simplified by calling EffectivePolicyTemplate directly, matching the pattern used in the vtxo package.
References
- Maintain consistency with the vtxo package implementation of RefreshOutputTemplate. (link)
- Ensure consistent error handling and fallback logic across similar subsystems (wallet vs vtxo).
cba20bf to
d848946
Compare
|
Reworked per review feedback. The previous iteration read the operator key from the daemon-startup Key changes vs the previous push:
@claude review this |
|
Claude finished @ellemouton's task in 3m 30s —— View job Claude finished @ellemouton's task ——
Code ReviewThe rework is a clear improvement over the cached-key approach. Using a live
|
d848946 to
493fd2a
Compare
|
Round 3 — all of @claude's actionable items + the two CI failures addressed:
|
|
@claude re-review |
|
Claude finished @ellemouton's task in 4m 16s —— View job Code Review (Round 4)
All three actionable items from Round 3 are addressed. The code is in good shape. One remaining finding below. Issue number references in code comments violate CLAUDE.mdCLAUDE.md is explicit: "Do not reference the current task, fix, or callers ("used by X", "added for the Y flow", "handles the case from issue #123"), since those belong in the PR description and rot as the codebase evolves." There are eight places in the new code where issue numbers appear in comments or godoc:
In each case the design principle is already expressed in plain English — "VTXOs commit to their operator key for life, the new output's key is chosen at join time" — so the issue number adds no semantic content and will become a dead reference once the issue is closed. The same applies to the Minor:
|
Fixes darepo-client issue #520. The client now resolves the operator's current long-term key at refresh-join time via a fresh GetInfo round-trip and rebuilds the NEW VTXO output's policy template against that key. The input VTXO's stored operator key is still respected for spend-time material — it has to be, the on-chain output's taproot tree commits to it — but the new output is a freshly-minted VTXO whose operator key is chosen at join time and is then stable on that new VTXO for life. This replaces the earlier iteration of this PR that read from the daemon-startup OperatorTerms cache. As noted in darepo#335, that cache is the wrong source of truth across a rotation because it only hydrates at bootstrap; the right source is a join-time fetch. Touched surfaces: - vtxo/policy.go, wallet/policy.go: RefreshOutputTemplate helper rebuilds the standard template against a caller-supplied current key, preserving the owner key and exit delay. Non-standard shapes (vHTLC, custom) return ErrRefreshOperatorKeyUnsupported. The wallet helper also wraps DecodePolicyTemplate failures with the sentinel so callers can branch on a single errors.Is check, mirroring the vtxo side's DecodeStandardPolicyTemplate path. - vtxo/actor.go, vtxo/manager.go: new FetchOperatorKey func field on VTXOActorConfig and ManagerConfig (manager forwards to spawned actors). The auto-refresh emission calls FetchOperatorKey at the ForfeitRequest branch; fetch errors propagate so the refresh fails rather than silently emitting against a stale key. Auto-refresh retries naturally on the next expiry tick. The build failure is logged at warn level (external trigger, not an internal bug). - wallet/wallet.go: new WithFetchOperatorKey ArkOption, plus a single fresh fetch hoisted to the top of handleRefreshVTXOs so one GetInfo covers the whole batch (every new VTXO in one RPC commits to the same key). composeRefreshTemplate is a free helper the loop uses with the pre-fetched key. - darepod/server.go: new fetchCurrentOperatorPubKey accessor that calls fetchOperatorTerms over the wire, refreshes the cache via storeOperatorTerms so unrelated readers see the same snapshot, and returns the pubkey. Wired into wallet.NewArk and vtxo.NewManager at construction time. - vtxo/refresh_operator_rotation_test.go: regression test with four subtests — rebuild against fetched key, fall back when fetch unset, propagate fetch errors, reject nil fetched keys. - wallet/refresh_operator_rotation_test.go: parallel regression test for composeRefreshTemplate, completing the coverage the vtxo side pins.
493fd2a to
520c32d
Compare
|
Round 4 review addressed:
|
2 participants
Summary
Fixes #520. After an operator key rotation, the client's refresh path
silently constructed a
JoinRoundRequestwhose new VTXO output templatestill carried the operator's pre-rotation key (K1), and the server
rejected the round with
ErrOperatorKeyMismatch. This PR rebuilds thenew output's standard policy template against the operator's current
long-term key (K2) while leaving spend-time material for the old VTXO
untouched (it still has to commit to K1, because the on-chain output's
taproot tree was built against K1).
The cause sat in two places, both of which cloned the descriptor's stored
PolicyTemplatebytes verbatim into the new output:vtxo/actor.go— auto-refresh emission on expiry (processOutbox/ForfeitRequest).wallet/wallet.go— explicitRefreshVTXOsRPC handler(
handleRefreshVTXOs).Both now consult a
func() *btcec.PublicKeyprovider wired in bydarepodfrom the cachedOperatorTermssnapshot, and rebuild the newoutput's template against the returned key. When the provider is unset
or the descriptor's stored policy is not the standard shape (vHTLC,
custom), the code falls back to the descriptor's stored bytes so legacy
paths and non-standard policies keep their current behavior.
What's in the diff
vtxo/policy.go,wallet/policy.go— newRefreshOutputTemplatehelper on each descriptor type. Standard shape only; non-standard
shapes return
ErrRefreshOperatorKeyUnsupportedso callers can fallback or surface rotation-specific UX explicitly.
vtxo/actor.go,vtxo/manager.go— newCurrentOperatorKey func()field on
VTXOActorConfigandManagerConfig(manager forwards toeach spawned actor). The auto-refresh emission goes through a new
refreshOutputTemplatehelper that prefers the rebuilt template andfalls back to the stored bytes on non-standard shapes.
wallet/wallet.go— newWithCurrentOperatorKeyArkOptionand aparallel
refreshOutputTemplatehelper used byhandleRefreshVTXOs.darepod/server.go— smallcurrentOperatorPubKey()accessor thatreads the cached
OperatorTerms, wired into bothwallet.NewArkandthe
vtxo.ManagerConfigat construction time.vtxo/refresh_operator_rotation_test.go— regression test with twosubtests:
a
VTXOActorconfigured with aCurrentOperatorKeyreturning K2emits a
RefreshVTXORequestwhosePolicyTemplatedecodes to K2,not the K1 baked into the descriptor.
legacy path so harness tests and non-standard policy holders keep
working unchanged.
Design notes for review
Forfeit and unilateral-exit material is intentionally untouched.
Only the new output's template is rewritten. The forfeit witness,
control block, and unilateral exit script for the input VTXO still
commit to K1 because that is what the on-chain output's taproot tree
was built against. This matches the acceptance criteria in client: refresh/join round can fail after operator key rotation #520.
Non-standard policies keep the legacy fallback. vHTLC and other
custom shapes still ship the stored bytes verbatim. A user holding a
vHTLC VTXO under K1 across a rotation will see the same failure as
before — out of scope for client: refresh/join round can fail after operator key rotation #520, but worth a follow-up if it matters.
A nil provider is treated as 'not wired yet'. The actor and wallet
helpers fall back to
EffectivePolicyTemplateso harnesses that don'tmock
OperatorTermskeep their current behavior.Test plan
go test ./vtxo/ ./wallet/ ./darepod/ -count=1— green(full unit suites for the three touched packages)
TestRefreshEmissionUsesCurrentOperatorKeypasses both subtests
darepo: re-run the existingTestRefreshIntegrationSingleVTXOLifecycleitest against this branch to confirm the seal-time fee handshake and
forfeit signing still work end-to-end through the new emission path
(this PR's tests don't drive a real round).