refactor: state witness struct #10934

pugachAG · 2024-04-04T09:03:31Z

This PR is a preparation for introducing state witness compression as part of #10780.

It addresses the following issues:

State witness is implicitly deserialised when it is received via network and then serialised again in order to verify signature. While this is not a big issue now, we can't afford doing that once compression is introduced. In this PR ChunkStateWitness is renamed to SignedChunkStateWitness and now it holds raw bytes of ChunkStateWitnessInner (now renamed to just ChunkStateWitness).
Duplicate validation in orphan witness handling and normal validation process. Currently we perform bunch of validations multiple times: first in handle_orphan_state_witness and then in start_validating_chunk, it would be nice to avoid that. This is addressed by introducing redundant fields as part of ChunkStateWitness: chunk_producer account id and epoch_id. This way we don't need to have previous block available in order to verify signature and perform a bunch of sanity checks (such as validity of shard_id), so duplicated logic is moved out of orphan witness handling to partially_validate_state_witness_in_epoch.
Currently we don't send state witness ack in case of orphan state witness because we cannot determine chunk producer account id. This is fixed by using newly introduced chunk_producer field of ChunkStateWitness.

codecov · 2024-04-04T12:49:36Z

Codecov Report

Attention: Patch coverage is 80.83832% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 71.36%. Comparing base (efd0b83) to head (d9c170c).
Report is 24 commits behind head on master.

Files	Patch %	Lines
...nt/src/stateless_validation/chunk_validator/mod.rs	75.90%	11 Missing and 9 partials ⚠️
...client/src/stateless_validation/shadow_validate.rs	0.00%	4 Missing ⚠️
chain/chain/src/test_utils/kv_runtime.rs	0.00%	2 Missing ⚠️
core/primitives/src/validator_signer.rs	50.00%	2 Missing ⚠️
...idation/chunk_validator/orphan_witness_handling.rs	80.00%	1 Missing ⚠️
..._validation/chunk_validator/orphan_witness_pool.rs	85.71%	1 Missing ⚠️
...src/stateless_validation/state_witness_producer.rs	95.65%	0 Missing and 1 partial ⚠️
chain/epoch-manager/src/adapter.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10934      +/-   ##
==========================================
- Coverage   71.51%   71.36%   -0.15%     
==========================================
  Files         758      760       +2     
  Lines      151663   152759    +1096     
  Branches   151663   152759    +1096     
==========================================
+ Hits       108467   109023     +556     
- Misses      38710    39209     +499     
- Partials     4486     4527      +41

