[chunks] Move chunk completion out of ShardsManager

[robin-near]

• Move handling of chunk completion to a ClientActor message (ShardsManagerResponse), because this will eventually be async. So no more passing DoneApplyingChunksCallback. Add support of this message to TestEnv; tests that rely on chunk part messaging now need to manually handle such messages (because the TestEnv doesn't have real actors). Fixed tests that need this.
• Move ReedSolomonWrapper inside ShardsManager. However Client still has a copy because it actually needs to encode the parts to produce a chunk - at least the merkle root is needed.
• Does not touch ChainStore yet. Leaving that till later.
• Minor refactoring to push chunk part processing logic from Client into ShardsManager (otherwise Client needs to fetch header and stuff).

[mzhangmzz]

Overall looks good! I have two comments:

1. It might be better to achieve the callback of on_chunk_complete not through actor messages. My concern of changing that to actor messages is that it shares the same queue as all other messages that ClientActor receives and if the queue is long, the callback is delayed. You can still have a ClientActorAdapterForShardsManager trait which has on_chunk_complete function and let client actor implement that function.
2. Maybe we should send the whole chunk instead of only chunk header to client and move the logic of saving chunks from ShardsManager to client.

[mzhangmzz]

Maybe we should move this function to ShardsManger too?

[robin-near]

Yeah, I just don't wanna do too much in this PR yet. So... kinda pausing at an arbitrary point.

[mzhangmzz]

I think this function is not used right now

[mzhangmzz]

Adding @matklad as a reviewer too

[robin-near]

Thanks! How would we invoke on_chunk_complete not through actor messages? We need a &mut Client for that call...

[matklad]

Do we have some broader context here? What is the end-goal of the refactor and such?

[matklad]

Let's maybe hide actix from the API here?

diff --git a/chain/chunks/src/client.rs b/chain/chunks/src/client.rs
index ed2b2ba58..e207c0cd1 100644
--- a/chain/chunks/src/client.rs
+++ b/chain/chunks/src/client.rs
@@ -2,12 +2,18 @@ use actix::Message;
 use near_network::types::MsgRecipient;
 use near_primitives::sharding::ShardChunkHeader;
 
+pub trait ClientAdapterForShardsManager {
+    fn did_complete_chunk(&self, chunk_header: ShardChunkHeader);
+}
+
 #[derive(Message)]
 #[rtype(result = "()")]
 pub enum ShardsManagerResponse {
     ChunkCompleted(ShardChunkHeader),
 }
 
-pub trait ClientAdapterForShardsManager: MsgRecipient<ShardsManagerResponse> {}
-
-impl<A: MsgRecipient<ShardsManagerResponse>> ClientAdapterForShardsManager for A {}
+impl<A: MsgRecipient<ShardsManagerResponse>> ClientAdapterForShardsManager for A {
+    fn did_complete_chunk(&self, chunk_header: ShardChunkHeader) {
+        self.do_send(ShardsManagerResponse::ChunkCompleted(chunk_header))
+    }
+}
diff --git a/chain/chunks/src/lib.rs b/chain/chunks/src/lib.rs
index 990d7cbee..b376f353d 100644
--- a/chain/chunks/src/lib.rs
+++ b/chain/chunks/src/lib.rs
@@ -85,7 +85,7 @@ use std::sync::Arc;
 use std::time::{Duration, Instant};
 
 use chrono::DateTime;
-use client::{ClientAdapterForShardsManager, ShardsManagerResponse};
+use client::ClientAdapterForShardsManager;
 use near_primitives::time::Utc;
 use rand::seq::IteratorRandom;
 use rand::seq::SliceRandom;
@@ -1967,7 +1967,7 @@ impl ShardsManager {
         self.encoded_chunks.remove_from_cache_if_outside_horizon(chunk_hash);
         self.requested_partial_encoded_chunks.remove(chunk_hash);
         debug!(target: "chunks", "Completed chunk {:?}", chunk_hash);
-        self.client_adapter.do_send(ShardsManagerResponse::ChunkCompleted(header));
+        self.client_adapter.did_complete_chunk(header);
     }
 
     /// Send the parts of the partial_encoded_chunk that are owned by `self.me` to the

In generally we want to move away from it, so better if stuff doesn't call actix APIs directly

[robin-near]

Thanks @matklad; @mzhangmzz, is this the same thing you were talking about? It doesn't solve the fact that this will go on the same client actor queue though. I don't see how to avoid that...

[robin-near]

Let me do this for the next PR; I need to change the API anyway.

[mzhangmzz]

Do we have some broader context here? What is the end-goal of the refactor and such?

@matklad The end goal is to move ShardsManager to its own thread(actor)

[matklad]

Is there some issue on that, so that we can mention it in the RP description, to make sure it gets surfaced in git blame in the future?

[mzhangmzz]

Is there some issue on that, so that we can mention it in the RP description, to make sure it gets surfaced in git blame in the future?

Good point. @robin-near do you mind creating a github and a jira issue to track this project?

[mzhangmzz]

Could you also run nayduck tests? Since this PR touches quite a few things