Validation bugfix #2089
Merged
Validation bugfix #2089
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
jbearer
reviewed
Oct 1, 2024
jbearer
reviewed
Oct 1, 2024
jbearer
reviewed
Oct 1, 2024
tbro
requested review from
nomaxg,
philippecamacho,
ImJeremyHe,
sveitser and
imabdulbasit
as code owners
jbearer
approved these changes
Oct 1, 2024
sveitser
reviewed
Oct 2, 2024
sveitser
reviewed
Oct 2, 2024
types/src/v0/impls/state.rs
Outdated
| @@ -663,6 +695,41 @@ impl HotShotState<SeqTypes> for ValidatedState { | |||
| .resolve() | |||
| .expect("Chain Config not found in validated state"); | |||
|
|
|||
| // Validate l1_finalized. | |||
| let Some(proposed_finalized) = proposed_header.l1_finalized() else { | |||
| tracing::error!("L1 finalized has None value."); | |||
There was a problem hiding this comment.
Does this mean the proposer proposed an invalid block? I think in that case this should be warn at most? We should only log error if this is something unexpected and/or it's worth waking people up for.
There was a problem hiding this comment.
appears to have been addressed here: 8f0d229
sveitser
reviewed
Oct 2, 2024
sveitser
reviewed
Oct 2, 2024
types/src/v0/impls/state.rs
Outdated
| return Err(BlockError::InvalidBlockHeader); | ||
| }; | ||
|
|
||
| let parent_finalized = parent_leaf.block_header().l1_finalized().unwrap().number(); |
There was a problem hiding this comment.
Since we're not adding tests now please open an issue for that (if we don't have it already) and link it in the code.
validate l1_head is incrementing
also avoid hitting timestamp validation in previously existing test (for the time being).
Also allow equality
* fix timestamp validation * fix system_time field in Error variant
Use this method in validation and in `wait_for_l1_finalized`.
This way we can tell which one failed
Co-authored-by: Mathis <sveitser@gmail.com>
tbro
force-pushed
the
tb/fix/validation
branch
from
adb3cd3
to
0f4ebc0
Compare
This was referenced Oct 2, 2024
tbro
added a commit
that referenced
this pull request
Oct 2, 2024
Adds missing validation steps * validate timestamp (against system time and previous block) * wait for l1_finalized * wait for l1_head * validate blocks are not decrementing * small update to pee-existing tests to avoid hitting these cases * adds TODOs for followups --------- Co-authored-by: tbro <tbro@users.noreply.github.com> Co-authored-by: Jeb Bearer <jeb@espressosys.com> Co-authored-by: Mathis <sveitser@gmail.com>
rob-maron
reviewed
Oct 3, 2024
Comment on lines
+729
to
+734
|
|
||
| let _ = instance | ||
| .l1_client | ||
| .wait_for_block(proposed_header.l1_head()) | ||
| .await; | ||
|
|
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.
Closes #2088
Closes #968
This PR:
Adds validation to replicas.