[refactor] Refactor client and client actor to move the code for block processing to client #7898
Conversation
Let'ss add this to ClientActor's docstring |
chain/client/src/client.rs
Outdated
| if let near_chain::Error::DBNotFoundErr(msg) = err { | ||
| debug_assert!(!msg.starts_with("BLOCK HEIGHT"), "{:?}", err); | ||
| } |
There was a problem hiding this comment.
This is a pre-existing sketchiness, iffing on error messages is yuck
| block: &Block, | ||
| was_requested: bool, | ||
| ) -> Result<bool, near_chain::Error> { | ||
| let head = self.chain.head()?; |
There was a problem hiding this comment.
Aha, so here we replace unwrap_or_returns with just ?, right? Looks correct to me, given that we just log the stuff at the call-side. Also, major kuros for removing unwrap_or_return -- it was making the code quite hard to read!
chain/client/src/client_actor.rs
Outdated
| @@ -1239,10 +1247,20 @@ impl ClientActor { | |||
| fn produce_block(&mut self, next_height: BlockHeight) -> Result<(), Error> { | |||
| let _span = tracing::debug_span!(target: "client", "produce_block", next_height).entered(); | |||
| if let Some(block) = self.client.produce_block(next_height)? { | |||
| let peer_id = self.node_id.clone(); | |||
| // We’ve produced the block so that counts as validated block. | |||
chain/client/src/client.rs
Outdated
| self.verify_and_rebroadcast_block(&block, was_requested, &peer_id)?; | ||
| let provenance = | ||
| if was_requested { near_chain::Provenance::SYNC } else { near_chain::Provenance::NONE }; | ||
| let res = self.start_process_block(block.into(), provenance, apply_chunks_done_callback); |
There was a problem hiding this comment.
| let res = self.start_process_block(block.into(), provenance, apply_chunks_done_callback); | |
| let res = self.start_process_block(block, provenance, apply_chunks_done_callback); |
| ); | ||
| // If we produced it, we don’t need to validate it. Mark the block | ||
| // as valid. | ||
| block.mark_as_valid(); |
There was a problem hiding this comment.
Note: we lost this statement, but I think this is correct, this seems to be dead code: when we have provenance == Provenance::PRODUCED, we already mark block as valid.
| /// propagated in the network fast. | ||
| fn verify_and_rebroadcast_block( | ||
| &mut self, | ||
| block: &MaybeValidated<Block>, |
There was a problem hiding this comment.
Hm, the fact that we statically know that the block is not validated here, but is validatde afterwards, gives me a pause. It seems that, eg, start_process_block always receives a validated block?
Not sure if there's anythnig to fixhere, and that's definietly out of scope for the pr
There was a problem hiding this comment.
Yes I had the same thoughts too and also agree that it's out of the scope of this PR. I will create a github issue and look more into this.
…k processing to client (#7898) This PR is a pure refactoring. The context is that any processing details should be put in Client instead of ClientActor. ClientActor should just serve as a coordinator class to handle messages and check triggers and immediately pass it to Client. This is better for testing since we can't write unit test for any logic in ClientActor and also better for code readability as the logic is not scattered in two classes. This PR only moves the part around block processing. The rest is tracked by #7899
…k processing to client (#7898) This PR is a pure refactoring. The context is that any processing details should be put in Client instead of ClientActor. ClientActor should just serve as a coordinator class to handle messages and check triggers and immediately pass it to Client. This is better for testing since we can't write unit test for any logic in ClientActor and also better for code readability as the logic is not scattered in two classes. This PR only moves the part around block processing. The rest is tracked by #7899
…k processing to client (#7898) This PR is a pure refactoring. The context is that any processing details should be put in Client instead of ClientActor. ClientActor should just serve as a coordinator class to handle messages and check triggers and immediately pass it to Client. This is better for testing since we can't write unit test for any logic in ClientActor and also better for code readability as the logic is not scattered in two classes. This PR only moves the part around block processing. The rest is tracked by #7899
This PR is a pure refactoring. The context is that any processing details should be put in Client instead of ClientActor. ClientActor should just serve as a coordinator class to handle messages and check triggers and immediately pass it to Client. This is better for testing since we can't write unit test for any logic in ClientActor and also better for code readability as the logic is not scattered in two classes.
This PR only moves the part around block processing. The rest is tracked by #7899