Replica Validation Refactor #2170

tbro · 2024-10-11T22:15:50Z

Closes #2090

This PR:

Refactors contents of validate_and_apply_header by introducing two new internal (non-pub) types. This should facilitate:

comprehension
documentation
testing

Basically this PR:

adds Proposal and ValidatedTransition types
moves validation steps to isolated methods.
adds doc comments to methods
updates error enum
adds many tests

Review

Most important changes are isolated ./types/src/v0/impls/state.rs. Many unit tests have been added there. Tests should pass and you should be convinced that there is sufficient coverage. Small changes were made to ./types/src/v0/impls/header.rs where we previously had testing for this validation in the form of high level success cases and eror cases tests. The success case test has been renamed and the error case test removed in favor of the more comprehensive error case tests in the new unit tests.

Review Documentation

You can run just doc and navigate here in your browser:
./target/doc/espresso_types/v0/impls/state/struct.ValidatedTransition.html

zacshowa

I know this is a WIP, but I left some general discussion points from my first read. I think that this will be a great use of the type system to make validation more readable.

types/src/v0/impls/state.rs

zacshowa · 2024-10-14T18:45:55Z

types/src/v0/impls/state.rs

+        // TODO 2 seconds of tolerance should be enough for reasonably
+        // configured nodes, but we should make this configurable.
+        if self.proposal.header.timestamp().abs_diff(system_time) > 2 {


Does it make sense for this value to live in the chain config? Given we have the chain config here, implementing this be low effort.

Requires some thought. Putting it in chain config means we require an upgrade to modify it. Maybe we can just pass it in on the command line.

Ah, I wasn't aware the chain config was immutable between upgrades. What's the reasoning for that?

It's meant to be immutable configuration for the block_chain. Part of blockchain the safety concerns I think.

types/src/v0/impls/state.rs

sveitser · 2024-10-24T07:51:16Z

types/src/v0/impls/header.rs

-            proposal_state.block_merkle_tree.commitment(),
-            proposal.block_merkle_tree_root()
-        );
+        // ValidatedTransition::new(


Is this intentionally left commented out?

Humm I thought this was previously commented in relation to the above TODO... Maybe I got confused will double check.

sveitser · 2024-10-24T08:10:16Z

types/src/v0/impls/state.rs

+}
+/// Type to hold cloned validated state and provide validation methods.
+#[derive(Debug)]
+pub(crate) struct ValidatedTransition<'a> {


A bit of a nitpick: the transition is only validated if validate() does not return an error but given current naming one might expect it to be validated upon creation unless one actually inspects the return type. I'm totally fine with the current name just thinking maybe we can make it even clearer at some point.

Its a tough one. It's a type that only exists to facilitate a state transition by encapsulating validation logic. The state is actually held in ValidatedState. Open to ideas, but I don't have any at the time being.

sveitser · 2024-10-24T08:12:05Z

types/src/v0/impls/state.rs

-        ..
-    } = state;
+        // Validate timestamp hasn't drifted too much from system time.
+        // Do this check first so we don't add unnecessary drift.


Nit: comment is now potentially misleading because in this function the drift check is done last.

I think we can move the comment about this check being time sensitive up to the doc string of validate_timestamp.

sveitser · 2024-10-24T10:00:39Z

types/src/v0/impls/state.rs

+#[derive(Debug)]
+pub(crate) struct Proposal<'a> {
+    header: &'a Header,
+    block_size: u32,


There is also a Proposal type in hotshot that we use in this repo, so I think we can use a different name here. ProposalValidationInput? A bit of a mouthful 🙄

sveitser · 2024-10-24T10:01:45Z

types/src/v0/impls/state.rs

+        self.validate_fee()?;
+        self.validate_fee_merkle_tree()?;
+        self.validate_block_merkle_tree()?;
+        self.validate_proposed_finalized()?;


Suggested change

self.validate_proposed_finalized()?;

self.validate_l1_finalized()?;

sveitser · 2024-10-24T10:05:42Z

types/src/v0/impls/state.rs

-
-        charge_fee(
-            &mut validated_state,
+        validated_state.charge_fees(


Should we do the fee charging as part of apply_proposal?

sveitser · 2024-10-24T10:08:10Z

types/src/v0/impls/state.rs

+                Header::V3(_) => panic!("You called `Header.next()` on unimplemented version (v3)"),
+            }
+        }
+        /// Replaces builder signature w/ invalid one.


Suggested change

/// Replaces builder signature w/ invalid one.

/// Replaces builder signature w/ a valid one from a random key pair

?

The intent is convey that signature validation will fail because we are adding a signature that doesn't match the fee_info. It is therefore invalid (relative to what it will be validated against).

Thinking again I see your point... The signature is structurally valid but will fail verification.

sveitser · 2024-10-24T10:09:05Z

types/src/v0/impls/state.rs

+    }
+
+    impl<'a> ValidatedTransition<'a> {
+        async fn mock(instance: NodeState, parent: &'a Header, proposal: Proposal<'a>) -> Self {


Remove async?

sveitser · 2024-10-24T10:11:36Z

types/src/v0/impls/state.rs

@@ -1259,4 +1855,112 @@ mod test {

        validate_builder_fee(&header).unwrap();
    }
+
+    // #[async_std::test]


Commented block

sveitser

LGTM. I think this is a great improvement in terms of readability. Just some suggestions.

I also think @0xkato should review this PR as well.

* add `Proposal` and `ValidatedTransition` types * move validation to isolated methods. * add doc comments to methods * update error enum * update tests * less clones * only get vid byte len once

Replace references to `validate_proposal` /w `ValidatedTransition`. Remove `validate_proposal_error_cases` as it nightmarish to navigate that and error cases are more thoroughly tested in unit tests now.

`validate_proposal` fn no longer exists

- Move the validate function to the top so it shows up first in docs. - Try to provide more context in doc strings.

now hotshot prints the same thing

Rename `validate_proposed_finalized()` to `self.validate_l1_finalized()`

I was going to move this test but decided not to.

and the TODO that goes w/ it

zacshowa reviewed Oct 14, 2024

View reviewed changes

tbro force-pushed the tb/validated_state_transition_refactor branch from cc46f2a to 1f1615e Compare October 15, 2024 19:15

tbro force-pushed the tb/validated_state_transition_refactor branch from fc0cab9 to 5756051 Compare October 23, 2024 20:09

tbro marked this pull request as ready for review October 23, 2024 21:43

tbro requested review from philippecamacho, ImJeremyHe, sveitser, jbearer and imabdulbasit as code owners October 23, 2024 21:43

tbro requested review from sveitser and removed request for sveitser October 23, 2024 21:44

tbro force-pushed the tb/validated_state_transition_refactor branch from 910ce81 to f7926c1 Compare October 24, 2024 00:50

sveitser reviewed Oct 24, 2024

View reviewed changes

sveitser approved these changes Oct 24, 2024

View reviewed changes

sveitser requested a review from 0xkato October 24, 2024 10:25

tbro and others added 7 commits October 24, 2024 10:38

Create internal type

555012d

* add `Proposal` and `ValidatedTransition` types * move validation to isolated methods. * add doc comments to methods * update error enum * update tests * less clones * only get vid byte len once

fix validate_proposal_success test

62e37ef

Replace references to `validate_proposal` /w `ValidatedTransition`. Remove `validate_proposal_error_cases` as it nightmarish to navigate that and error cases are more thoroughly tested in unit tests now.

cargo clippy

684e9f6

rename success test

a188b81

`validate_proposal` fn no longer exists

WIP: some doc improvements

61d6bdd

- Move the validate function to the top so it shows up first in docs. - Try to provide more context in doc strings.

remove duplicated message

ecad586

now hotshot prints the same thing

Rename fn

c12846c

Rename `validate_proposed_finalized()` to `self.validate_l1_finalized()`

tbro added 4 commits October 24, 2024 10:38

remove async from mock

9f91cd0

remove commented block

abd5552

I was going to move this test but decided not to.

move drift comment

da8abc3

Remove extra clone of ValidatedState

3ea125f

and the TODO that goes w/ it

tbro force-pushed the tb/validated_state_transition_refactor branch from da1f3f4 to 3ea125f Compare October 24, 2024 16:39

tbro merged commit 4144ba4 into main Oct 24, 2024
13 checks passed

tbro deleted the tb/validated_state_transition_refactor branch October 24, 2024 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replica Validation Refactor #2170

Replica Validation Refactor #2170

tbro commented Oct 11, 2024 •

edited

Loading

zacshowa left a comment

zacshowa Oct 14, 2024

tbro Oct 15, 2024 •

edited

Loading

zacshowa Oct 21, 2024

tbro Oct 22, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

sveitser Oct 24, 2024 •

edited

Loading

tbro Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser Oct 24, 2024

tbro Oct 24, 2024

sveitser left a comment

	self.validate_proposed_finalized()?;
	self.validate_l1_finalized()?;

	/// Replaces builder signature w/ invalid one.
	/// Replaces builder signature w/ a valid one from a random key pair

Replica Validation Refactor #2170

Replica Validation Refactor #2170

Conversation

tbro commented Oct 11, 2024 • edited Loading

This PR:

Review

Review Documentation

zacshowa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbro Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveitser Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveitser left a comment

Choose a reason for hiding this comment

tbro commented Oct 11, 2024 •

edited

Loading

tbro Oct 15, 2024 •

edited

Loading

sveitser Oct 24, 2024 •

edited

Loading