[stateless_validation] Hack sending chunk endorsement to self for single validator tests #10501
Conversation
…ks proposed by us
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10501 +/- ##
==========================================
+ Coverage 71.97% 71.98% +0.01%
==========================================
Files 720 720
Lines 146260 146269 +9
Branches 146260 146269 +9
==========================================
+ Hits 105264 105286 +22
+ Misses 36163 36150 -13
Partials 4833 4833
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
chain/client/src/chunk_validation.rs
Outdated
| let witness = | ||
| self.create_state_witness(prev_chunk_header, chunk, transactions_storage_proof)?; | ||
|
|
||
| if let Some(my_signer) = self.validator_signer.clone() { | ||
| // Since we've created the state witness, we can directly send the chunk endorsement to ourselves. |
There was a problem hiding this comment.
but should we send the chunk endorsement to ourselves? Shouldn't we send it to the block producer?
There was a problem hiding this comment.
Absolute brain fart... Update the logic to send the block producers. I'm keeping the logic to send chunk endorsement to self tho, useful for tests
| chunk_validators.retain(|account_id| account_id != my_signer.validator_id()); | ||
|
|
||
| // Additionally send a copy of the chunk endorsement to ourselves. | ||
| // Mainly useful in tests where we don't have a good way to handle network messages and there's |
There was a problem hiding this comment.
Ah... this makes me a bit uncomfortable :( If a test doesn't handle network messages causing the endorsements to not propagate, the correct thing to do should be to fix the test, not to change production behavior. A similar problem happened when the ShardsManager was refactored out, which broke a ton of tests because they weren't handling network messages, and the solution there was to make a SynchronousShardsManagerAdapter. It's not great, really, but if we can do something like that for the problem here, I'd prefer that to hacking the code here.
If we must hack the code here then may I suggest that we only send ourselves the endorsement, if we are a block producer?
Btw I'm also not super comfortable with just skipping the validation of our own witness. It doesn't save any latency (as we still have to wait for others to validate), and if our witness somehow turns out to be incorrect, we're going to be signing an invalid state witness just because we produced it. So that makes me think that this is purely a change to ease testing.
There was a problem hiding this comment.
Hmm, I see... In that case we can consider skipping over this optimization and for the short term measure of getting in changes for the MVP, send the endorsement to ourselves and add a major TODO for refactoring?
There was a problem hiding this comment.
Yeah, sounds reasonable. Thanks!
…10507) This PR uncomments two checks - We now filter chunks based on whether they have enough stake endorsed or not. - We validate chunk endorsements in block header. Enabling this breaks a bunch of tests mainly for two reasons - Some tests are handled by mocking out parts/doing manual block production which doesn't take care of chunk endorsement signatures - A number of tests that work with multiple validators don't have the appropriate network request handlers for chunk endorsement and state witness processing. These tests are impossible to explore and fix in a single PR and I've created a task list to help with this here: #10506 Note that we need to merge in the changes from #10501 , #10503 and #10504 before we can merge this. Till then probably the tests would keep failing Tip: Probably a good idea to review PR commit by commit
This is extremely useful in 90% of the test cases that have just one validator. This way we don't need to explicitly handle network messages related to
ChunkStateWitnessandChunkEndorsementCreated an issue to fix this in the future: #10508