Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove #[derive(Clone)] from Synchronizer #4161

Merged

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Sep 17, 2023

What problem does this PR solve?

I submit this PR for 2 reason:

  1. I think there should be only one Synchronizer instance in CKB process. And struct BlockFetchCMD should depend on SyncState instead of Synchronizer
  2. In Asynchronous Block Download and Verification #3958 , when ChainService verify a failed block, ChainService should transfer the VerifyFailedBlockInfo to Synchronizer, then Synchronizer must ban the malicious peer.

So I want to add a new field: tokio::sync::mpsc::UnboundedReceiver<VerifyFailedBlockInfo> field to the Synchronizer struct:
https://github.com/nervosnetwork/ckb/pull/3958/files#diff-66d6800da6ed28c35f087178b6e0f1b31d6fea529dd97a86f6b5ce5dda26bbfcR884-R903

But UnboundedReceiver cannot be cloned, and UnboundedReceiver's fn recv(&mut self) method need mutable self. Then I have to use Arc<Mutex< ... >> to wrap UnboundedReceiver, this is not good:

#[derive(Clone)]
pub struct Synchronizer {
    ...

    pub(crate) verify_failed_blocks_rx:
        Arc<Mutex<tokio::sync::mpsc::UnboundedReceiver<VerifyFailedBlockInfo>>>,
}

...

#[async_trait]
impl CKBProtocolHandler for Synchronizer {
    async fn poll(&mut self, nc: Arc<dyn CKBProtocolContext + Sync>) -> Option<()> {
        let mut have_malformed_peers = false;
        while let Some(malformed_peer_info) = self.verify_failed_blocks_rx.lock().recv().await {
            // ban peer
        }
    }
}

By removing #[derive(Clone)] attribute from Synchronizer, this code can be improved to this:

/// we remove derive Cone
pub struct Synchronizer {
   ...

    pub(crate) verify_failed_blocks_rx: tokio::sync::mpsc::UnboundedReceiver<VerifyFailedBlockInfo>,
}


#[async_trait]
impl CKBProtocolHandler for Synchronizer {
    async fn poll(&mut self, nc: Arc<dyn CKBProtocolContext + Sync>) -> Option<()> {
        let mut have_malformed_peers = false;
        while let Some(malformed_peer_info) = self.verify_failed_blocks_rx.recv().await {            
            // ban peer
        }
    }
}

What is changed and how it works?

What's Changed:

Related changes

  • Remove #[derive(Clone)] from Synchronizer
  • Make BlockFetchCMD depend on SyncState instead of Synchronizer. This means that the initialization of BlockFetchCMD will no longer require cloning the Synchronizer.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec requested a review from a team as a code owner September 17, 2023 10:26
@eval-exec eval-exec requested review from quake and removed request for a team September 17, 2023 10:26
@eval-exec eval-exec force-pushed the exec/remove-Synchronizer-Clone branch from 85885b9 to b65176e Compare September 17, 2023 10:27
@eval-exec eval-exec added the help wanted Extra attention is needed label Sep 17, 2023
@eval-exec eval-exec force-pushed the exec/remove-Synchronizer-Clone branch from b65176e to 874f377 Compare September 17, 2023 10:39
@zhangsoledad zhangsoledad added this pull request to the merge queue Sep 20, 2023
Merged via the queue into nervosnetwork:develop with commit 922516e Sep 20, 2023
36 checks passed
@doitian doitian mentioned this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants