[Merged by Bors] - Make slashing protection import more resilient #2598
Conversation
michaelsproul
added
ready-for-review
michaelsproul
added
work-in-progress
The existing test generator covers a lot of ground, particularly with |
I would like to see my scenario tested. I'll try to look at test code and see if it's covered. If not I'll try to create a test. Wish me luck, I'll need it :) |
I've added a test lighthouse/validator_client/slashing_protection/src/bin/test_generator.rs Lines 326 to 360 in 5891afd These tests get published from Lighthouse to a repo used by all clients as test vectors for EIP-3076: https://github.com/eth2-clients/slashing-protection-interchange-tests. I'll do a new release of the tests once this branch is merged. |
michaelsproul
added
ready-for-review
michaelsproul
force-pushed
the
slashing-protection-import
branch
from
f448824 to
76b8548
Compare
There was a problem hiding this comment.
Nice one! Reads really well.
I had a very minor comment and two points:
- It seems slightly odd to me that even though we declare minification redundant and deprecated, we'll still perform it if the flag is supplied. Perhaps this is just a defensive measure?
- There's some duplication of minification code, but it doesn't seem outrageous to any degree. I had a look at how to unify it, and it might end up making things ultimately more complex. I just thought I'd mention it in case you had any ideas.
I'll give this an approval, since I think it's personally fine to merge as-is. Feel free to enact/ignore my comments as you see fit :)
| // Summary of minimum and maximum messages pre-import. | ||
| let prev_summary = self.validator_summary(pubkey, txn)?; | ||
|
|
||
| // If the interchange contains a new maximum slot block, import it. |
There was a problem hiding this comment.
It's very minor, but we don't necessarily import this as per the comment.
There was a problem hiding this comment.
Because of the relevance check? I think the "new maximum slot block" phrasing implies we only import the block if its slot is greater than all known blocks, which is checked below where it says
Block is relevant if there are no previous blocks, or new block has slot greater than
previous maximum.
There was a problem hiding this comment.
I'm open to suggestions regarding wording, however
paulhauner
added
waiting-on-author
|
Thank you for the review! 🙏
I kept it intentionally separate so that the two implementations could be tested against each other, which was quite useful.
Minifying and importing should be harmless, it just wastes time in the worst case. I figured we may as well keep the functionality for flexibility. And you're right, defensively, |
michaelsproul
added
ready-for-merge
|
bors r+ |
## Issue Addressed Closes #2419 ## Proposed Changes Address a long-standing issue with the import of slashing protection data where the import would fail due to the data appearing slashable w.r.t the existing database. Importing is now idempotent, and will have no issues importing data that has been handed back and forth between different validator clients, or different implementations. The implementation works by updating the high and low watermarks if they need updating, and not attempting to check if the input is slashable w.r.t itself or the database. This is a strengthening of the minification that we started to do by default since #2380, and what Teku has been doing since the beginning. ## Additional Info The only feature we lose by doing this is the ability to do non-minified imports of clock drifted messages (cf. Prysm on Medalla). In theory, with the previous implementation we could import all the messages in case of clock drift and be aware of the "gap" between the real present time and the messages signed in the far future. _However_ for attestations this is close to useless, as the source epoch will advance as soon as justification occurs, which will require us to make slashable attestations with respect to our bogus attestation(s). E.g. if I sign an attestation 100=>200 when the current epoch is 101, then I won't be able to vote in any epochs prior to 101 becoming justified because 101=>102, 101=>103, etc are all surrounded by 100=>200. Seeing as signing attestations gets blocked almost immediately in this case regardless of our import behaviour, there's no point trying to handle it. For blocks the situation is more hopeful due to the lack of surrounds, but losing block proposals from validators who by definition can't attest doesn't seem like an issue (the other block proposers can pick up the slack).
|
Build failed: |
|
Looks like a spurious ganache failure bors retry |
## Issue Addressed Closes #2419 ## Proposed Changes Address a long-standing issue with the import of slashing protection data where the import would fail due to the data appearing slashable w.r.t the existing database. Importing is now idempotent, and will have no issues importing data that has been handed back and forth between different validator clients, or different implementations. The implementation works by updating the high and low watermarks if they need updating, and not attempting to check if the input is slashable w.r.t itself or the database. This is a strengthening of the minification that we started to do by default since #2380, and what Teku has been doing since the beginning. ## Additional Info The only feature we lose by doing this is the ability to do non-minified imports of clock drifted messages (cf. Prysm on Medalla). In theory, with the previous implementation we could import all the messages in case of clock drift and be aware of the "gap" between the real present time and the messages signed in the far future. _However_ for attestations this is close to useless, as the source epoch will advance as soon as justification occurs, which will require us to make slashable attestations with respect to our bogus attestation(s). E.g. if I sign an attestation 100=>200 when the current epoch is 101, then I won't be able to vote in any epochs prior to 101 becoming justified because 101=>102, 101=>103, etc are all surrounded by 100=>200. Seeing as signing attestations gets blocked almost immediately in this case regardless of our import behaviour, there's no point trying to handle it. For blocks the situation is more hopeful due to the lack of surrounds, but losing block proposals from validators who by definition can't attest doesn't seem like an issue (the other block proposers can pick up the slack).
|
Pull request successfully merged into unstable. Build succeeded: |
bors
bot
changed the title
## Issue Addressed Part of #2557 ## Proposed Changes Refactor the slashing protection export so that it can export data for a subset of validators. This is the last remaining building block required for supporting the standard validator API (which I'll start to build atop this branch) ## Additional Info Built on and requires #2598
Issue Addressed
Closes #2419
Proposed Changes
Address a long-standing issue with the import of slashing protection data where the import would fail due to the data appearing slashable w.r.t the existing database. Importing is now idempotent, and will have no issues importing data that has been handed back and forth between different validator clients, or different implementations.
The implementation works by updating the high and low watermarks if they need updating, and not attempting to check if the input is slashable w.r.t itself or the database. This is a strengthening of the minification that we started to do by default since #2380, and what Teku has been doing since the beginning.
Additional Info
The only feature we lose by doing this is the ability to do non-minified imports of clock drifted messages (cf. Prysm on Medalla). In theory, with the previous implementation we could import all the messages in case of clock drift and be aware of the "gap" between the real present time and the messages signed in the far future. However for attestations this is close to useless, as the source epoch will advance as soon as justification occurs, which will require us to make slashable attestations with respect to our bogus attestation(s). E.g. if I sign an attestation 100=>200 when the current epoch is 101, then I won't be able to vote in any epochs prior to 101 becoming justified because 101=>102, 101=>103, etc are all surrounded by 100=>200. Seeing as signing attestations gets blocked almost immediately in this case regardless of our import behaviour, there's no point trying to handle it. For blocks the situation is more hopeful due to the lack of surrounds, but losing block proposals from validators who by definition can't attest doesn't seem like an issue (the other block proposers can pick up the slack).