Wallet rotation safety (integration preview): eager forfeit + multi-wallet sweeps + signer-key-per-leaf - #1120
Closed
bitcoin-coder-bob wants to merge 41 commits into
Closed
Wallet rotation safety (integration preview): eager forfeit + multi-wallet sweeps + signer-key-per-leaf#1120bitcoin-coder-bob wants to merge 41 commits into
bitcoin-coder-bob wants to merge 41 commits into
Conversation
…key PSBT Forfeit txs are now operator-signed at collection time. Re-signing them at fraud-broadcast time appended a second identical operator signature, producing an invalid PSBT (duplicate key) that failed to finalize. Only sign when the operator signature is still missing (legacy forfeits).
arkd no longer creates or unlocks the wallet. At startup it now fails fast unless the primary arkd-wallet reports initialized and unlocked. The unlock phase is retained but only unlocks the macaroon (admin auth) service. This is the first step towards letting a single arkd front multiple LPs (arkd-wallets).
Replace the README curl steps with first-class arkd-wallet commands that init/unlock the wallet out of band via its own gateway. Bare arkd-wallet still runs the service.
- arkd-wallet create/unlock subcommands build their request bodies with json.Marshal instead of fmt.Sprintf, so a password or mnemonic containing quotes or backslashes can't corrupt the payload or inject fields. - The admin Lock RPC now returns Unimplemented: arkd no longer manages the wallet lifecycle, and locking a (possibly shared) wallet via arkd would break other consumers. Lock out of band through arkd-wallet instead. - Drop the unreachable return after log.Exit by stopping the service explicitly.
Add ARKD_WALLET_FALLBACK_ADDRS so arkd can connect to a primary arkd-wallet plus additional LP wallets. Fallbacks are dialed and validated (reachable, same network, initialized and unlocked) at startup; the primary is unchanged and remains the sole source of the forfeit pubkey, addresses and signing. Fallbacks are not used by sweep yet. The regtest compose stack now runs a second arkd-wallet so the existing e2e suite exercises arkd with a fallback plugged in.
Split the sweep build from signing: the destination and fees always come from the primary wallet (stable txid), then signing is attempted with each of the primary/fallback arkd-wallets until one succeeds. Broadcasting stays on the primary. This lets a single arkd sweep batches signed by any of its LPs' wallets.
- newSweeper uses named struct fields so inserting a field can't silently misassign the positional values. - Extract primaryThenFallbacks so the sweeper and adminService share the primary-then-fallbacks ordering instead of duplicating it.
Move the primary and fallback arkd-wallet connection addresses into the DB-backed settings: seeded from env on first boot, sourced from the settings to dial the wallets, and settable via the admin Settings API. Wallet address changes are persisted immediately but only take effect on the next restart (re-dialing live is a follow-up).
- walletService only overrides env wallet addresses when the settings carry a value, so an upgrade (whose migrated row defaults them to empty) doesn't drop env-configured fallback wallets; log instead of swallowing a settings read error. - splitFallbackAddrs trims and drops empty entries, matching the env parser. - Document that the fallback list can be replaced but not cleared via the API.
- Validate() rejects a comma in the primary or any fallback wallet address, since they're persisted comma-separated (a comma would corrupt the column). - Move the comma-separated encode/decode into domain (EncodeFallbackAddrs / DecodeFallbackAddrs) so postgres and sqlite share one implementation. - Document the wallet-target trust boundary in walletService and log the effective primary wallet address at startup so operators can audit it.
# Conflicts: # internal/infrastructure/db/postgres/sqlc/query.sql
broadcastForfeitTx now reads the operator key set (current + deprecated) from the cached settings instead of calling the live signer, so a forfeit pre-signed at collection time stays broadcastable when the signer is unavailable, which is the point of signing at collection time. Deprecated keys are included so a forfeit signed before a signer-key rotation is recognized as already signed and not re-signed with the current key (which would not satisfy its old-key tapscript). The backfill tool applies the same current+deprecated check.
UpdateForfeitTx silently no-op'd on a txid miss (sqlite/postgres) or inserted a stray record (badger), so the forfeit backfill could report a tx as signed when nothing was written. - sqlite/postgres: UpdateForfeitTx -> :execrows; PatchForfeitTxs errors when zero rows match. - badger: Upsert -> Update so a missing txid returns ErrNotFound instead of inserting; mapped to the same not-found error. - tests: assert the not-found path on the sqlite/postgres harness and add a focused badger PatchForfeitTxs test.
Pair each dialed fallback wallet with its address (FallbackWallet) so ensureWalletReady reports the specific wallet that failed (host:port) instead of a positional index, which isn't actionable when the error surfaces. Also clarify the comment on why fallback balance isn't checked.
On this branch arkd only dials and readiness-checks fallback wallets at startup; sweep signing isn't wired up yet. Reword the docker-compose and dialFallbackWallets comments so they don't imply active sweep-fallback use.
…er fallback Fail fast at dial time if a fallback address equals the primary WalletAddr or repeats another fallback, instead of silently dialing the same wallet twice (pointless now, double-spend/lock-contention risk once fallbacks sign sweeps). Matching is by literal address.
… bob/arkd-sweep-fallback
- inline the one-line primary+fallbacks append into each signingWallets() method and drop the primaryThenFallbacks helper (one less indirection) - rename sweep_fallback_internal_test.go to sweeper_test.go to match sweeper.go and the package's white-box test convention - split the BuildSweepTx/SignSweepTx interface doc into per-method comments and trim the verbose dialFallbackWallets / sweep tx comments
Resolve config.go dialFallbackWallets: combine #1102's settings-sourced fallbackAddrs parameter with sweep-fallback's []FallbackWallet return and dedup/self-reference validation. Pass the resolved primary (arkWallet) so the self-reference check is correct when the primary comes from settings, not just the WALLET_ADDR env var. Update the test call sites to the 2-arg signature.
The previous note read as a contradiction ('an exception: like the other
settings'). Spell out that wallet addresses are seeded+persisted like settings
but, unlike other settings, the env var stays a fallback on every boot when the
stored value is empty, which is what keeps upgrades (empty migrated columns)
working.
Trim verbose comments across the wallet-address settings code (proto, config walletService, domain Settings, admin UpdateSettings, adminservice parse) and remove the UpdateSettings log that warned wallet-address changes apply on the next restart.
…ned-tx store Address review on the sweep-fallback path: - BuildSweepTx now returns (txid, unsignedTx, err) to match buildAndSignSweepTx's (txid, signedTx, err), removing the foot-gun where createBatchSweepTask had both orderings (both string, so the compiler couldn't catch a mis-destructure). - buildAndSignSweepTx returns the txid even on signing failure, so the already-spent reconciliation path reuses it instead of calling BuildSweepTx a second time. - That path now stores an empty tx (not the unsigned PSBT) in the swept event, since it is never broadcast; round.Sweep only needs the txid.
…ures Introspect CSV and condition-CSV closures too so deprecated-key sweeps use the right key, and error when a required leaf references none of the wallet's keys instead of silently signing with the current one. Also stop dereferencing keyMgr in signer mode.
Regression test proving #1110's collection-time forfeit signing lets the server punish fraud across a hard signer rotation. Also remove the duplicate ARKD_BAN_THRESHOLD that fails docker compose validation.
Contributor
|
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 |
Collaborator
Author
|
Closing this integration/CI preview branch. It bundled several in-flight PRs so the full wallet-rotation-safety story could be reviewed and exercised by CI together; it was never meant to merge as a unit. The individual pieces live in their own PRs:
The TestEagerForfeitSurvivesWalletRotation e2e test added on this branch is a regression test for #1110's eager-forfeit fix and should be added there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
Integration / preview branch, not meant to be merged as a unit. It bundles several in-flight PRs plus one new fix so the full "what happens when you switch out a wallet with active batches/vtxos" story can be reviewed and exercised by CI together. The individual pieces land through their own PRs (linked below).
Why
Rotating the signer key, or switching the arkd wallet, while batches and vtxos are still live is high risk. On-chain funds and vtxo scripts stay locked to the old signer key, and several operations still need that key:
What's bundled
presign-forfeit-txs): sign forfeit txs at collection time so they are broadcast-ready without the live signer. Core fix for the fraud-after-rotation gap.dial/sweep-fallback/wallet-settings): dial a primary plus fallback arkd-wallets, sign sweeps with the primary or any fallback, and persist the wallet addresses in the settings domain. Lets an old wallet stay reachable as a sweep fallback across a switch.New in this branch
signerKeyForLeafhardening (pkg/arkd-wallet): select the signer key per tapscript leaf across all multisig closures, including CSV and condition-CSV. Previously CSV (sweep) leaves fell through to the current key, so deprecated-key sweeps of old-key outputs used the wrong key. Now an explicitly required leaf that references none of the wallet's keys is a hard error instead of a silent wrong-key signature, and the signer no longer dereferenceskeyMgrin signer mode (also fixes a nil-pointer panic in the existing wallet unit test).TestEagerForfeitSurvivesWalletRotation(e2e): regression test proving Sign forfeit txs at collection-time #1110's eager forfeit lets the server punish fraud across a hard signer rotation. It mirrors the pre-fix repro exactly (hard rotation, no deprecated key retained) but asserts the opposite outcome: the server broadcasts the pre-signed forfeit and claims the fraudulently unrolled vtxo.Validation (live regtest)
All green on this combined branch:
TestEagerForfeitSurvivesWalletRotation: eager forfeit survives a hard rotationTestDeprecatedSignerKey: rotation + deprecated keys + boarding (the explicit-index path through the newsignerKeyForLeaf)TestReactToFraud: normal forfeit / sweep / fraud reactions with everything combinedBase and how the pieces actually land
Targeted at
masterso CI runs the whole stack. Because it bundles #1110 and #1099/#1101/#1102, it should not be merged here; those PRs merge individually. The only genuinely new work, thesignerKeyForLeaffix and the verify test, should land as their own PRs once the others are in (signerKeyForLeafintomaster; the verify test onto #1110).