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

[State Sync] Add MempoolNotifier interface and implementation - (133) #10683

Merged
merged 8 commits into from Jan 4, 2023

Conversation

ankitkacn
Copy link
Contributor

Motivation

Today, state sync is responsible for notifying mempool whenever new transactions are committed. However, the interface between the two components is very messy, e.g., the code leaks implementation details between the components and forces a very tight coupling. This makes it difficult to update (or swap out) component implementations.

To address this, the PR introduces an explicit interface and set of components between state sync and mempool:

MempoolNotificationSender is the interface allowing state sync to send notifications to mempool (MempoolNotifier is the component that implements this interface).
MempoolNotificationListener is the component that mempool uses to listen for notifications and respond accordingly.
This structure avoids leaking implementation details (e.g., the fact that we use a one-shot callback to respond to notifications). In addition, the PR adds explicit (previously missing) tests for the implementation.

Note: at a fundamental level, this new interface implementation still uses channels, but wraps the interface-specific details of the communication (e.g., message formats, channel timeouts, etc.) to offer better implementation encapsulation.

Test Plan

  • CI/CD testcases were covered.

[State Sync] Small cleanups to MempoolNotifier. #9047=

Motivation

This (mini) PR offers two small cleanups for the newly added MempoolNotifier (#9006), each in their own commit:

First, remove the redundant Result wrapper when the MempoolNotificationListener acks a commit notification. The result is not used.
Second, it removes the need for mutability when methods of the MempoolNotifierSender are invoked. Mutability is also not required.
Have you read the Contributing Guidelines on pull requests?
Yes.

Test Plan

  • CI/CD testcases were covered.

[State Sync] Add ConsensusNotifier interface and implementation. #9057

Motivation

Today, consensus is responsible for notifying state sync of two events: (i) when a set of transactions has just been committed; and (ii) when state sync should synchronize to a specified target. However, the interface between these components is very messy, leaks implementation details and is not thoroughly tested. As a result, it becomes difficult to update (or swap out) implementations.

To address this, the PR introduces an explicit interface and set of components between consensus and state sync:

ConsensusNotificationSender is the interface allowing consensus to send notifications to state sync (ConsesusNotifier is the component that implements this interface).
ConsensusNotificationListener is the component that state sync uses to listen for notifications and respond accordingly.
This structure avoids leaking implementation details. In addition, the PR adds explicit (previously missing) tests for the implementation. To achieve this, the PR offers the following commits:

First, we add the new ConsesusNotifier, ConsensusNotificationSender and ConsensusNotificationListener components and interfaces (alongside tests for these).
Second, we update consensus to use the ConsensusNotifier (instead of the current StateSyncClient) to notify state sync.
Third, we update state sync to use the ConsensusNotificationListener directly and update the StateSyncClient to remove support for the old consensus APIs.
Fourth, we update Diem node to wire consensus and state sync together using the ConsensusNotifier.
Finally, we remove the (newly) redundant code from mempool.
Note: This PR takes a very similar approach to: #9006

Have you read the Contributing Guidelines on pull requests?
Yes.

Test Plan

  • cargo test --package state-sync-v1 --tests

@bors-diem bors-diem added this to In Review in bors Jan 3, 2023
@satyamacn
Copy link
Contributor

/canary

@bors-diem bors-diem moved this from In Review to Canary in bors Jan 3, 2023
bors-diem pushed a commit that referenced this pull request Jan 3, 2023
@bors-diem bors-diem moved this from Canary to In Review in bors Jan 3, 2023
@bors-diem
Copy link
Contributor

☀️ Canary successful

@ankitkacn
Copy link
Contributor Author

/land

@bors-diem bors-diem moved this from In Review to Queued in bors Jan 4, 2023
@bors-diem bors-diem moved this from Queued to Testing in bors Jan 4, 2023
@bors-diem bors-diem removed this from Testing in bors Jan 4, 2023
@bors-diem bors-diem merged commit dc38b65 into diem:latest Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants