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. #9006 - (124) #10671

Closed
wants to merge 1 commit into from

Conversation

rutkaracn
Copy link

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.

@bors-diem bors-diem added this to In Review in bors Dec 28, 2022
@ankitkacn
Copy link
Contributor

/canary

@bors-diem bors-diem moved this from In Review to Canary in bors Dec 28, 2022
bors-diem pushed a commit that referenced this pull request Dec 28, 2022
@bors-diem
Copy link
Contributor

💔 Test Failed - ci-test

@bors-diem bors-diem moved this from Canary to In Review in bors Dec 28, 2022
@ankitkacn
Copy link
Contributor

/canary

bors-diem pushed a commit that referenced this pull request Dec 28, 2022
@bors-diem bors-diem moved this from In Review to Canary in bors Dec 28, 2022
@bors-diem
Copy link
Contributor

💔 Test Failed - ci-test

@bors-diem bors-diem moved this from Canary to In Review in bors Dec 28, 2022
@ankitkacn
Copy link
Contributor

/canary

@bors-diem bors-diem moved this from In Review to Canary in bors Dec 28, 2022
bors-diem pushed a commit that referenced this pull request Dec 28, 2022
@bors-diem
Copy link
Contributor

☀️ Canary successful

@bors-diem bors-diem moved this from Canary to In Review in bors Dec 28, 2022
@satyamacn
Copy link
Contributor

/land

@bors-diem bors-diem moved this from In Review to Queued in bors Dec 28, 2022
@bors-diem bors-diem moved this from Queued to Testing in bors Dec 28, 2022
bors-diem pushed a commit that referenced this pull request Dec 28, 2022
@bors-diem
Copy link
Contributor

💔 Test Failed - ci-test

@bors-diem bors-diem moved this from Testing to In Review in bors Dec 28, 2022
@ankitkacn
Copy link
Contributor

Closing this PR due to technical issue, raised new one #10683

@ankitkacn ankitkacn closed this Feb 21, 2023
@bors-diem bors-diem removed this from In Review in bors Feb 21, 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

6 participants