Implement simple state witness distribution, chunk validation, and chunk approval distribution skeletons. #10287
Conversation
…unk approval distribution skeletons.
|
I've turned off automatic merging altogether until I understand this problem. |
chain/client/src/chunk_validation.rs
Outdated
| /// A module that handles chunk validation logic. Chunk validation refers to a | ||
| /// critical process of stateless validation, where chunk validators (certain | ||
| /// validators selected to validate the chunk) verify that the chunk's state | ||
| /// witness is correct, and then sends chunk endorsements to the block producer |
There was a problem hiding this comment.
grammar nit: should "send" be used here instead of "sends"? 🤔
chain/client/src/chunk_validation.rs
Outdated
| /// endorsement message to the block producer. The actual validation logic | ||
| /// happens in a separate thread. | ||
| pub fn start_validating_chunk(&self, state_witness: ChunkStateWitness) -> Result<(), Error> { | ||
| if self.my_signer.is_none() { |
There was a problem hiding this comment.
nit: I suggest to avoid unwrapping this later:
let Some(my_signer) = self.my_signer else {
return Err(Error::NotAValidator);
}
and then use my_signer instead of self.my_signer.unwrap()
There was a problem hiding this comment.
whoaaaaaaaaaa, I never knew let-else is a thing in Rust!! I had always been doing
let my_signer = if let Some(my_signer) = self.my_signer.as_ref() { my_signer } else { return ... }
THANK YOU!
| let runtime_adapter = self.runtime_adapter.clone(); | ||
| rayon::spawn(move || match validate_chunk(&state_witness, &*runtime_adapter) { | ||
| Ok(()) => { | ||
| tracing::debug!( |
There was a problem hiding this comment.
nit: Having chunk hash and block producer not as a part of the message should make logs easier to work with. Also that seems to be a bit more common pattern in our code base. So maybe consider moving those outside of the message.
There was a problem hiding this comment.
Do you mean moving them as fields?
There was a problem hiding this comment.
yeah, having separate fields instead of string interpolation into the message
| /// witness is correct, and then sends chunk endorsements to the block producer | ||
| /// so that the chunk can be included in the block. | ||
| pub struct ChunkValidator { | ||
| my_signer: Option<Arc<dyn ValidatorSigner>>, |
There was a problem hiding this comment.
what does 'my_signer' mean? :o
There was a problem hiding this comment.
If I'm a validator, this is my signer. It's the signer of the node itself. I'll make a comment.
| /// Performs the chunk validation logic. When done, it will send the chunk | ||
| /// endorsement message to the block producer. The actual validation logic | ||
| /// happens in a separate thread. | ||
| pub fn start_validating_chunk(&self, state_witness: ChunkStateWitness) -> Result<(), Error> { |
There was a problem hiding this comment.
out of curiosity, how is the word 'start' used in rust? Does it indicate async nature of the function?
There was a problem hiding this comment.
I don't think Rust functions commonly spawn an asynchronous task to do things. Those that do are called "spawn(...)" but that specifically spawns a generic function or task into some thread. So I'm following the naming used in Chain::start_processing_block etc.
|
|
||
| /// Distributes the chunk state witness to chunk validators that are | ||
| /// selected to validate this chunk. | ||
| pub fn send_chunk_state_witness_to_chunk_validators( |
There was a problem hiding this comment.
This is little confusing. Is this client used by both chunk proposer AND chunk validator?
There was a problem hiding this comment.
Yes, Client is used by everyone so this is just an additional function on the Client. I didn't want to put it in client.rs, so that I don't further increase the size of that file.
| let num_requests = self.requests.read().unwrap().len(); | ||
| let mut handled = false; | ||
| for _ in 0..num_requests { | ||
| let request = self.requests.write().unwrap().pop_front().unwrap(); |
There was a problem hiding this comment.
Is pop_front().unwrap() actually fine here? If someone else called handle_filtered against it, it is possible that there will be no requests in the queue in the end and it will panic. Looks scary...
There was a problem hiding this comment.
I mean, this mock object is supposed to be drained by a single thread - the testing thread. Other threads can only push messages to it. Having two threads calling handle_filtered can't possibly lead to a good test even if this implementation was correct.
There was a problem hiding this comment.
Oh that's a test environment, I've missed that. Anyway worth fixing
There was a problem hiding this comment.
I mean, this mock object is supposed to be drained by a single thread - the testing thread. Other threads can only push messages to it. Having two threads calling handle_filtered can't possibly lead to a good test even if this implementation was correct.
Just got your message. Let's have it documented then - by saying that MockPeerManagerAdapter is owned only by one thread?
| let mut records = Vec::new(); | ||
| for (i, account) in accounts.iter().enumerate() { | ||
| // The staked amount must be consistent with validators from genesis. | ||
| let staked = if i < 8 { validator_stake } else { 0 }; | ||
| records.push(StateRecord::Account { | ||
| account_id: account.clone(), | ||
| account: Account::new(0, staked, CryptoHash::default(), 0), | ||
| }); | ||
| // The total supply must be correct to pass validation. | ||
| genesis_config.total_supply += staked; | ||
| } | ||
| let genesis = Genesis::new(genesis_config, GenesisRecords(records)).unwrap(); |
There was a problem hiding this comment.
nit: looks generic enough to make it a method Genesis::from_accounts(genesis_config, accounts). I'd guess other tests should do the same. Not even relevant to this PR, just makes me think that logic of creating genesis is convoluted.
|
So this apparently breaks tens of tests :( because of the additional network message that integration tests don't expect. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10287 +/- ##
==========================================
+ Coverage 71.84% 71.90% +0.05%
==========================================
Files 712 714 +2
Lines 143447 143704 +257
Branches 143447 143704 +257
==========================================
+ Hits 103061 103324 +263
+ Misses 35658 35640 -18
- Partials 4728 4740 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This is a pretty rough PR, but I'm making an attempt to concretely implement the flow from distributing the state witness to sending the chunk endorsement. The following is left out: (1) production of the state witness; (2) actual validation logic; (3) receiving the chunk endorsement. However, the skeleton of the flow is there, including multithreading of the validation logic.
Very open to comments.