Repository navigation
fix(validator): slashing-protection review follow-ups: attestation index check, exit on a poisoned record - #789
pablodeymo wants to merge 3 commits into
Conversation
) - A re-import whose registration failed and was fully undone now answers "not imported: ...; its keystore and definitions entry are back as they were". "keystore rewritten" stopped being true once the undo put the files back too, and is kept for the case where the undo itself failed. - The interchange vector runner ends a case only when the refused step holds an attestation whose source is at or past its target outside genesis, the one shape this importer refuses, rather than on any step flagged as slashable. Every other slashable step must now import. - an_import_carrying_history_is_refused_whole is renamed for what it has tested since the importer landed, history that does not parse, and asserts that this is the reason. - The Interchange types' doc says they are shared by the record reader and the importer, both strict. - docs/cli.md no longer says an export needs a writable directory: the lock is on the directory itself and an export writes nothing there. A test pins it on a mode 0500 directory.
validate_attestation_data checked the slot, the target epoch and the source, and passed data.index through unchecked. It now takes the slot's fork and refuses any index but 0 before gloas and any above 1 from gloas, where the index is the payload signal. Those are gossip's own bounds (attestation.data.index == 0 from electra, < 2 from gloas), and Lodestar's validator client refuses the same before signing. It is a liveness check rather than a slashing one. A vote whose index cannot be valid is rejected by every peer, after slashing protection has recorded its target, so the validator has no second vote to cast in that epoch. Refused in the beacon-node implementation, the answer is an Err that failover moves past, and nothing is recorded for it. Before electra the index is a committee, and this client only ever asks about committee 0. At gloas only the range is checked, since which of 0 and 1 is right depends on the node's view of the block. Every fork is a named match arm, so a fork after gloas has to say what its index means.
Once a panic poisoned the record's lock, every duty was refused until a restart, but the process stayed up and answered /health, so nothing that supervises it had a reason to restart it. The duty loop now asks slashing_protection::ensure_usable at each slot boundary and returns the error from run on a poisoned lock: the process exits with a failure, and systemd or Kubernetes applies the remedy the refusals ask for. The restart rereads the record from disk, which every write either replaced whole or left alone. The access whose own operation panicked was told "an earlier access panicked", which points at an event that never happened. It is now told it was its own panic, with the panic's message; only later accesses get the old wording. docs/cli.md says to run the client under a supervisor.
🤖 Codex Code ReviewNo actionable findings in the diff. The fork-specific attestation index validation runs before recording or signing, preserves beacon-node failover, and correctly allows Gloas indices 0 and 1. The poisoned-lock check propagates a fatal error for supervisor restart. Could not run validator tests: Rustup attempted to write to the read-only Automated review by OpenAI Codex · gpt-6.1-sol · custom prompt |
🤖 Claude Code ReviewReview of PR 789Overall this is a solid PR. The index check closes a real liveness gap: a bad vote gets recorded by slashing protection and then rejected by every peer. The poisoned-lock exit gives supervisors a signal they can act on. The interchange-vector runner is now stricter in a way that catches regressions it used to hide. A few points are worth addressing. CorrectnessItem 1: Pre-Electra
Item 2:
Item 3:
Item 4: Keystore import message claims more than the old one did (
Minor
Looks good
Automated review by Claude · claude-opus-5-5 · custom prompt |
🤖 Gemini Code ReviewThe changes look solid and well-tested:
LGTM! Automated review by Gemini · gemini-4-argon · custom prompt |
🤖 Kimi Code ReviewNo blocking issues found in the provided diff.
Optional coverage improvement: the tests exercise Automated review by Kimi (Moonshot AI) · kimi-k3.1-pro · custom prompt |
🤖 Grok Code ReviewThe attestation-index gate and the poisoned-record exit are the right safety direction. A few spots are weaker than the comments claim.
Automated review by Grok (xAI) · grok-4.7 · custom prompt |
🤖 DeepSeek Code ReviewOverall this looks solid. The attestation A few minor points:
No consensus-layer correctness issues found in this diff; the changes are validator-client validation and slashing-protection lifecycle improvements. Automated review by DeepSeek · deepseek-v4-pro · custom prompt |
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
Nothing blocking. The exit path in lib.rs is the one I'd fix before merging; the rest are smaller.
| // Kubernetes the restart the refusals ask for. Asked once a slot, at | ||
| // the boundary, which is soon enough: in between, the record refuses | ||
| // every duty on its own. | ||
| if let Err(err) = slashing_protection::ensure_usable(&protection) { |
There was a problem hiding this comment.
Returning here drops the runtime, and with it any keymanager request in flight. An import that has already written its keystore files and enabled definitions entry is then parked on slashing_protection::blocking(...).await (always a spawn_blocking, so a real yield) and is dropped there, so undo_import never runs. The next start finds a loaded key with no record and refuses the whole client, and a supervisor restarts it into the same refusal every time.
The window is narrow (a poisoned record plus an import in flight at the boundary), and a crash in the same spot already leaves this state by design. But this exit is deliberate, so it can be graceful: signal shutdown from where the poisoning is observed (e.g. a CancellationToken cancelled in blocking on the panic) and let the keymanager finish or undo before run returns. That would also stop the client at once rather than up to a slot later.
| )), | ||
| // The undo failed as well, so what this request wrote | ||
| // over the key's files and entry may still be there. | ||
| (true, false) => KeyStatus::error(format!( |
There was a problem hiding this comment.
undo_import saves the definitions first and only then restores the keystore and password files, so rolled_back being Err doesn't mean the entry stayed. If a put_back failed, the entry is back, and it's the keystore and password that are still the new ones, which this message doesn't say. Same for the (false, false) arm: if the entry was removed but the files weren't, they're orphans and nothing is refused at the next start. Having undo_import report which step failed would let both messages say what's actually on disk.
| // say, a duplicated pubkey or a surround, with that case's blocks | ||
| // and attestations never attempted. | ||
| (false, true) | ||
| if step.contains_slashable_data && holds_impossible_source(&step.interchange) => |
There was a problem hiding this comment.
This forgives any refusal of an interchange that happens to contain an impossible source, not a refusal for it. If the importer started refusing something else (a duplicated pubkey, a surround) in one of the single_validator_source_greater_than_target* cases, the case would still return Ok with its blocks and attestations never tried, which is the masking this change sets out to remove. Matching the error as well (it says "is at or after its target epoch", slashing_protection/file.rs:178) would close it.
| /// still signed. `context()` is mainnet's schedule, where slot 96 is long | ||
| /// before gloas, so 1 is not an index a vote there can carry. | ||
| #[tokio::test] | ||
| async fn an_index_invalid_at_the_slots_fork_is_refused_and_records_nothing() { |
There was a problem hiding this comment.
This error comes from MockBeaconNode::attestation_data, which runs validate_attestation_data itself (mock.rs:602), not from the re-check in attest at line 138. Deleting that line, or passing it the wrong fork, keeps every test green. Covering it needs a BeaconNodeApi impl that hands the data back unchecked.
| ForkName::Gloas => Ok(1), | ||
| // Not a Beacon Chain fork: `Config::fork_at_epoch` never names it, so | ||
| // a caller that does is wrong, and nothing is signed for it. | ||
| ForkName::Lean => Err(crate::error::Error::InconsistentResponse( |
There was a problem hiding this comment.
Can't happen today, but if it did: this fires after the HTTP request, as InconsistentResponse, which is retryable and blamed on the node, so failover tries every node and reports them all as failed. A local bug would read as a beacon-node outage every slot. Checking the fork before the request, or unreachable! here, would report it as what it is.
| propose(&mut protection, 1, 97).expect("propose"); | ||
| drop(protection); | ||
|
|
||
| let read_only = std::fs::Permissions::from_mode(0o500); |
There was a problem hiding this comment.
Missing the running_as_root() skip that a_failed_write_refuses_and_changes_nothing uses (line 1211). Under root the 0o500 mode restricts nothing, so in a root container this passes even if export starts writing to the directory.
| /// restart. A poisoned lock is a bug, never a disk, and it stays poisoned | ||
| /// for the life of the process, so every duty is refused until it is | ||
| /// restarted. Never retryable. | ||
| /// for the life of the process, so every duty is refused from then on |
There was a problem hiding this comment.
"Every duty is refused" is broader than what happens: only attestations and proposals consult the record. Sync committee messages and contributions, aggregates and PTC votes keep signing until the next boundary, then stop with the process. Same wording in ensure_usable and docs/cli.md.
| /// it, and the `Err` is one failover moves past to a node that may answer | ||
| /// properly. Lodestar's validator client refuses the same before signing | ||
| /// (`validateAttestationDuty`). | ||
| pub fn validate_attestation_data(slot: Slot, fork: ForkName, data: &AttestationData) -> Result<()> { |
There was a problem hiding this comment.
Nit: fork comes from the caller and is never checked against slot, so the index bound is only as right as the caller's fork. Every call site derives it correctly today, but the new failover test already passes ForkName::Electra for slot 96, which is phase0 on mainnet. A debug_assert against the schedule, or deriving the fork here, would remove the footgun.
| /// forgives along with it. | ||
| fn holds_impossible_source(interchange: &serde_json::Value) -> bool { | ||
| let epoch = | ||
| |value: &serde_json::Value| value.as_str().and_then(|text| text.parse::<u64>().ok()); |
There was a problem hiding this comment.
Nit: only quoted epochs are read here, while the importer also takes bare integers (quoted_or_bare). The vectors quote them per EIP-3076, so this is theoretical, but the restated rule covers less than the one it mirrors.
| /// A panic's message, when it carried one: `panic!` with a literal gives a | ||
| /// `&str` and with arguments a `String`. Anything else was raised with | ||
| /// `panic_any` and has no text to show. | ||
| fn panic_message(panic: &(dyn std::any::Any + Send)) -> &str { |
There was a problem hiding this comment.
Nit: the same downcast as guarded in crates/net/p2p/src/beacon/verdict.rs (&str, then String, then a fallback), with a different fallback string.
🗒️ Description / Motivation
#734 (durable slashing protection) and #736 (EIP-3076 import and export) were approved with a last round of review comments still open, and with two follow-ups set aside for a separate PR. This is that PR. It changes nothing about the slashing rule or the record's format.
Seven of the eight open points are addressed here. The eighth,
ethlambda-rpc's dev-dependency onethlambda-validator, is left alone and explained at the end.What Changed
From the last round on #734
not imported: its slashing-protection record could not be written: …; its keystore and definitions entry are back as they were. The old wording is kept for the case where the undo itself failed. (http_api/keystores.rs)From the last round on #736
contains_slashable_datawas refused. Now the step's interchange must also hold an attestation whose source is at or past its target outside genesis; every other slashable step has to import. At v5.3.0, 21 steps are flagged slashable and expected to import, and 4 of them hold such an attestation (thesingle_validator_source_greater_than_target*cases). The predicate is restated in the test rather than borrowed from the crate, so the importer widening what it refuses cannot widen what the runner forgives. (tests/interchange_vectors.rs)an_import_carrying_history_is_refused_wholeis renamed toan_import_whose_history_does_not_parse_is_refused_whole, with its doc, and now asserts that the refusal is for the history's shape (review).Interchangedoc says what the types are for (comment): shared by the record reader and the importer, strict in both, as Lighthouse's are. Behaviour unchanged. (slashing_protection/file.rs)exportno longer claims to need a writable directory (comment). The lock is anflockon the directory itself and an export writes nothing there. A test pins it on a mode0500directory. (docs/cli.md)The two follow-ups #734 set aside
indexcannot be valid for its fork is refused (review).validate_attestation_datanow takes the slot's fork and refuses any index but 0 before gloas, and any above 1 from gloas, where it is the payload signal. Those are gossip's own bounds (attestation.data.index == 0from electra,< 2from gloas). Lodestar's validator client refuses the same before signing (validateAttestationDuty); Teku enforces them on its beacon-node side (validateCommitteeIndexValue).Errthat failover moves past, and nothing is recorded.slashing_protection::ensure_usableat each slot boundary and returns the error fromrunon a poisoned lock, so the process exits with a failure and systemd or Kubernetes restarts it. Before, it stayed up, answered/healthand refused every duty.blockingnow tells the call whose own operation panickedthis access panicked while holding the record (<panic message>). Only later calls getan earlier access panicked.Docs:
docs/cli.mdgets the export wording, the index check in the attestation walkthrough, and a note to run the client under a supervisor.Correctness / Behavior Guarantees
indexoutside what the slot's fork allows is now failed over from, where its answer used to be signed. If every configured node answers that way, the slot's attestations are not made and nothing is recorded for them.Tests Added / Run
a_nonzero_index_is_refused_before_gloas(every fork before gloas),a_gloas_index_is_zero_or_one,the_lean_fork_has_no_valid_index;a_node_answering_with_an_index_invalid_at_the_fork_is_failed_over_from;an_index_invalid_at_the_slots_fork_is_refused_and_records_nothing: the service still signs a correct answer for the same slot afterwards.ethlambda-rpc's wire tests, which drive this client against the node's own Beacon API at fulu and gloas, pass unchanged.a_poisoned_record_stops_the_client,the_access_that_panics_is_told_so_and_later_ones_that_an_earlier_one_did,a_panics_message_is_read_whatever_it_was_raised_with. The four lines inrunthat act onensure_usablehave no test of their own, sincerunneeds a live beacon node.an_export_needs_no_writable_directory.a_failed_registration_restores_a_replaced_definitions_entrynow asserts the new wording.duplicate_pubkey_*cases without a word; the new skip reports all 3.make fmt: cleanmake lint: cleanmake test: 2927 passed, 0 failed (1324 intest-consensus, 1603 intest-node)Related Issues / PRs
Not changed:
ethlambda-rpc's dev-dependency onethlambda-validator(comment). The dependency predates #734 (it came with #661); #734 only added thetest-utilsfeature to it. It exists forrpc/src/beacon/validator_client_tests.rs, about 1,600 lines that drive the validator client's own HTTP client against this node's Beacon API over a socket, to check the two ends agree on the wire. Removing the dependency means moving that file to a crate that depends on both, and it reaches intoethlambda-rpcinternals in about ten places (build_beacon_api_router, thetest_utilsmodule,FuluSignedBlockContents;proposal.rs's tests also borrow a helper back from it), so those would have to become a public test surface. That is a PR of its own, and it is worth agreeing where those tests should live before moving them.✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(test-consensusplustest-node, atrelease-fast) — all passing