refactor: extract chunk producer logic from Client #12852
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12852 +/- ##
==========================================
+ Coverage 70.35% 70.37% +0.01%
==========================================
Files 853 854 +1
Lines 174013 174086 +73
Branches 174013 174086 +73
==========================================
+ Hits 122428 122514 +86
+ Misses 46364 46359 -5
+ Partials 5221 5213 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -352,8 +368,8 @@ impl ChainStore { | |||
| /// But we need to implement a more theoretically correct algorithm if shard layouts will change | |||
| /// more often in the future | |||
| /// <https://github.com/near/nearcore/issues/4877> | |||
| pub fn get_outgoing_receipts_for_shard( | |||
| &self, | |||
| pub fn get_outgoing_receipts_for_shard_from_store( | |||
There was a problem hiding this comment.
Nice! Does it make sense to push this in the same helper module we had created last time?
There was a problem hiding this comment.
Yes. I just don't want to move too much code at once.
chain/client/src/lib.rs
Outdated
| @@ -8,13 +8,16 @@ pub use near_client_primitives::types::{ | |||
| QueryError, Status, StatusResponse, SyncStatus, TxStatus, TxStatusError, | |||
| }; | |||
|
|
|||
| pub use crate::client::{Client, ProduceChunkResult}; | |||
| #[cfg(feature = "test_features")] | |||
| pub use crate::chunk_producer::AdvProduceChunksMode; | |||
There was a problem hiding this comment.
Checking, do we need to expose this outside the module?
| @@ -6,7 +6,8 @@ | |||
|
|
|||
| use near_chain_configs::Genesis; | |||
| use near_client::test_utils::{create_chunk_on_height, TestEnv}; | |||
| use near_client::{ProcessTxResponse, ProduceChunkResult}; | |||
| use near_client::ProcessTxResponse; | |||
| use near_client::ProduceChunkResult; | |||
There was a problem hiding this comment.
mini nit: Can revert to use near_client::{ProcessTxResponse, ProduceChunkResult};? Fine to leave as is.
| @@ -1966,14 +1671,16 @@ impl Client { | |||
| .with_label_values(&[&shard_id.to_string()]) | |||
| .start_timer(); | |||
| let last_header = Chain::get_prev_chunk_header(epoch_manager, block, shard_id).unwrap(); | |||
| match self.try_produce_chunk( | |||
| let result = self.chunk_producer.produce_chunk( | |||
There was a problem hiding this comment.
How did we determine if the parent function produce_chunks was supposed to be a part of client.rs or chunk_producer.rs module?
There was a problem hiding this comment.
Again, I didn't want to touch it yet. The main reason is persist_and_distribute_encoded_chunk using many client fields
chain/client/src/chunk_producer.rs
Outdated
| let prev_prev_hash = *self.chain.get_block_header(&prev_block_hash)?.prev_hash(); | ||
| if !ChainStore::prev_block_is_caught_up(&self.chain, &prev_prev_hash, &prev_block_hash)? | ||
| { | ||
| // See comment in similar snipped in `produce_block` |
There was a problem hiding this comment.
nit: this comment is not very helpful IMO, is it possible to copy paste what is said in produce_block?
Reducing number of fields in Client from 36 to 31 and number of lines in client.rs from 2913 to 2624.
This is done by separating logic exclusive to chunk production, thus new struct is named
ChunkProducer. It is almost as simple as moving some methods from Client to ChunkProducer.I had to pass
chain_validate: &dyn Fn(&SignedTransaction) -> bool,through which is not great but already used in other places. Also, I removemutkeywords because the closure doesn't mutate neither environment nor variable.Later, we can also hide
produce_chunksand state witness production logic. This process is a bit heavy because currently access to the whole Client is used everywhere - which is not really needed.Another next step can be separating BlockProducer logic.