[Merged by Bors] - Minify slashing protection interchange data #2380
Conversation
michaelsproul
added
work-in-progress
michaelsproul
added
ready-for-review
|
I'm marking this as ready for review. I've made some more changes in the last few days, including removing partial imports. The rationale for doing so is in the release notes of the EIP-3076 tests here: https://github.com/eth2-clients/slashing-protection-interchange-tests/releases/tag/v5.1.0-alpha.1 Pending approval from other implementers of those changes, the only change required to this PR will be renaming |
There was a problem hiding this comment.
Looks good, nice work!
Looking at the docs, I notice we don't make a clear statement about whether or not IIFs containing slashable data are Good or Bad. I'm a bit conflicted here; on the one hand it's nice to avoid making a statement about the integrity of some external input (the IIF). But, on the other hand, I'm struggling to think of a scenario where you would want to abort an import because there's slashable data there. Perhaps the double-signing would indicate a corruption in the data?
I don't really have a concrete suggestion and I think what you have is choosing safety from uncertainty, which is sensible. I thought I'd just raise it and see if it inspires anything in you.
I'm happy to merge as is 🙂
paulhauner
added
ready-for-merge
Hmm, you're right that the docs don't say this explicitly. Part of the issue is that there are legitimate cases where the data will fail to import because it appears slashable (e.g. importing the same file twice), which we don't differentiate from the input data being slashable w.r.t. to itself. My hope is that by doing minification by default we'll provide safe and error-free behaviour for 99% of users, and the ones that run into oddities will either wade through the intricacies themselves or contact us on Discord so we can help them.
I think if you're not expecting there to be slashable data, and you opt into I don't have any concrete ideas for improving the docs right now, so I think we merge and see how this is understood. bors r+ |
## Issue Addressed Closes #2354 ## Proposed Changes Add a `minify` method to `slashing_protection::Interchange` that keeps only the maximum-epoch attestation and maximum-slot block for each validator. Specifically, `minify` constructs "synthetic" attestations (with no `signing_root`) containing the maximum source epoch _and_ the maximum target epoch from the input. This is equivalent to the `minify_synth` algorithm that I've formally verified in this repository: https://github.com/michaelsproul/slashing-proofs ## Additional Info Includes the JSON loading optimisation from #2347
|
Pull request successfully merged into unstable. Build succeeded: |
bors
bot
changed the title
## 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).
## 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).
Issue Addressed
Closes #2354
Proposed Changes
Add a
minifymethod toslashing_protection::Interchangethat keeps only the maximum-epoch attestation and maximum-slot block for each validator. Specifically,minifyconstructs "synthetic" attestations (with nosigning_root) containing the maximum source epoch and the maximum target epoch from the input. This is equivalent to theminify_synthalgorithm that I've formally verified in this repository:https://github.com/michaelsproul/slashing-proofs
Additional Info
Includes the JSON loading optimisation from #2347