[stateless_validation] Split chunk_validation module into multiple files #10522

shreyan-gupta · 2024-01-29T14:43:14Z

Personally found it extremely hard to locate code in the chunk validator module.

Refactored into several files.

…on/refactor

codecov · 2024-01-29T15:14:01Z

Codecov Report

Attention: 137 lines in your changes are missing coverage. Please review.

Comparison is base (95c80bf) 71.92% compared to head (6404632) 71.90%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
...client/src/stateless_validation/shadow_validate.rs	3.77%	102 Missing ⚠️
...src/stateless_validation/state_witness_producer.rs	86.00%	6 Missing and 22 partials ⚠️
.../stateless_validation/chunk_endorsement_tracker.rs	92.85%	2 Missing and 4 partials ⚠️
chain/client/src/chunk_inclusion_tracker.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10522      +/-   ##
==========================================
- Coverage   71.92%   71.90%   -0.03%     
==========================================
  Files         720      723       +3     
  Lines      146747   146751       +4     
  Branches   146747   146751       +4     
==========================================
- Hits       105553   105522      -31     
- Misses      36331    36363      +32     
- Partials     4863     4866       +3

Flag	Coverage Δ
backward-compatibility	`0.08% <0.00%> (ø)`
db-migration	`0.08% <0.00%> (ø)`
genesis-check	`1.25% <0.00%> (-0.01%)`	⬇️
integration-tests	`36.81% <66.17%> (-0.07%)`	⬇️
linux	`71.11% <8.64%> (-0.03%)`	⬇️
linux-nightly	`71.25% <66.17%> (-0.04%)`	⬇️
macos	`53.30% <8.64%> (-1.70%)`	⬇️
pytests	`1.47% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.27% <0.00%> (-0.01%)`	⬇️
unittests	`67.88% <64.19%> (-0.02%)`	⬇️
upgradability	`0.13% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wacban · 2024-01-29T15:36:30Z

chain/client/src/stateless_validation/endrosement_tracker.rs

Ugh, I'm getting too many of these nowdays :(
Thanks

shreyan-gupta · 2024-01-29T15:42:22Z

chain/client/src/client.rs

@@ -187,6 +188,8 @@ pub struct Client {
    /// Tracks current chunks that are ready to be included in block
    /// Also tracks banned chunk producers and filters out chunks produced by them
    pub chunk_inclusion_tracker: ChunkInclusionTracker,
+    /// Tracks chunk endorsements received from chunk validators. Used to filter out chunks ready for inclusion
+    pub chunk_endorsement_tracker: EndorsementTracker,


Moved LRU cache tracking chunk endorsements to a separate struct. No functional changes

shreyan-gupta · 2024-01-29T15:43:49Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        endorsement: ChunkEndorsement,
+    ) -> Result<(), Error> {
+        let chunk_header = self.chain.get_chunk(endorsement.chunk_hash())?.cloned_header();
+        self.chunk_endorsement_tracker.process_chunk_endorsement(chunk_header, endorsement)


Tiny tiny amount of redirection here. Logic for process_chunk_endorsement pushed to EndorsementTracker from Client

wacban

LGTM

just please add comments to the public methods

wacban · 2024-01-29T15:41:12Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+// Ideally, we should not be processing more than num_shards chunks at a time.
+const NUM_CHUNK_ENDORSEMENTS_CACHE_COUNT: usize = 100;
+
+pub struct EndorsementTracker {


Can you add a comment please?

Should we call this ChunkEndorsementTracker, because Endorsement is an overloaded term with "block endorsement"

wacban · 2024-01-29T15:43:41Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        // TODO(stateless_validation): It's possible for a malicious validator to send endorsements
+        // for 100 unique chunks thus pushing out current valid endorsements from our cache.
+        // Maybe add check to ensure we don't accept endorsements from chunks already included in some block?
+        // Maybe add check to ensure we don't accept endorsements from chunks that have too old height_created?


Another option: deduplicate based on the (height, account_id of the chunk validator)

Yeah that, plus a bypass for chunks that are currently being considered for inclusion in a block.

It's kind of a similar issue compared to the ShardsManager forwarding cache, we can't validate much until we wait for the header to arrive. So, one possible way to do this is to have a separate cache based on (height, account id of chunk validator) and only when we have the chunk header do we actually process those. Yeah... a bit messy.

@staffik is working on fixing this issue. We can communicate this idea! Meanwhile for purposes of refactoring, I'm leaving this as is.

wacban · 2024-01-29T15:44:36Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        let epoch_id =
+            self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?;
+        let protocol_version = self.epoch_manager.get_epoch_protocol_version(&epoch_id)?;
+        if !checked_feature!("stable", ChunkValidation, protocol_version) {


We can also consider renaming ChunkValidation to StatelessValidation.

StatelessValidationStage0 maybe? StatelessValidation is a very big name :) Also I still feel like this feature flag is currently only used for stateless chunk validation, not anything else we're planning to launch with stateless validation like validator role change, so maybe StatelessChunkValidation?

StatelessValidation is a big name because we call the whole project that, not of itself ;) For the sake of naming things in code I wouldn't consider role chages etc. as stateless validation. Anyway ChunkValidation is confusing because we most certainly do already validate chunks today so any change would be good.

Hmm, agree with "StatelessValidation" but not sure if we have consensus here? I'm leaving ChunkValidation as is for this PR and we can get back.

wacban · 2024-01-29T15:45:12Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        let Some(chunk_endorsements) = self.chunk_endorsements.peek(&chunk_header.chunk_hash())
+        else {


funny formatting but I guess this is what the autoformatter does

wacban · 2024-01-29T15:46:10Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        let epoch_id =
+            self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?;


you already have epoch_id variable

Good catch! Might have been introduced while merging from master in some previous PR

wacban · 2024-01-29T15:46:58Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+
+        // Check whether the current set of chunk_validators have enough stake to include chunk in block.
+        if !chunk_validator_assignments
+            .does_chunk_have_enough_stake(&chunk_endorsements.keys().cloned().collect())


Is cloned necessary? Can you pass by reference?

Changed signature to take HashSet<&AccountId> instead of &HashSet<AccountId>.

wacban · 2024-01-29T15:47:50Z

chain/client/src/stateless_validation/shadow_validate.rs

+use crate::{metrics, Client};
+
+impl Client {
+    pub(crate) fn shadow_validate_block_chunks(&mut self, block: &Block) -> Result<(), Error> {


nit: add a small comment please

robin-near

Very nice refactoring, thank you! Future developers will thank you for keeping the files small and the structs modular. LGTM.

robin-near · 2024-01-29T18:16:25Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+// Ideally, we should not be processing more than num_shards chunks at a time.
+const NUM_CHUNK_ENDORSEMENTS_CACHE_COUNT: usize = 100;
+
+pub struct EndorsementTracker {


Should we call this ChunkEndorsementTracker, because Endorsement is an overloaded term with "block endorsement"

robin-near · 2024-01-29T18:17:02Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+
+// This is the number of unique chunks for which we would track the chunk endorsements.
+// Ideally, we should not be processing more than num_shards chunks at a time.
+const NUM_CHUNK_ENDORSEMENTS_CACHE_COUNT: usize = 100;


nit: maybe call it NUM_CHUNKS_IN_CHUNK_ENDORSEMENTS_CACHE? It's more descriptive.

robin-near · 2024-01-29T18:20:57Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        // TODO(stateless_validation): It's possible for a malicious validator to send endorsements
+        // for 100 unique chunks thus pushing out current valid endorsements from our cache.
+        // Maybe add check to ensure we don't accept endorsements from chunks already included in some block?
+        // Maybe add check to ensure we don't accept endorsements from chunks that have too old height_created?


Yeah that, plus a bypass for chunks that are currently being considered for inclusion in a block.

It's kind of a similar issue compared to the ShardsManager forwarding cache, we can't validate much until we wait for the header to arrive. So, one possible way to do this is to have a separate cache based on (height, account id of chunk validator) and only when we have the chunk header do we actually process those. Yeah... a bit messy.

robin-near · 2024-01-29T18:22:17Z

chain/client/src/stateless_validation/endorsement_tracker.rs

+        let epoch_id =
+            self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?;
+        let protocol_version = self.epoch_manager.get_epoch_protocol_version(&epoch_id)?;
+        if !checked_feature!("stable", ChunkValidation, protocol_version) {


StatelessValidationStage0 maybe? StatelessValidation is a very big name :) Also I still feel like this feature flag is currently only used for stateless chunk validation, not anything else we're planning to launch with stateless validation like validator role change, so maybe StatelessChunkValidation?

pugachAG

super nit: maybe change the title to be a bit more specific: "split chunk_validation module into multiple files"
and please mention #10275

…on/refactor

@wacban

…ValidationV0` (#10540) Originally discussed in [this comment](#10522 (comment)), I feel like renaming feature `ChunkValidation` to `StatelessValidationV0` makes sense. With subsequent releases, we can have `StatelessValidationV1` etc. @wacban @robin-near, thoughts?

[stateless_validaiton] Refactor chunk_validator code

4ab652f

shreyan-gupta requested review from pugachAG, Longarithm, staffik, robin-near and jancionear January 29, 2024 14:43

shreyan-gupta requested a review from a team as a code owner January 29, 2024 14:43

Shreyan Gupta added 2 commits January 29, 2024 20:14

Merge remote-tracking branch 'origin' into shreyan/stateless_validati…

dcb3f80

…on/refactor

less churn + fmt

cb203d8

wacban reviewed Jan 29, 2024

View reviewed changes

typo

70958dc

shreyan-gupta commented Jan 29, 2024

View reviewed changes

wacban approved these changes Jan 29, 2024

View reviewed changes

wacban changed the title ~~[stateless_validaiton] Refactor chunk_validator code~~ [stateless_validation] Refactor chunk_validator code Jan 29, 2024

robin-near approved these changes Jan 29, 2024

View reviewed changes

pugachAG approved these changes Jan 29, 2024

View reviewed changes

shreyan-gupta changed the title ~~[stateless_validation] Refactor chunk_validator code~~ [stateless_validation] Split chunk_validation module into multiple files Jan 29, 2024

shreyan-gupta mentioned this pull request Jan 29, 2024

Break down large source code files into smaller ones #10275

Open

Shreyan Gupta added 2 commits January 30, 2024 17:58

Merge remote-tracking branch 'origin' into shreyan/stateless_validati…

9fd7ebc

…on/refactor

resolve comments

889a715

shreyan-gupta enabled auto-merge January 30, 2024 12:47

-_-

6404632

shreyan-gupta added this pull request to the merge queue Jan 30, 2024

Merged via the queue into master with commit 699a5c7 Jan 30, 2024
24 of 26 checks passed

shreyan-gupta deleted the shreyan/stateless_validation/refactor branch January 30, 2024 13:35

staffik mentioned this pull request Jan 30, 2024

[stateless_validation] Chunk endorsements spam protection #10516

Closed

shreyan-gupta mentioned this pull request Jan 31, 2024

[stateless_validation] Rename feature ChunkValidation to StatelessValidationV0 #10540

Merged

shreyan-gupta added the A-stateless-validation Area: stateless validation label Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stateless_validation] Split chunk_validation module into multiple files #10522

[stateless_validation] Split chunk_validation module into multiple files #10522

shreyan-gupta commented Jan 29, 2024

codecov bot commented Jan 29, 2024 •

edited

Loading

wacban Jan 29, 2024

shreyan-gupta Jan 29, 2024

shreyan-gupta Jan 29, 2024

shreyan-gupta Jan 29, 2024

wacban left a comment

wacban Jan 29, 2024

robin-near Jan 29, 2024

wacban Jan 29, 2024

robin-near Jan 29, 2024

shreyan-gupta Jan 30, 2024

wacban Jan 29, 2024

robin-near Jan 29, 2024

wacban Jan 29, 2024

shreyan-gupta Jan 30, 2024

wacban Jan 29, 2024

wacban Jan 29, 2024

shreyan-gupta Jan 30, 2024

wacban Jan 29, 2024

shreyan-gupta Jan 30, 2024

wacban Jan 29, 2024

robin-near left a comment

robin-near Jan 29, 2024

robin-near Jan 29, 2024

robin-near Jan 29, 2024

robin-near Jan 29, 2024

pugachAG left a comment

		let Some(chunk_endorsements) = self.chunk_endorsements.peek(&chunk_header.chunk_hash())
		else {

		let epoch_id =
		self.epoch_manager.get_epoch_id_from_prev_block(chunk_header.prev_block_hash())?;

[stateless_validation] Split chunk_validation module into multiple files #10522

[stateless_validation] Split chunk_validation module into multiple files #10522

Conversation

shreyan-gupta commented Jan 29, 2024

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

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

wacban left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robin-near left a 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

pugachAG left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2024 •

edited

Loading