feat: avoid processing state witness multiple times #11111
Conversation
pugachAG
force-pushed
the
track-received-witnesses
branch
from
8cc51d4
to
ef0fdad
Compare
pugachAG
force-pushed
the
track-received-witnesses
branch
from
ef0fdad
to
4127a9f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11111 +/- ##
=======================================
Coverage 71.06% 71.06%
=======================================
Files 767 767
Lines 153390 153362 -28
Branches 153390 153362 -28
=======================================
- Hits 109003 108990 -13
+ Misses 39943 39928 -15
Partials 4444 4444
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| /// (shard_id, epoch_id, height_created). This protects against malicious chunk | ||
| /// producers wasting stateless validator resources by making it apply chunk multiple | ||
| /// times. | ||
| fn check_duplicate_witness(&mut self, state_witness: &ChunkStateWitness) -> Result<(), Error> { |
There was a problem hiding this comment.
Could we store the chunk hash as "value" for LRU cache and add a check to see if we have received the same chunk hash? If the chunk hash of the witness is the same it can probably be deemed as a non-malicious behavior?
There was a problem hiding this comment.
Technically submitting the witness for the same chunk (hence with the same chunk hash) could also be an attack vector. For example another stateless validator could send a witness it received to us multiple times and in this case it is not coming from the chunk producer.
On top of that currently we don't really distinguish between "definitely malicious behaviours" and "potentially malicious behaviours", in both cases we just ignore the witness. So I suggest to keep it simple for now and extend it later if we ever need it.
| @@ -103,6 +107,8 @@ impl ChunkValidator { | |||
| ))); | |||
| } | |||
|
|
|||
| self.check_duplicate_witness(&state_witness)?; | |||
There was a problem hiding this comment.
Hmm IMO this should be placed after pre_validate_chunk_state_witness().
Otherwise a malicious chunk producer can do something like:
- Submit valid
ChunkStateWitness - Submit 100 junk
ChunkStateWitnessesto fill the cache with junk (possibly from other random nodes to avoid getting banned) - Go to (1), the cache doesn't contain the valid
ChunkStateWitnessso it'll be processed again.
This way a malicious chunk producer could force the validator to process the valid ChunkStateWitness an arbitrary number of times.
There was a problem hiding this comment.
This should not be possible because of the checks as part of partially_validate_state_witness. In particular chunk producer could only submit one witness for every height it suppose to produce a chunk on. On top of that it should be on top of a not-yet-final block which is known to us (otherwise it goes to orphan pool). So chunk producer could only submit junk for a very narrow set of heights hence the cache size of 100.
There was a problem hiding this comment.
Oh I didn't notice that apart from pre_validate_chunk_state_witness we also do partially_validate_state_witness at the beginning of process_chunk_state_witness . That makes the situation better 👍
There was a problem hiding this comment.
But could a malicious chunk producer submit 100 fake witnesses (with existing prev_block_hash) for the next 100 heights for which it's a chunk producer? I think we don't limit the distance from the tip for non-orphan witnesses?
There was a problem hiding this comment.
Only if we don't have a final block for those 100 heights. If that happens then the blockchain is in a very degraded state and we have bigger problems to worry about 🙃 Keeping Flat Storage deltas in memory was built on the same assumptions by the way.
There was a problem hiding this comment.
The scenario that I'm imagining would look something like this:
There's a final block F at height 100
There's a chunk producer P, responsible for producing chunks at heights 102, 104, 106, 108, 110, ...
P is malicious and constructs a fake witness for each of those heights:
Witness {height: 102, prev_block_hash: F, receipts: lotsofwork}Witness {height: 104, prev_block_hash: F, receipts: lotsofwork}Witness {height: 106, prev_block_hash: F, receipts: lotsofwork}- ...
Then P sends all of those witnesses to validator V.
Wouldn't V process all of those witnesses? P could fill the whole cache with them, or just make V do a lot of useless work.
There was a problem hiding this comment.
Ah, I see, that is indeed a valid scenario, good catch!
Moving this PR to draft until I have a solution that addresses that.
This PR introduces tracking of processes state witnesses as a protection against wasting stateless validator resources.
Note that we track state witnesses for chunks built only on top of the blocks that are known to us (orphan pool takes care of the rest) and not yet final (covered by #11081). So we only need a pretty limited capacity for the LRU cache keeping track of received instances.
Part of #10565.