chore: add consensus-key guard when processing deposits

[matthias-wright]

Builds on #377 (which builds on #276 and #275).

Addresses #339.

Changes:

• Adds consensus-key guard when processing deposits.
  Note: This guard is purely defensive,
  • Parse already enforces the match. A top-up is only accepted into the queue if its consensus key equals the stored account's. For a new-validator deposit, the placeholder's consensus key is set from the deposit, so they match trivially.
  • The account can't change identity before consume. While the deposit is queued, has_pending_deposit = true, which blocks the account from being removed/replaced. So the account that's looked up at consume time is the same account, with the same consensus key, that was validated at parse.
• Stake-bound enforcement is one-shot (runs only at the boundary where a stake param changes) and fix: skip validators with pending deposits for stake-bound balance checks #188 skips any account with a pending deposit there. So a validator with a too-small (or never-processed, e.g. low/zero deposit cap) top-up queued across a MinimumStake increase was skipped and never re-checke. It stayed active below the new
  minimum indefinitely. This is fixed by:
  • At both the penultimate staging and the boundary scan, defer removal for a has_pending_deposit account only if its queued deposit could bring it back into [min, max]; otherwise enforce now. Zero-balance new-validator placeholders
    stay excluded.
  • Add regression test test_pending_topup_does_not_evade_minimum_stake_increase.

[sebastian-osec]

I think this is a good partial fix for #339.

The stale queued-deposit lifecycle can still reach a no-account path: a top-up can be queued while max_deposits_per_epoch = 0, the validator can be force-removed and deleted, and then the queued top-up can be popped later with no corresponding account.

In that case this branch only logs and skips the deposit:

let Some(mut account) = state.get_account(&node_pubkey_bytes).cloned() else {
    warn!("Deposit request has no corresponding account, skipping: {request:?}");
    continue;
};

That means the queued top-up is not processed against its original account lifecycle and is not refunded either. I think the remaining invariant should be that a queued deposit must either be processed against its original account lifecycle or refunded.

[matthias-wright]

Update(f3cd812):

• Refund queued deposits that outlived their account

Update(5112dc7):

• Add regression test to cover refund of queued deposits that outlived their account.

[sebastian-osec]

Thanks, the new commits fix the no-account queued-deposit path I commented on.

One remaining edge case: if a later replacement deposit recreates an inactive placeholder for the same node key and same consensus key but different withdrawal credentials before the stale old queued deposit is popped, FIFO still pops the old deposit first, but the replacement placeholder already exists. The no-account branch does not fire, and the new consensus-key guard passes.

The stale deposit can then be processed against the replacement account and use account.withdrawal_credentials from the replacement placeholder rather than the stale deposit’s own request.withdrawal_credentials.

So the no-account issue looks fixed, but I think the consume-time binding should also prove the current account belongs to the same account lifecycle that accepted the queued deposit, for example by checking withdrawal credentials / expected deposit index, or otherwise refund the queued deposit to request.withdrawal_credentials.

[matthias-wright]

Update(81e233d):

• Refund deposits to the deposit's own withdrawal credentials

[sebastian-osec]

Thanks, I tested latest 81e233d locally.

The refund-destination fix works for the refund path: if the stale queued deposit is rejected/refunded, it now refunds to request.withdrawal_credentials.

However, I think the successful-processing path is still not bound to the original account lifecycle. I modified the new same-key replacement regression test so the stale deposit amount is in range instead of below minimum. In that case the stale deposit does not enter the refund path. The current account exists, the consensus key matches, and the deposit is accepted as a valid new-validator deposit against the replacement placeholder.

The test passed while asserting that the replacement account was credited with the stale deposit amount and retained the replacement withdrawal credentials.

So 81e233d fixes wrong refund destination, but it does not prevent an in-range stale queued deposit from being successfully applied to a same-node-key/same-consensus-key replacement account. I think consume-time processing still needs to bind to the same account lifecycle that accepted the queued deposit, for example by checking withdrawal credentials / expected deposit index / explicit account generation, or refunding when that binding fails.

[matthias-wright]

I'm working on a refactor of the deposit and withdrawal processing (out of scope for this audit) that fixes #339, so I will close this PR.