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

Fix panic in pending_runtime_api with Aura consensus env (#814) #114

Merged

Conversation

tgmichel
Copy link

Rel MOON-1909

Copy link

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

This changes the error when the header of the best block cannot be unwrapped from:

for all runtime APIs
"Runtime api access error: {:?}", e

to

Cannot get header for block {:?}, block

@tgmichel
Copy link
Author

This changes the user error when the header cannot be unwrapped from:

for all runtime APIs
"Runtime api access error: {:?}", e

to

Cannot get header for block {:?}

Before we were just assuming the header could be unwrapped (wrong) and now handle that error specifically. Runtime api access error message still the same.

@tgmichel tgmichel merged commit afc693a into moonbeam-polkadot-v0.9.26 Sep 22, 2022
@4meta5
Copy link

4meta5 commented Sep 22, 2022

Thanks for explaining! As far as I understand

Before we were just assuming the header could be unwrapped (wrong) and now handle that error specifically.

We handle header not being unwrapped by returning the client error Cannot get header for block {:?}

Runtime api access error message still the same.

Now I think the runtime API access error is not emitted if the header cannot be unwrapped

@librelois librelois deleted the tgm-fix-pending-runtime-api-0.26.0 branch September 22, 2022 16:08
@@ -12,6 +12,7 @@ describeWithFrontier("Frontier RPC (Balance)", (context) => {
});

step("balance to be updated after transfer", async function () {
await createAndFinalizeBlock(context.web3);

Choose a reason for hiding this comment

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

This should go after the timeout line.
Also why do we need to create a block for this ?

Copy link
Author

Choose a reason for hiding this comment

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

This cherry-picked solution from upstream uses parent hash for building the pending runtime api and, in the case of tests like this one, parent hash of genesis is not a valid block to initialize. Thus the need of creating one.

Tbh I don't like it and I can propose a better way of doing it upstream, which will not require modifying the tests like that.

@4meta5
Copy link

4meta5 commented Sep 23, 2022

So this PR applies polkadot-evm#842 to our frontier fork

I'm still not sure what this has to do with pallet-randomness::on_initialize being introduced in 1802...

@tgmichel
Copy link
Author

So this PR applies polkadot-evm#842 to our frontier fork

Not really, or not only. This PR avoids the unwrap thing, but also builds the pending block over parent instead of best. And that's the main issue:

api.initialize_block(&best, &header) VS. api.initialize_block(&parent_hash, &header)

@4meta5
Copy link

4meta5 commented Sep 23, 2022

interesting, when is best != parent_hash? is it only when a node is syncing?

@tgmichel
Copy link
Author

when is best != parent_hash?

Always. best is the tip of the canonical chain, and in this case parent is best's parent.

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.

4 participants