Repository navigation
fix(validator): slashing-protection review follow-ups: attestation index check, exit on a poisoned record - #789
pablodeymo wants to merge 6 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.
There was a problem hiding this comment.
Done in 11bec9b, along the lines you suggest. SharedProtection is now a handle carrying the record and a CancellationToken. The blocking task cancels it itself: when it finds the lock poisoned, and, through a drop guard, when its own operation panics, so it fires even if the caller was dropped at its deadline. run races the duty loop against that signal, so the client stops at once and the once-a-slot poll is gone.
Whatever ends the loop, run then cancels a shutdown token and waits for the keymanager server, which stops accepting and finishes its requests in flight (http_api::serve). The wait is bounded at 60 s, so a request that never ends cannot hold the restart up.
an_import_in_flight_when_the_record_is_poisoned_is_undone_before_the_client_stops runs your scenario over a real socket: the import has saved its key and is waiting to register it when the record is poisoned. It meets the poisoned record, undoes itself and answers before the client is reported stopped. With the wait removed, the test fails.
| )), | ||
| // 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.
There was a problem hiding this comment.
Done in 346b523. undo_import now says which step failed: UndoFailure::Definitions, where nothing was undone, or UndoFailure::Files, naming the keys whose files could not be put back. It no longer stops at the first such key, so the others' files still go back.
The status is built per key from that (unregistered_status), six messages in all. A re-import whose put_back failed now reads "its definitions entry is back as it was, but its keystore and password files could not be put back (…) and may be the ones this request wrote". The (false, false) case you describe reads "its definitions entry is removed, but its keystore and password files could not be (…) and are left behind, unused", and the log line no longer says the next start refuses anything in that case.
One thing changed on the way: an Unflushed definitions save inside the undo was returned as a failure before the files were touched, although the file had already been replaced. It now warns and goes on to the files.
| // 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.
There was a problem hiding this comment.
Done in 36277cb: the skip now also requires the importer's error to say "is at or after its target epoch", on top of the flag and the interchange holding such an attestation. With the importer changed to refuse those four interchanges for another reason, the four cases fail; before, they passed.
| /// 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.
There was a problem hiding this comment.
Right, and the same was true of the two older tests beside it. Done in 36277cb: MockBeaconNode::without_attestation_data_check hands the data back unchecked, and the three service-level tests (wrong slot, wrong target epoch, invalid index) use it, so what refuses is attest's own check. Deleting that line now fails all three, and running it under the wrong fork fails the index one.
| 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.
There was a problem hiding this comment.
Done in 36277cb: unreachable!, naming the function, as the lean arms elsewhere do. highest_attestation_index returns a plain u64 again, and the comment says why it is not an Err.
| 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.
There was a problem hiding this comment.
Done in 36277cb: skipped under root, like a_failed_write_refuses_and_changes_nothing.
| /// 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.
There was a problem hiding this comment.
Fixed in 11bec9b. error.rs and docs/cli.md now say that only attestations and proposals consult the record, so those are what a poisoned one refuses, and ensure_usable is gone with the poll. With the stop now immediate, the other duties no longer run on until the next boundary either.
| /// 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.
There was a problem hiding this comment.
Partly, in 36277cb. The failover tests now derive the fork from mainnet's schedule for the slot they ask about (fork_of), instead of naming one.
I did not add the assert or derive the fork inside. The two implementations that call this are handed the fork with the request and hold no Config to check it against. In attest, the one caller with a schedule, the fork is derived from it for that same slot just before the call, so an assert there would compare a value with itself. The doc now states the precondition, and that attest re-runs the check under the fork it derives. Deriving it here would mean handing HttpBeaconNode the spec, which seems more than the nit warrants; say so if you'd rather have it.
| /// 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.
There was a problem hiding this comment.
Done in 36277cb: bare integers are read too.
| /// 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.
There was a problem hiding this comment.
Removed in 11bec9b rather than shared. The validator crate does not depend on p2p, so sharing it meant finding a common home for one helper, and the message was redundant anyway: the panic hook prints it, with its location, just before. The error now says the panic was this access's own and points there.
The duty loop asked ensure_usable once a slot, so a poisoned record was seen up to a slot late, and run returned with the keymanager server still serving. An import caught between saving its keys and registering them was cut off there, leaving keys on disk with no record, which every later start refuses. SharedProtection is now a handle carrying the record and a signal. The blocking task raises the signal itself: when it finds the lock poisoned, and, through a drop guard, when its own operation panics, so it is raised even if the caller gave up at its deadline. run races the duty loop against it and the once-a-slot poll is gone. Whatever ends the loop, run then tells the keymanager server to stop accepting requests and waits for the ones in flight (http_api::serve, axum's graceful shutdown). The import above meets the poisoned record, takes its keys off disk again and answers. The wait is bounded at 60 s, so a request that never ends cannot hold the restart up. The access whose operation panicked is still told the panic was its own, but no longer repeats the panic's message: the panic hook prints it, with its location, just before. That removes a second copy of the payload downcast. Only attestations and proposals consult the record, so the docs no longer say a poisoned one refuses every duty. tokio-util joins the validator crate for CancellationToken.
When registering an imported key failed and the undo failed too, the answer and the log said the next start would refuse the key, whichever step of the undo had failed. That is true only when the definitions entry is still there. When the entry was removed and only the files stayed, the next start is fine and the files are merely left behind, unused. undo_import now says which step failed: UndoFailure::Definitions, where nothing was undone, or UndoFailure::Files, naming the keys whose files could not be put back. Each key's status is built from that (unregistered_status) and says what is on disk for that key, and the log line warns of a refused start only where one follows. Two changes on the way. The undo no longer stops at the first key whose files cannot be put back, so the others' files still go back. And an Unflushed definitions save inside the undo is no longer returned as a failure before the files are touched: the file was already replaced, so it warns and goes on.
- The interchange vector runner skips a refused case only when the importer's own error says the refusal is for an attestation whose source is at or after its target, on top of the step being flagged slashable and the interchange holding one. A refusal of such an interchange for any other reason now fails the case. - MockBeaconNode::without_attestation_data_check hands attestation data back unchecked, and the three service tests (wrong slot, wrong target epoch, invalid index) use it, so what refuses is attest's own re-check rather than the mock's. Deleting the re-check now fails all three. - validate_attestation_data's lean arm is an unreachable! naming the function, as the lean arms elsewhere are, instead of an error blamed on the beacon node. highest_attestation_index returns a plain u64 again. - validate_attestation_data's doc states that fork must be the fork of slot, and the failover tests derive it from mainnet's schedule instead of naming one. - an_export_needs_no_writable_directory is skipped under root, where a mode 0500 directory refuses nothing. - The vector runner reads epochs written as bare integers as well as strings.
🗒️ 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.The review of this PR is addressed too, and what follows describes the result.
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. (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, and the importer's own error must say that is why it refused; 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.unreachable!naming the function. AnErrthere would be blamed on the beacon node and retried on every other one.forkmust be the fork ofslotand is taken on trust, since the implementations are handed it with the request and hold no schedule. The doc says so, andattestruns the check again under the fork it derives from its ownConfig.SharedProtectionis now a handle carrying the record and a signal, which the blocking task raises when it finds the lock poisoned or its own operation panics.runraces the duty loop against it, so the client stops at once, exits with a failure, and systemd or Kubernetes restarts it. Before, it stayed up, answered/healthand refused every attestation and proposal.runtells its server to stop accepting requests and waits, up to 60 s, for the ones in flight. An import caught between saving its keys and registering them meets the poisoned record and takes its keys off disk again, instead of leaving keys the next start refuses.blockingtells the call whose own operation panicked that the panic was its own. Only later calls getan earlier access panicked.tokio-util, already in the workspace, joins the validator crate forCancellationToken.From the review of this PR, beyond what is folded in above
undo_importreports which step failed: the definitions save, where nothing was undone, or the files, naming the keys whose keystore and password could not be put back. Each key's status and the log line are built from that. A key whose entry was removed and whose files stayed is reported as left behind and unused, where it used to be reported as one the next start refuses. (http_api/keystores.rs)Unflusheddefinitions save inside the undo warns and goes on to the files. It used to be returned as a failure before the files were touched, although the definitions file had already been replaced.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_is_a_bug_in_the_caller;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. It and the two older service tests beside it (wrong slot, wrong target epoch) now run against a mock that hands the data back unchecked (MockBeaconNode::without_attestation_data_check), so what refuses isattest's own re-check. Deleting the re-check fails all three.ethlambda-rpc's wire tests, which drive this client against the node's own Beacon API at fulu and gloas, pass unchanged.the_access_that_panics_is_told_so_and_raises_the_signal,the_access_that_finds_the_record_poisoned_raises_the_signal,the_signal_is_raised_even_when_the_caller_is_gone;run_until_stopped, the part ofrunthat needs no beacon node:an_unusable_record_stops_the_client_once_the_keymanager_has_finished,duties_that_stop_on_their_own_also_wait_for_the_keymanager,a_keymanager_that_never_finishes_does_not_hold_the_client_up,stopping_without_a_keymanager_waits_for_nothing;an_import_in_flight_when_the_record_is_poisoned_is_undone_before_the_client_stops, end to end over a real socket: the import has saved its key and is waiting to register it when the record is poisoned, and it has undone itself and answered before the client is reported stopped. Without the wait for the keymanager the test fails.an_export_needs_no_writable_directory, skipped under root, where a mode0500directory refuses nothing.a_failed_registration_restores_a_replaced_definitions_entrynow asserts the new wording. New:an_undo_names_the_keys_whose_files_it_could_not_put_back,an_undo_that_cannot_save_the_definitions_undoes_nothing,the_status_of_an_unregistered_key_says_what_is_on_disk.duplicate_pubkey_*cases without a word; the new skip reports all 3.single_validator_source_greater_than_target*interchanges for another reason, the four cases fail. Before the error was matched, they passed.make fmt: cleanmake lint: cleanmake test: 2935 passed, 0 failed (1324 intest-consensus, 1611 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