Message validation #1066
Merged
Message validation #1066
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lior-blox
reviewed
Jul 18, 2023
lior-blox
reviewed
Jul 18, 2023
lior-blox
reviewed
Jul 18, 2023
lior-blox
reviewed
Jul 18, 2023
lior-blox
reviewed
Jul 18, 2023
nkryuchkov
commented
Jul 20, 2023
network/p2p/GENERAL_RULES.md
Outdated
| - `Ignore` - message is invalid, ignored and not processed further, violation count is increased. | ||
| - `Reject` - message is rejected, ignored and not processed further, violation count is set to violation threshold, peer is banned for the current round. | ||
|
|
||
| ## Semantic assertions |
There was a problem hiding this comment.
add a case when message for slot N is sent in multiple slots
network/p2p/GENERAL_RULES.md
Outdated
| - Generally, If violation happens by a large margin, the message is rejected. | ||
| - If an assertion has a rule for rejecting, the rule has higher priority than this one. | ||
| - If violation count reaches a certain threshold, all further messages from the validator for current round are rejected. | ||
| - Each round resets violation count to 0. |
There was a problem hiding this comment.
Handle case if node is a bit malicious (ignored) each round but does it repetitively: should it be punished?
nkryuchkov
commented
Jul 20, 2023
network/p2p/GENERAL_RULES.md
Outdated
| - TODO: Need to check if upon reorg there can be more than one duty per epoch. | ||
| - First violation is ignored. Further ones are rejected. | ||
| - Message round is equal to estimated current round. | ||
| - Violation by [1; ceil(MAX_POSSIBLE_ROUND * 0.2)] rounds is ignored. Violations above that are rejected. |
There was a problem hiding this comment.
- future rounds should be punished
- earlier ones are to be discussed
- test if clock drift may cause any false positives (if so, consider checking Prysm code regarding clock drift)
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
lior-blox
reviewed
Aug 1, 2023
moshe-blox
previously approved these changes
Sep 28, 2023
MatheusFranco99
previously approved these changes
Sep 28, 2023
y0sher
reviewed
Sep 28, 2023
nkryuchkov
dismissed stale reviews from MatheusFranco99 and moshe-blox
via
af3124f
y0sher
reviewed
Sep 29, 2023
y0sher
reviewed
Oct 1, 2023
y0sher
reviewed
Oct 1, 2023
y0sher
reviewed
Oct 1, 2023
y0sher
reviewed
Oct 1, 2023
y0sher
reviewed
Oct 1, 2023
y0sher
previously approved these changes
Oct 1, 2023
There was a problem hiding this comment.
Added just some minor comments. This is a lot of code, lets get it merged soon.
Generally I'm not a big fan of this approach, we're replicating the consensus state in a way and keeping the messages twice. the code is pretty well organized though and functions as a "separate entity" which is good because one of my concerns was bloating the p2p layer with consensus logic.
anyway I hope one day we would just relay on the protocol to validate its own messages since it should already have all tools in place. there are some extra stuff we validate by tracking the messages but IMO eventually it should be splitted from consensus logic.
y0sher
previously approved these changes
Oct 2, 2023
MatheusFranco99
previously approved these changes
Oct 3, 2023
moshe-blox
previously approved these changes
Oct 3, 2023
nkryuchkov
dismissed stale reviews from moshe-blox, MatheusFranco99, and y0sher
via
81b2763
MatheusFranco99
approved these changes
Oct 3, 2023
y0sher
approved these changes
Oct 3, 2023
y0sher
added a commit
that referenced
this pull request
Oct 15, 2023
* Message validation (#1066) --------- Co-authored-by: moshe-blox <moshe@blox.io> Co-authored-by: moshe-blox <89339422+moshe-blox@users.noreply.github.com> * e2e tests for eth execution layer package (#1143) * New slot ticker (#1149) --------- Co-authored-by: Matus Kysel <matus@Matuss-MacBook-Air.local> Co-authored-by: moshe-blox <moshe@blox.io> --------- Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com> Co-authored-by: moshe-blox <moshe@blox.io> Co-authored-by: moshe-blox <89339422+moshe-blox@users.noreply.github.com> Co-authored-by: Anton Korpusenko <antokorp@gmail.com> Co-authored-by: Matus Kysel <MatusKysel@users.noreply.github.com> Co-authored-by: Matus Kysel <matus@Matuss-MacBook-Air.local>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.