feat: check partial witness metadata #11441
Conversation
pugachAG
force-pushed
the
check-partial-witness-metadata
branch
from
d15ca10
to
3bfbd4c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11441 +/- ##
==========================================
- Coverage 71.47% 71.45% -0.02%
==========================================
Files 785 785
Lines 158756 158757 +1
Branches 158756 158757 +1
==========================================
- Hits 113466 113445 -21
- Misses 40398 40423 +25
+ Partials 4892 4889 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -753,11 +752,10 @@ impl Client { | |||
| /// you can use the `processing_done_tracker` argument (but it's optional, it's safe to pass None there). | |||
| pub fn process_chunk_state_witness( | |||
| &mut self, | |||
| encoded_witness: EncodedChunkStateWitness, | |||
| witness: ChunkStateWitness, | |||
| raw_witness_size: ChunkStateWitnessSize, | |||
There was a problem hiding this comment.
I checked the use case of raw_witness_size and it's being used in two places
For the size check, we can easily just move it to the partial witness tracker module.
I'm wondering whether it makes sense to push the metrics elsewhere (example decode_state_witness fn) so that we can have a cleaner interface and skip on sending the raw sizes everywhere?
There was a problem hiding this comment.
I totally agree that passing raw_witness_size around sucks as it pollutes the interface.
I think the proper solution here would be to get rid of orphan pool and merge its functionality into partial witness tracker. That is something I want to do in the long term, so I suggest keeping this size ugliness for now.
There was a problem hiding this comment.
For the size check, we can easily just move it to the partial witness tracker module.
The size check is meant only for orphan witnesses. It's dangerous to apply size limit to all witnesses, as we could end up in a state when the witness is too big and we can't move the chain forward.
| /// Returns size in bytes of borsh-serialized state witness. | ||
| /// NOTE: this is potentially expensive operation and should only be | ||
| /// used in tests. | ||
| pub fn borsh_size(&self) -> ChunkStateWitnessSize { |
There was a problem hiding this comment.
If we stop caring about passing raw_size to process_chunk_state_witness fn, we can get rid of this function as well.
There was a problem hiding this comment.
In case this is only used in tests, can we pass some dummy value instead of implementing borsh_size?
There was a problem hiding this comment.
dummy value would where it doesn't matter, but most test cases are actually on orphan pool where it is actually needed
decided to completely get rid of it and just inline usage for now: 535cef9
pugachAG
force-pushed
the
check-partial-witness-metadata
branch
from
b24a33e
to
052e687
Compare
This PR implements check for partial state witness metadata fields (
epoch_id,shard_id,height_created) after decoding complete state witness. This is needed to protect against malicious chunk producer providing incorrect metadata in the partial witness.Large part of this PR is about moving state witness decoding (decompression + borsh-deserialization) from client to partial witness actor. This is required to implement the check, but also a nice change on its own since wintess decompression can take several dozens of milliseconds, so it is better to avoid blocking client actor.
Closes #11303.