Skip to content

node, wallet: clean up derived validator keys on failed deposits (#379) - #1331

Open
Sanchay117 wants to merge 1 commit into
rocket-pool:masterfrom
Sanchay117:fix/cleanup-failed-deposit-keys-379
Open

Sanchay117 wants to merge 1 commit into
rocket-pool:masterfrom
Sanchay117:fix/cleanup-failed-deposit-keys-379

Conversation

@Sanchay117

@Sanchay117 Sanchay117 commented Nov 6, 2026 •

Copy link
Copy Markdown

Fixes #379

Summary

When rocketpool node deposit (nodeDeposits) fails mid-batch or during DepositMulti submission, Smartnode previously left newly derived validator keystore and secret files on disk across all consensus client keystores and discarded the in-memory next_account increment (because w.Save() was only called on the happy path at the end of nodeDeposits).

Under Saturn megapool batch deposits (count >= 1), this created two compounding hazards:

  1. Stray armed validator keys on disk (#379): Every w.CreateValidatorKey() iteration immediately writes encrypted keystore + password files for Lighthouse, Lodestar, Nimbus, Prysm (all-accounts.keystore.json), and Teku. If deposit validation, status lookup, or node.DepositMulti failed (e.g. FeeTooLowToCompete as in #386, RPC timeout, or contract revert), up to count unfunded validator keys remained on disk and would be loaded on the next VC restart.
  2. Derivation index reuse on retry (#379 / #427): Because services.GetWallet reloads hdWallet from disk on each API call, omitting w.Save() on failure caused next_account to revert to its pre-batch value. If an earlier transaction broadcast was ambiguous (e.g. mempool propagation timeout that later landed), a retry re-derived the exact same validator pubkeys.

Per @jshufro's specification in #379, failed deposits now delete all newly derived validator keys from every keystore while preserving (persisting) the incremented next_account counter on live (submit == true) deposit attempts.


Changes

  1. Per-key deletion across all 5 consensus client keystores (shared/services/wallet/keystore/...):

    • Added DeleteValidatorKey(pubkey types.ValidatorPubkey) error to keystore.Keystore.
    • Lighthouse / Lodestar / Nimbus: Idempotently remove validators/0x<pubkey> directory and secrets/0x<pubkey> file (os.ErrNotExist treated as nil).
    • Teku: Idempotently remove keys/0x<pubkey>.json and passwords/0x<pubkey>.txt.
    • Prysm (prysm-non-hd): If all-accounts.keystore.json does not exist yet, return nil without creating directories or secret. Otherwise load the account store, filter out matching pubkey/privkey entries while preserving the order of all remaining keys, and re-encrypt all-accounts.keystore.json in place.
  2. Wallet-level key deletion & partial-write rollback (shared/services/wallet/...):

    • Added DeleteValidatorKey(pubkey rptypes.ValidatorPubkey) error to wallet.Wallet, hdWallet, and masqueradeWallet (ErrIsMasquerading).
    • hdWallet.DeleteValidatorKey iterates deterministically across all registered keystores and joins errors via errors.Join so an error in one keystore never prevents cleanup of the remaining keystores. It intentionally leaves w.ws.NextAccount untouched.
    • hdWallet.CreateValidatorKey now rolls back any partially written keystore artifacts via DeleteValidatorKey if StoreValidatorKey fails mid-way across client keystores, while keeping NextAccount incremented.
  3. Transactional batch cleanup in nodeDeposits (rocketpool/api/node/deposit.go):

    • Extracted executeDepositBatch with a defer cleanup guard tracking createdPubkeys and depositSucceeded.
    • On any pre-broadcast error or panic (!depositSucceeded), all createdPubkeys from the batch are deleted across all 5 keystores, and when submit == true, w.Save() persists the advanced next_account to disk.
    • When submit == false (offline/unsigned tx generation), temporary keystore files are cleaned up on failure without mutating next_account on disk.
    • Post-broadcast safety invariant: depositSucceeded = true is set immediately upon submitDeposits (node.DepositMulti) returning err == nil (prior to w.Save()), guaranteeing that live validator keys for an already-broadcast deposit transaction are never deleted even if saving the wallet file subsequently fails.

Testing

Added unit tests covering both layers:

  • shared/services/wallet/validator_cleanup_test.go:
    • Deleting a middle key across all 5 real keystores (lighthouse, lodestar, nimbus, prysm, teku), verifying disk artifact removal, neighbor key preservation, Prysm reload from disk, idempotency, and next_account non-decrement.
    • Emptying Prysm's all-accounts.keystore.json down to 0 keys and reusing it for subsequent keys.
    • No-op delete on uninitialized keystores (no stray directories or Prysm password creation).
    • Rollback of partial keystore writes when CreateValidatorKey encounters a failing keystore.
    • Best-effort continuation across remaining keystores when one keystore errors on delete.
    • masqueradeWallet.DeleteValidatorKey returning ErrIsMasquerading.
  • rocketpool/api/node/deposit_cleanup_test.go:
    • Happy-path multi-validator batch deposit (count = 2) across all 5 real keystores + persisted next_account.
    • DepositMulti submission failure with pre-existing active validators: verifies pre-existing keys survive, all failed batch keys are wiped across all 5 keystores, reloaded wallet preserves next_account (5), subsequent retry derives fresh index 5, and RecoverValidatorKey recovers index 5 across the burned index gap (2..4).
    • Table-driven mid-batch failures (getBondRequirement, getDepositData, getValidatorStatus RPC error, status.Exists duplicate pubkey alert, validateDeposit signature failure).
    • submit = false failure leaving persisted wallet next_account untouched while wiping temporary keystore files.
    • Post-broadcast w.Save() failure retaining all live validator keys on disk.
    • Mid-batch panic recovery guard wiping derived keys and saving next_account before re-panicking.
go test -v ./shared/services/wallet/... ./rocketpool/api/node/... ./shared/services ./rocketpool/api/wallet/...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

smartnode should clean up keys from failed deposits

1 participant