Do not call `initialize_block` before any runtime api by bkchr · Pull Request #8953 · paritytech/substrate

bkchr · 2021-05-31T07:34:24Z

Before this change we always called initialize_block before calling
into the runtime. There was already support with skip_initialize to skip
the initialization. Almost no runtime_api requires that
initialize_block is called before. Actually this only leads to higher
execution times most of the time, because all runtime modules are
initialized and this is especially expensive when the block contained a
runtime upgrade.

TLDR: Do not call initialize_block before calling a runtime api.

Fixes: paritytech/polkadot#3053

polkadot companion: paritytech/polkadot#3140

Before this change we always called `initialize_block` before calling into the runtime. There was already support with `skip_initialize` to skip the initialization. Almost no runtime_api requires that `initialize_block` is called before. Actually this only leads to higher execution times most of the time, because all runtime modules are initialized and this is especially expensive when the block contained a runtime upgrade. TLDR: Do not call `initialize_block` before calling a runtime api.

…lize

tomusdrw

Looks good, but I'm surprised there is no docs being updated. I think it's worth to document the behavior now.

tomusdrw · 2021-05-31T08:59:45Z

-/// Method is executed using passed header as environment' current block.
 /// Proof should include both environment preparation proof and method execution proof.
-pub fn check_execution_proof_with_make_header<Header, E, H, MakeNextHeader>(
+pub fn check_execution_proof_with_make_header<Header, E, H>(


The method name doesn't make sense now?

tomusdrw · 2021-05-31T09:00:38Z

 		H::Out: Ord + codec::Codec + 'static,
 {
-	check_execution_proof_with_make_header::<Header, E, H, _>(
+	check_execution_proof_with_make_header::<Header, E, H>(


Doesn't this break compatibility between proof formats? Are there no consequences?

…lize

bkchr · 2021-06-29T12:15:11Z

@pepyakin @tomusdrw burn in etc was all successful. So, now it is time to do a proper review.

tomusdrw

Awesome work!

tomusdrw · 2021-06-30T10:51:00Z

 		/// The implementation should make sure to verify the correctness of the transaction
-		/// against current state.
+		/// against current state. The given `block_hash` corresponds to the hash of the block
+		/// that is used as current state.


Shall we add info about the need to manually initialize the frame_system pallet?

For frame this is done automatically through Executive::validate_transaction. While this is also just required when you have a signed extension that checks for block hashes.

tomusdrw · 2021-06-30T10:51:20Z

 	}
+
+	/// Returns the api version found for api with `id`.
+	pub fn api_version(&self, id: &ApiId) -> Option<u32> {


pepyakin · 2021-07-01T14:47:24Z

Prior this patch, when an runtime API is called we would call initialize_block. In turn, it would call on_initialize of all modules. pallet_session is of special interest, since it would invoke SessionHandler::on_new_session as part of its on_initialize. With this patch, these routines would not be executed.

Specifically, a difference in behavior will be observed if an runtime API accesses some state that can be changed either in on_initialize or on_new_session.

I went through all the runtime API impls and have marked some that do that. Getting into the list doesn't mean that they are wrong, but I think it would be a good idea to check for somebody deeply familiar with the respective code, or at least acknowledge that they are aware of this change.

Here is the list:

BabeApi @andresilva . Runtime APIs access state which is tainted by do_initialize and enact_epoch_change.
This won't be a big concern, as far as I can tell. Runtime APIs are called with no digests and thus do_initialize does nothing (except randomness collection), should_epoch_change returns false and thus no enact_epoch_change.
Equivocation proof generation and verification @andresilva . AFAICT, this should be fine since we always generate a proper proof. That enables it to be verified in any case.
Beefy @tomusdrw @adoerr . This one seems can change authorities on new session and thus the validator_set API will return a different set at the last block. As I mentioned for BABE it may be fine since it appropriates ShouldEndSession.
AuthorityDiscoveryApi @ordian . On new session this module rotates the keys.

pepyakin

Regarding the patch itself, LGTM!

tomusdrw · 2021-07-01T15:01:25Z

Thanks @pepyakin!
I'm crossing-out BEEFY from that list. We are planning to rely on the digest item anyway, currently afair we indeed do the state call and it might be an issue if we have BEEFY validators running two different versions (the ones running older version will think we already transitioned to a new set), but given it's currently deployed only on Rococo it should be easy to coordinate the upgrade, so I don't think any preventive measures are needed.

bkchr · 2021-07-01T15:36:55Z

I checked again Babe and AuthorityDiscovery and couldn't find anything problematic.

bkchr · 2021-07-01T15:50:37Z

bot merge

ghost · 2021-07-01T15:50:41Z

Trying merge.

andresilva · 2021-07-06T12:16:06Z

Both BABE and GRANDPA should be unaffected, I wasn't sure about the equivocation report submission but I tested it now.

* Do not call `initialize_block` before any runtime api Before this change we always called `initialize_block` before calling into the runtime. There was already support with `skip_initialize` to skip the initialization. Almost no runtime_api requires that `initialize_block` is called before. Actually this only leads to higher execution times most of the time, because all runtime modules are initialized and this is especially expensive when the block contained a runtime upgrade. TLDR: Do not call `initialize_block` before calling a runtime api. * Change `validate_transaction` interface * Fix rpc test * Fixes and comments * Some docs

JoshOrndorff · 2021-07-08T19:57:20Z

What is the recommended way to port runtime APIs that actually did depend on the initialization? Should each one manually initialize only what it needs to?

cc @tgmichel I believe this is the change that broke our EVM tracing.

shawntabrizi · 2021-07-08T20:03:03Z

@JoshOrndorff not super familiar with how one might initialize the block, but it may be that rather than trying to turn this back on, you actually want to see if you can adjust your code or tracing to work without needing this initialize.

I think in general, it is risky to your chain's performance if you have any of the runtime apis call initialize_block because some blocks can be very heavy, and it seems your chain may be attackable on those blocks

(afaik, not an expert on this)

bkchr · 2021-07-08T22:15:13Z

Yeah, I would not advise to do this.

If possible, I would prevent this. Or if you only need to initialize some stuff like System, you can only initialize this.

crystalin · 2021-07-08T22:59:01Z

@shawntabrizi Thanks for the feedback.
Just a bit of context, we understand those are a concern for resources (just the fact of fully tracing blocks is very expensive), and that is why we don't expose those features by default. Those are meant for projects to use internally to query the trace logs.

* Do not call `initialize_block` before any runtime api Before this change we always called `initialize_block` before calling into the runtime. There was already support with `skip_initialize` to skip the initialization. Almost no runtime_api requires that `initialize_block` is called before. Actually this only leads to higher execution times most of the time, because all runtime modules are initialized and this is especially expensive when the block contained a runtime upgrade. TLDR: Do not call `initialize_block` before calling a runtime api. * Change `validate_transaction` interface * Fix rpc test * Fixes and comments * Some docs

JoshOrndorff · 2021-08-26T08:17:26Z

Before this PR was merged, was each pallet's on_initialize hook called for each runtime API?

bkchr · 2021-08-26T08:53:29Z

Yes

* Do not call `initialize_block` before any runtime api Before this change we always called `initialize_block` before calling into the runtime. There was already support with `skip_initialize` to skip the initialization. Almost no runtime_api requires that `initialize_block` is called before. Actually this only leads to higher execution times most of the time, because all runtime modules are initialized and this is especially expensive when the block contained a runtime upgrade. TLDR: Do not call `initialize_block` before calling a runtime api. * Change `validate_transaction` interface * Fix rpc test * Fixes and comments * Some docs Signed-off-by: Vladislav Markushin <negigic@gmail.com>

bkchr added 2 commits May 28, 2021 23:14

Merge remote-tracking branch 'origin/master' into bkchr-api-no-initia…

40fbd35

…lize

bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels May 31, 2021

bkchr requested a review from tomusdrw as a code owner May 31, 2021 07:34

bkchr added D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B5-clientnoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels May 31, 2021

github-actions Bot added the A7-needspolkadotpr label May 31, 2021

tomusdrw reviewed May 31, 2021

View reviewed changes

pepyakin self-requested a review May 31, 2021 17:36

tomaka mentioned this pull request May 31, 2021

smoldot doesn't call initialize_block paritytech/smoldot#877

Closed

Change validate_transaction interface

fe3b52d

bkchr requested a review from NikVolf as a code owner June 2, 2021 20:21

bkchr added 5 commits June 3, 2021 10:46

Fix rpc test

f36f793

Merge remote-tracking branch 'origin/master' into bkchr-api-no-initia…

8eeb841

…lize

Fixes and comments

dab115b

Merge remote-tracking branch 'origin/master' into bkchr-api-no-initia…

35d66c5

…lize

Some docs

7302b96

bkchr requested a review from tomusdrw June 29, 2021 12:14

pepyakin reviewed Jun 29, 2021

View reviewed changes

Comment thread frame/merkle-mountain-range/primitives/src/lib.rs

Comment thread primitives/consensus/aura/src/lib.rs

tomusdrw approved these changes Jun 30, 2021

View reviewed changes

pepyakin approved these changes Jul 1, 2021

View reviewed changes

ghost merged commit baf3736 into master Jul 1, 2021

ghost deleted the bkchr-api-no-initialize branch July 1, 2021 15:50

adoerr mentioned this pull request Jul 2, 2021

Bump Substrate and Deps paritytech/grandpa-bridge-gadget#226

Merged

JoshOrndorff mentioned this pull request Jul 8, 2021

Update substrate/polkadot/cumulus from v0.9.7 to v0.9.8 moonbeam-foundation/moonbeam#586

Closed

kianenigma mentioned this pull request Jul 9, 2021

Vision: Execute a hook after inherent but before transactions paritytech/polkadot-sdk#312

Open

tgmichel mentioned this pull request Jul 9, 2021

Update dependencies to v0.9.8 series moonbeam-foundation/moonbeam#587

Merged

tomaka mentioned this pull request Aug 9, 2021

Backport update to validate_transaction runtime API paritytech/smoldot#1106

Closed

JoshOrndorff mentioned this pull request Aug 23, 2021

Make --force-authoring skip slot prediction moonbeam-foundation/moonbeam#722

Merged

github-actions Bot mentioned this pull request Aug 24, 2021

Update substrate/polkadot/cumulus from v0.9.8 to v0.9.9 moonbeam-foundation/moonbeam#729

Closed

JoshOrndorff mentioned this pull request Aug 26, 2021

Run collator election during slot prediction moonbeam-foundation/moonbeam#744

Merged

Centril mentioned this pull request Oct 28, 2021

Interesting in substrate since last time PolymeshAssociation/Polymesh#1151

Closed

This pull request was closed.

Uh oh!

Conversation

bkchr commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomusdrw May 31, 2021

Choose a reason for hiding this comment

Uh oh!

tomusdrw May 31, 2021

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jun 29, 2021

Uh oh!

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

bkchr Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

pepyakin commented Jul 1, 2021 • edited by bkchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepyakin left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Jul 1, 2021

Uh oh!

bkchr commented Jul 1, 2021

Uh oh!

bkchr commented Jul 1, 2021

Uh oh!

ghost commented Jul 1, 2021

Uh oh!

andresilva commented Jul 6, 2021

Uh oh!

JoshOrndorff commented Jul 8, 2021

Uh oh!

shawntabrizi commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Jul 8, 2021

Uh oh!

crystalin commented Jul 8, 2021

Uh oh!

JoshOrndorff commented Aug 26, 2021

Uh oh!

bkchr commented Aug 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bkchr commented May 31, 2021 •

edited

Loading

pepyakin commented Jul 1, 2021 •

edited by bkchr

Loading

shawntabrizi commented Jul 8, 2021 •

edited

Loading