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

Unify code_at logic between CallExecutor & Client #4618

Merged
merged 10 commits into from
Jun 18, 2024
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented May 28, 2024

This unifies the logic between CallExecutor and Client when it comes to fetching the code for a given block. The actual code depends on potential overrides/substitutes.

Besides that it changes the logic in the lookahead collator on which ValidationCodeHash it sends to the validator alongside the POV. We are now sending the code hash as found on the relay chain. This is done as the local node could run with an override which is compatible to the validation code on the relay chain, but has a different hash.

This unifies the logic between `CallExecutor` and `Client` when it comes
to fetching the `code` for a given block. The actual `code` depends on
potential overrides/substitutes.

Besides that it changes the logic in the lookahead collator on which
`ValidationCodeHash` it sends to the validator alongside the `POV`. We
are now sending the code hash as found on the relay chain. This is done
as the local node could run with an override which is compatible to the
validation code on the relay chain, but has a different hash.
@bkchr bkchr added T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus. labels May 28, 2024
if state != *local_validation_code_hash {
tracing::warn!(
target: super::LOG_TARGET,
%para_id,
?relay_parent,
?local_validation_code_hash,
relay_validation_code_hash = ?state,
"Parachain code doesn't match validation code stored in the relay chain state",
"Parachain code doesn't match validation code stored in the relay chain state. This is expected if you are using an override for example.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected actually? If we look at how lookahead collator is typically instantiated, we pass client.code_at as code_hash_provider like here:

code_hash_provider: move |block_hash| {
client.code_at(block_hash).ok().map(|c| ValidationCode::from(c).hash())
},

With your changes in client.rs this will already take substitutes and overrides into account. The hash should match then and we would not even need to return the code hash from the relay chain, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are using a override, you will never find the hash of the override in the relay chain state. However, the relay chain would reject your pov if you send it with the hash of you override, as it doesn't know this hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

So what is not super clear to me is what is the point of this check here except the logging message.

We fetch the code hash from the relay chain here and then submit it to the relay chain along with the collation. Previously if there is some other reason for a code hash mismatch, it would be caught. Can we not check if there is an override or substitute in place and only in that case return the relay chain code hash here?

Copy link
Member Author

Choose a reason for hiding this comment

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

So what is not super clear to me is what is the point of this check here except the logging message.

The entire purpose of this function was always to just log. There is nothing we can do, besides informing the human that something is wrong.

Previously if there is some other reason for a code hash mismatch, it would be caught.

Not sure what other reason you mean, but we would still catch it. I mean even if the parachain is using a completely different code in its state, it will be printed. There is no real difference to before. The only difference is that we tell the relay chain what it expects from us (a little bit like cheating). However, the worst case would be that the relay chain rejects the Pov.


crates:
- name: cumulus-client-consensus-aura
bump: patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this changes behaviour I am in favor of minor.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6485817

pub fn code_at(
&self,
block: Block::Hash,
ignore_overrides: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

The ignore_overrides seems to always be true here if I'm looking at it correctly, so maybe remove this argument and rename the function code_at_ignoring_overrides?

Comment on lines +31 to +36
pub struct CodeProvider<Block: BlockT, Backend, Executor> {
backend: Arc<Backend>,
executor: Arc<Executor>,
wasm_override: Arc<Option<WasmOverride>>,
wasm_substitutes: WasmSubstitutes<Block, Executor, Backend>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering there are three Arcs in here it'd be nicer to maybe do something like this:

struct CodeProviderInner<...> {
    backend: Backend,
    executor: Executor,
    ...
}

struct CodeProvider<...>(Arc(CodeProviderImpl<...>));

use substrate_test_runtime_client::{runtime, GenesisInit};

#[test]
fn should_get_override_if_exists() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe would be nice to also have a test if there's no override to trigger the passthrough?

@bkchr bkchr enabled auto-merge June 18, 2024 11:14
@bkchr bkchr added this pull request to the merge queue Jun 18, 2024
Merged via the queue into master with commit 029a656 Jun 18, 2024
149 of 157 checks passed
@bkchr bkchr deleted the bkchr-code-provider branch June 18, 2024 12:28
pandres95 pushed a commit to virto-network/polkadot-sdk that referenced this pull request Jul 3, 2024
)

This unifies the logic between `CallExecutor` and `Client` when it comes
to fetching the `code` for a given block. The actual `code` depends on
potential overrides/substitutes.

Besides that it changes the logic in the lookahead collator on which
`ValidationCodeHash` it sends to the validator alongside the `POV`. We
are now sending the code hash as found on the relay chain. This is done
as the local node could run with an override which is compatible to the
validation code on the relay chain, but has a different hash.
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
)

This unifies the logic between `CallExecutor` and `Client` when it comes
to fetching the `code` for a given block. The actual `code` depends on
potential overrides/substitutes.

Besides that it changes the logic in the lookahead collator on which
`ValidationCodeHash` it sends to the validator alongside the `POV`. We
are now sending the code hash as found on the relay chain. This is done
as the local node could run with an override which is compatible to the
validation code on the relay chain, but has a different hash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants