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

Improve block import notification strategy #1030

Conversation

tgmichel
Copy link
Contributor

We currently use substrate block import notification stream in our pubsub module, that can lead to a race condition if a Dapp subscribes to newHeads and quickly after requests for data that requires the mapping in our offchain db to exist (i.e. eth_getBlockByHash). This data may or may not exist at that moment. As in any race condition cannot be reliably reproduced but this behaviour has been reported to exist in Moonbeam several times.

This PR proposes to switch to use our own Ethereum block import notification stream, that is, send a message through a channel when data is already mapped in our offchain db and receive through the channel in the pubsub subscription.

@tgmichel tgmichel requested a review from sorpaas as a code owner March 24, 2023 15:52
Copy link
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please fix cargo lints.

@tgmichel
Copy link
Contributor Author

LGTM, please fix cargo lints.

Done, please wait to merge I'm going to push some change

@tgmichel tgmichel requested a review from sorpaas March 27, 2023 08:13
@tgmichel
Copy link
Contributor Author

tgmichel commented Mar 27, 2023

@sorpaas added functionality to only notify when the node is synced, mostly to mimic what substrate block import notification does.

What do you think of https://github.com/paritytech/frontier/pull/1030/files#diff-238ded0693b6a31db32cd68411cd48e079cd87097b3436fe7ef70b222b2eeb25R248-R249? do you think is reasonable to close the channel and remove it from memory when the node is in major syncing?

# Conflicts:
#	template/node/src/rpc/mod.rs
@sorpaas sorpaas merged commit eb34c78 into polkadot-evm:master May 8, 2023
ashutoshvarma pushed a commit to AstarNetwork/frontier that referenced this pull request May 29, 2023
* Improve block import notification strategy

* oops

* taplo

* clippy

* Notify only when not major syncing
ashutoshvarma pushed a commit to AstarNetwork/frontier that referenced this pull request Jun 8, 2023
* Improve block import notification strategy

* oops

* taplo

* clippy

* Notify only when not major syncing
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.

2 participants