Flag	Coverage Δ
backward-compatibility	`0.24% <0.00%> (-0.01%)`	⬇️
db-migration	`0.24% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.43% <0.00%> (-0.01%)`	⬇️
integration-tests	`37.00% <78.44%> (-0.11%)`	⬇️
linux	`69.84% <10.77%> (-0.14%)`	⬇️
linux-nightly	`70.83% <80.83%> (-0.18%)`	⬇️
macos	`54.41% <10.77%> (+0.01%)`	⬆️
pytests	`1.65% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.44% <0.00%> (-0.01%)`	⬇️
unittests	`67.02% <35.92%> (-0.14%)`	⬇️
upgradability	`0.29% <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.

core/primitives/src/stateless_validation.rs

chain/client/src/stateless_validation/chunk_validator/mod.rs

shreyan-gupta

Like the consolidation of validation logic between orphan pool and chunk validator :)

core/primitives/src/stateless_validation.rs

chain/epoch-manager/src/adapter.rs

shreyan-gupta · 2024-04-05T00:20:39Z

core/primitives/src/stateless_validation.rs

+}
+
+#[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)]
+pub struct SignedChunkStateWitness {


Could we create a SignedChunkStateWitness::new(witness: &ChunkStateWitness, signer: &dyn ValidatorSigner) -> Self?

Maybe add a fn witness_size() here as well

added new: 680d7ef
regarding fn witness_size(): we can use signed_witness.witness_bytes.size_bytes(), it is not used in many places, so let's keep it like this for now

shreyan-gupta · 2024-04-05T00:44:09Z

chain/client/src/stateless_validation/chunk_validator/orphan_witness_handling.rs

-                witness_height
-            )));
-            }
+        if !possible_epochs.contains(&witness.epoch_id) {


Since we are doing partially_validate_state_witness_in_epoch, I think we can completely get rid of this check here. We can even delete the possible_epochs_of_height_around_tip function.

I thought about that as well, but there is one nasty scenario where this could backfire: state witness has random non-existing prev_block_hash and some old epoch where the chunk producer happened to be a chunk producer for that height and shard id. While this is highly unlikely, I don't think we can just disregard this.
Later I'm planning adding more epoch and height checks to partially_validate_state_witness_in_epoch and then we can remove it, but let's keep it for now, this PR is big enough as it is right now.

It's really nice that we don't have to validate in multiple epochs now, the epoch_idfield is really useful 👍.
But I feel that we'll have to keep possible_epochs_of_height_around_tip around, otherwise a malicious chunk producer might be able to spam the orphan witness pool.

But don't we have ALLOWED_ORPHAN_WITNESS_DISTANCE_FROM_HEAD.contains(&head_distance) to handle such scenarios?

we don't have chunk to verify chunk header

chunk_producer can not be faked as we are verifying signature

we verify if chunk_producer is the correct chunk producer for the epoch and height (here it shouldn't be possible to have some old/bad epoch_id for the given height?)

we verify that the height parameter isn't too far away from head height

But I guess it's fine to leave it here for now and think about it later

Also, we can consider moving ALLOWED_ORPHAN_WITNESS_DISTANCE_FROM_HEAD.contains(&head_distance) to partially_validate_state_witness?

But don't we have ALLOWED_ORPHAN_WITNESS_DISTANCE_FROM_HEAD.contains(&head_distance) to handle such scenarios?

We do, but I think we also need to check that the epoch is within some distance from the tip.
Let's say we have a chunk producer who has been a chunk producer for the past 300 epochs and the current height is 1000.
This chunk producer could try producing a malicious chunk state witness with height 1003 and epochs 1, 2, 3, 4, ...
For some of those epochs he would be the chosen chunk producer at that height. For each of such epoch he can produce a ChunkStateWitness which willll pass the signature and distance from tip check, and enter the orphan witness pool. This way this chunk producer could theoretically spam the orphan pool, which only has capacity for 25 witnesses.
I'm not sure how much of a real threat it is, but it's safer to check that it's one of the epochs around the tip.

As for why it's good to use possible_epochs_of_height_around_tip, I wrote a longer comment here: #10613 (comment)

Will go through the comments and would have to think harder. It's still not super clear to me why we need this but will spend some time :)

Thanks @jancionear for the context! Meanwhile this definitely doesn't need to be a blocker.

chain/client/src/stateless_validation/chunk_validator/mod.rs

jancionear

lgtm 👍

jancionear · 2024-04-05T13:32:56Z

chain/client/src/stateless_validation/state_witness_producer.rs

+        metrics::CHUNK_STATE_WITNESS_TOTAL_SIZE
+            .with_label_values(&[&chunk_header.shard_id().to_string()])
+            .observe(signed_witness.witness_bytes.size_bytes() as f64);


It'd be nice to have separate metrics for encoded and decoded witness sizes. But that can be added later along with the compression.

yep, definitely, will do that as part of adding compression

jancionear · 2024-04-05T13:43:26Z

chain/client/src/stateless_validation/chunk_validator/mod.rs

+    /// Performs state witness decoding and partial validation without requiring the previous block.
+    /// Here we rely on epoch_id provided as part of the state witness. Later we verify that this
+    /// epoch_id actually corresponds to the chunk's previous block.
+    fn partially_validate_state_witness_in_epoch(


I guess the _in_epoch part of the name could be removed. The previous function was called partially_validate_orphan_witness_in_epoch because it took epoch_id as an argument.

good point, done: 815be9d

jancionear · 2024-04-05T13:52:48Z

chain/client/src/stateless_validation/chunk_validator/mod.rs

+                self.handle_orphan_state_witness(
+                    witness,
+                    signed_witness.witness_bytes.size_bytes(),
+                )?;


This should be the size of a decoded ChunkStateWitness. For now that's the same size as encoded, so it's ok, but later when compression is introduced it'll have to be changed.

ah, good point, I will keep that in mind when adding compression

jancionear · 2024-04-05T13:59:43Z

chain/client/src/stateless_validation/chunk_validator/mod.rs

-        // First find the AccountId for the chunk producer and then send the ack to that account.
-        let chunk_header = &witness.inner.chunk_header;
-        let prev_block_hash = chunk_header.prev_block_hash();
-        let epoch_id = self.epoch_manager.get_epoch_id_from_prev_block(prev_block_hash)?;
-        let chunk_producer = self.epoch_manager.get_chunk_producer(
-            &epoch_id,
-            chunk_header.height_created(),
-            chunk_header.shard_id(),
-        )?;
-
+    fn send_state_witness_ack(&self, witness: &ChunkStateWitness) {
        self.network_adapter.send(PeerManagerMessageRequest::NetworkRequests(
            NetworkRequests::ChunkStateWitnessAck(
-                chunk_producer,
-                ChunkStateWitnessAck::new(&witness),
+                witness.chunk_producer.clone(),
+                ChunkStateWitnessAck::new(witness),
            ),
        ));


The chunk_producer field in ChunkStateWitness feels a bit redundant. AFAIU the only use of this field is to send an Ack back to the chunk producer, but we could take this information from the message itself. Each RoutedMessage contains PeerId of the original message author:

nearcore/chain/network/src/network_protocol/mod.rs

Line 623 in 4cea196

pub author: PeerId,

Yeah, I agree that it is redundant, but it makes code a bit simpler since it is directly available as part of ChunkStateWitness. I've decided to add it since it doesn't affect state witness size much and also we already have redundant fields such as applied_receipts_hash there.

jancionear · 2024-04-05T14:04:28Z

chain/client/src/stateless_validation/chunk_validator/orphan_witness_handling.rs

-                witness_height
-            )));
-            }
+        if !possible_epochs.contains(&witness.epoch_id) {


It's really nice that we don't have to validate in multiple epochs now, the epoch_idfield is really useful 👍.
But I feel that we'll have to keep possible_epochs_of_height_around_tip around, otherwise a malicious chunk producer might be able to spam the orphan witness pool.

pugachAG force-pushed the refactor-state-witness-struct branch 3 times, most recently from cd40791 to 9c34503 Compare April 4, 2024 11:56

jancionear self-requested a review April 4, 2024 11:58

pugachAG force-pushed the refactor-state-witness-struct branch from 9c34503 to 3651a5e Compare April 4, 2024 12:28

impl

46a410a

pugachAG force-pushed the refactor-state-witness-struct branch from 3651a5e to 46a410a Compare April 4, 2024 12:57

pugachAG requested review from tayfunelmas and shreyan-gupta April 4, 2024 12:57

pugachAG marked this pull request as ready for review April 4, 2024 12:57

pugachAG requested a review from a team as a code owner April 4, 2024 12:57

tayfunelmas reviewed Apr 4, 2024

View reviewed changes

core/primitives/src/stateless_validation.rs Show resolved Hide resolved

pugachAG added 2 commits April 4, 2024 16:22

Add EncodedChunkStateWitness rustdoc

e1a843f

Add rustdoc for SignedChunkStateWitness fields

6c7325a

pugachAG requested a review from tayfunelmas April 4, 2024 14:28

tayfunelmas reviewed Apr 4, 2024

View reviewed changes

chain/client/src/stateless_validation/chunk_validator/mod.rs Show resolved Hide resolved

shreyan-gupta approved these changes Apr 5, 2024

View reviewed changes

pugachAG added 3 commits April 5, 2024 13:13

Add epoch id comment

017902d

rename SignedEncodedChunkStateWitness

8a51329

Add SignedEncodedChunkStateWitness::new

680d7ef

pugachAG force-pushed the refactor-state-witness-struct branch from 3b29f73 to fa24d81 Compare April 5, 2024 11:59

remove unused import

d2c7a99

jancionear approved these changes Apr 5, 2024

View reviewed changes

rename partially_validate_state_witness

815be9d

pugachAG force-pushed the refactor-state-witness-struct branch from fa24d81 to 815be9d Compare April 5, 2024 14:13

tayfunelmas approved these changes Apr 5, 2024

View reviewed changes

fix comment

d9c170c

pugachAG enabled auto-merge April 8, 2024 14:06

pugachAG added this pull request to the merge queue Apr 8, 2024

Merged via the queue into master with commit 6c71411 Apr 8, 2024
28 of 29 checks passed

pugachAG deleted the refactor-state-witness-struct branch April 8, 2024 14:54

tayfunelmas mentioned this pull request Apr 12, 2024

Measure chunk validation network roundtrip #10790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: state witness struct #10934

refactor: state witness struct #10934

pugachAG commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

shreyan-gupta left a comment

shreyan-gupta Apr 5, 2024

shreyan-gupta Apr 5, 2024

pugachAG Apr 5, 2024

shreyan-gupta Apr 5, 2024

pugachAG Apr 5, 2024

jancionear Apr 5, 2024

shreyan-gupta Apr 5, 2024

shreyan-gupta Apr 5, 2024 •

edited

Loading

shreyan-gupta Apr 5, 2024

jancionear Apr 5, 2024

jancionear Apr 5, 2024

shreyan-gupta Apr 5, 2024 •

edited

Loading

jancionear left a comment

jancionear Apr 5, 2024

pugachAG Apr 5, 2024

jancionear Apr 5, 2024

pugachAG Apr 5, 2024

jancionear Apr 5, 2024

pugachAG Apr 5, 2024

jancionear Apr 5, 2024

pugachAG Apr 5, 2024

jancionear Apr 5, 2024

refactor: state witness struct #10934

refactor: state witness struct #10934

Conversation

pugachAG commented Apr 4, 2024 • edited Loading

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

shreyan-gupta 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

shreyan-gupta Apr 5, 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

shreyan-gupta Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

jancionear 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

pugachAG commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading

shreyan-gupta Apr 5, 2024 •

edited

Loading

shreyan-gupta Apr 5, 2024 •

edited

Loading