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

[eth-indexer] subscribe to finalize blocks instead of best blocks #7260

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

pgherveou
Copy link
Contributor

For eth-indexer, it's probably safer to use subscribe_finalized and index these blocks into the DB rather than subscribe_best

@pgherveou
Copy link
Contributor Author

/cmd prdoc --audience runtime_dev --bump minor

@pgherveou pgherveou added T7-smart_contracts This PR/Issue is related to smart contracts. R0-silent Changes should not be mentioned in any release notes labels Jan 20, 2025
@pgherveou pgherveou marked this pull request as ready for review January 20, 2025 14:47
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Yeah makes sense. Otherwise we would probably need to deal with rolling back blocks.

@@ -324,7 +336,7 @@ impl Client {
let client = self.clone();
spawn_handle.spawn("subscribe-blocks", None, async move {
let res = client
.subscribe_new_blocks(|block| async {
.subscribe_new_blocks(SubscriptionType::BestBlocks, |block| async {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not using finalized here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that for the "in memory" cache maintained by the eth-rpc. In Ethereum you don't wait for the block to be finalized when you call the RPC, so I am not waiting for finalized blocks here. We could decide to wait for the finalized block but that would also impact the user's experience, you will see MM spinning for much longer.

Copy link
Member

Choose a reason for hiding this comment

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

Okay no its fine then. It wasn't apparent from the function names.

@athei athei added this pull request to the merge queue Jan 20, 2025
Merged via the queue into master with commit cbf3925 Jan 20, 2025
202 of 210 checks passed
@athei athei deleted the pg/eth-indexer-sub-finalized branch January 20, 2025 23:35
Nathy-bajo pushed a commit to Nathy-bajo/polkadot-sdk that referenced this pull request Jan 21, 2025
…ritytech#7260)

For eth-indexer, it's probably safer to use `subscribe_finalized` and
index these blocks into the DB rather than `subscribe_best`

---------

Co-authored-by: command-bot <>
Nathy-bajo pushed a commit to Nathy-bajo/polkadot-sdk that referenced this pull request Jan 21, 2025
…ritytech#7260)

For eth-indexer, it's probably safer to use `subscribe_finalized` and
index these blocks into the DB rather than `subscribe_best`

---------

Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T7-smart_contracts This PR/Issue is related to smart contracts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants