Skip to content

*: allow incomplete validator_keys store when parsing - #4576

Merged
KaloyanTanev merged 1 commit into
mainfrom
kalo/allow-incomplete-validator_keys
Jul 6, 2026
Merged

KaloyanTanev merged 1 commit into
mainfrom
kalo/allow-incomplete-validator_keys

Conversation

@KaloyanTanev

Copy link
Copy Markdown
Collaborator

Previously Charon failed commands requiring the partial keys when only a subset of the keys is available in validator_keys directory:

13:54:58.491 ERRO cmd        Application failed to start: match local validator key shares with their counterparty in cluster lock: public key share from provided private key share not found in provided lock
        eth2util/keystore/keystore.go:288 .KeysharesToValidatorPubkey
        cmd/exit_sign.go:126 .runSignPartialExit
        cmd/exit_sign.go:44 .func1
        cmd/cmd.go:111 .func1
        main.go:19 .main

This is actually incorrect as the said key did exist, however, not the full set of keys in the lock were available in the directory. Scenarios in which allowing this is useful:

  1. Operator has Charon in one machine and VC in separate machine;
  2. Operator does not hold the validator keys in the Charon machine;
  3. Operator wants to do some action with Charon's CLI (e.g.: sign exit, sign new deposit, etc.);
  4. Operator moves part of the keys for which the operation will be done inside the Charon machine;
  5. Operator runs the command for the said keys.

category: bug
ticket: none

@KaloyanTanev
KaloyanTanev requested a review from pinebit July 3, 2026 14:00
@KaloyanTanev KaloyanTanev self-assigned this Jul 3, 2026
@sonarqubecloud

sonarqubecloud Bot commented Jul 3, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.21%. Comparing base (3d48812) to head (7991524).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4576      +/-   ##
==========================================
- Coverage   57.22%   57.21%   -0.02%     
==========================================
  Files         245      245              
  Lines       33448    33448              
==========================================
- Hits        19141    19136       -5     
- Misses      11881    11886       +5     
  Partials     2426     2426              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pinebit

pinebit commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

A few findings by Fable for your to consider:

  1. cmd/exit_sign.go:220 — charon exit sign --all now silently signs exits for only the validators whose keystores are on disk. An operator with a partially restored validator_keys dir (e.g.  keystore-0..4 of 10 — contiguous, so SequencedKeys passes) previously got a hard error; now the command succeeds, posts partial exits for 5 validators, and nothing in the output mentions the 5 skipped lock validators — the active_validators/inactive_validators log counts are both computed from the on-disk subset. If enough operators share the gap, those exits never reach threshold. The --all path should compare len(shares) against len(cl.Validators) and at least warn.

  2. eth2util/keystore/keystore.go:285 — the break after the first match makes a second share of the same validator fail with a factually wrong error. With shares = [sA1, sA2] both belonging to validator A (consolidated keystore dirs, or a duplicated keystore file), the inner loop matches sA1 and breaks, so matched for sA2 stays false and the function returns "public key share from provided private key share not found in provided lock" — even though it is in the lock. This input was accepted on main (the extra share was silently ignored), so it's a behavioral regression, and the message misdiagnoses the cause.

  3. eth2util/keystore/load.go:47 — the PR's motivating scenario still fails one layer upstream. All three callers reach the relaxed function via SequencedKeys, which rejects any keystore set whose file indexes aren't exactly 0..N-1. Copying just keystore-5.json (the PR body's use case) dies with "out of sequence keystore index" before keysharesToValidatorPubkey runs — only contiguous-prefix subsets, or renamed files, benefit. The fix likely needs to reach the load layer too, since matching is by derived pubkey and the filename index carries no meaning for these commands.

@KaloyanTanev
KaloyanTanev merged commit 723c99a into main Jul 6, 2026
11 checks passed
@KaloyanTanev
KaloyanTanev deleted the kalo/allow-incomplete-validator_keys branch July 6, 2026 17:42
@KaloyanTanev
KaloyanTanev requested a review from Copilot July 6, 2026 17:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes CLI flows that rely on partial validator_keys by allowing KeysharesToValidatorPubkey to succeed when only a subset of validator keyshares are present, as long as every provided share belongs to a validator in the cluster lock.

Changes:

  • Relaxed KeysharesToValidatorPubkey validation to allow missing validators’ keyshares (subset operation), while still rejecting foreign/unmatched shares.
  • Added targeted unit tests covering subset success and foreign-share failure cases.
  • Updated function documentation to reflect the new (subset-friendly) behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
eth2util/keystore/keystore.go Adjusts keyshare-to-validator mapping validation to support subset key stores and improves error reporting.
eth2util/keystore/keystore_test.go Adds tests ensuring subset keyshares work and foreign shares are rejected.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +289 to +293
// Every provided private key share must belong to a validator in the cluster lock.
for shareIdx, ok := range matched {
if !ok {
return nil, errors.New("public key share from provided private key share not found in provided lock", z.Int("share_index", shareIdx))
}
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.

3 participants