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

Patch moonbase block #3034

Closed
wants to merge 3 commits into from
Closed

Conversation

RomarQ
Copy link
Contributor

@RomarQ RomarQ commented Nov 6, 2024

What does it do?

This PR fixes a problem that was caused by a change in frontier, which moved Pending::<T>::kill() from on_initialize to on_finalize: polkadot-evm/frontier#638

The bug was included with runtime 1603 in block 2285347, which caused block 2285348 to include transactions from the previous block.

What important points reviewers should know?

This is a client specific fix, which just checks if the block being queried is the one affected and filters out the invalid transactions.

Is there something left for follow-up PRs?

  • Add smoke test

@RomarQ RomarQ added the B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes label Nov 6, 2024
@RomarQ RomarQ self-assigned this Nov 6, 2024
@RomarQ RomarQ added breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. D2-notlive PR doesn't change runtime code (so can't be audited) labels Nov 6, 2024
@crystalin
Copy link
Collaborator

@RomarQ I think we decided not to fix it as it required a permanent patch and wasn't worth it.
Do you think it is worth? What was the reason you need it?

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 7, 2024

@RomarQ I think we decided not to fix it as it required a permanent patch and wasn't worth it. Do you think it is worth? What was the reason you need it?

A user opened an issue about it: #3026, it seems to be causing issues in their application.

@crystalin
Copy link
Collaborator

Ok, Can you evaluate the cost of maintaining this patch long term vs just maintaining that knowledge (and replying to possible rising issues in the future) ?
(It might already be included in our inconsistant state doc: https://docs.moonbeam.network/builders/build/historical-updates/)

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 7, 2024

Ok, Can you evaluate the cost of maintaining this patch long term vs just maintaining that knowledge (and replying to possible rising issues in the future) ? (It might already be included in our inconsistant state doc: https://docs.moonbeam.network/builders/build/historical-updates/)

Yes, it is documented there already: https://docs.moonbeam.network/builders/build/historical-updates/#ethereum-transactions-duplicated-in-storage

Since the problem cannot be solved entirely without an hard-fork, I think it will be better to just keep the issue documented and expect people to handle these inconsistencies on their side.

@RomarQ RomarQ closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes breaking Needs to be mentioned in breaking changes D2-notlive PR doesn't change runtime code (so can't be audited) D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants