Skip to content

[author-inherent] Use ensure!() to avoid storage transactions are left open by the runtime#91

Closed
arturgontijo wants to merge 1 commit intomainfrom
artur/author-inherent-ensure
Closed

[author-inherent] Use ensure!() to avoid storage transactions are left open by the runtime#91
arturgontijo wants to merge 1 commit intomainfrom
artur/author-inherent-ensure

Conversation

@arturgontijo
Copy link
Contributor

This PR replaces assert!() by ensure!() to avoid a panic during EthereumRuntimeRPCApi::initialize_pending_block().

In order to check this we need to run a moonbeam's zombienet with multiple collators (heads up for "-leth-pending=trace" too):
moonbeam/zombienet/configs/moonbeam-polkadot.toml

...
[[parachains.collators]]
name = "alith"
command = "moonbeam"
rpc_port = 8800
args = [
    "--no-hardware-benchmarks",
    "--no-telemetry",
    "-leth-pending=trace",
    "-lruntime::bridge-grandpa=trace",
    "--pool-type=fork-aware"
]

[[parachains.collators]]
name = "baltathar"
command = "moonbeam"
rpc_port = 8801
args = [
    "--no-hardware-benchmarks",
    "--no-telemetry",
    "-lruntime::bridge-grandpa=trace",
    "--pool-type=fork-aware"
]

[[parachains.collators]]
name = "charleth"
command = "moonbeam"
rpc_port = 8802
args = [
    "--no-hardware-benchmarks",
    "--no-telemetry",
    "-lruntime::bridge-grandpa=trace",
    "--pool-type=fork-aware"
]
...

Once parachain blocks are being produced send any eth_call(txn, "pending") to Alith's node, eg:

curl -X POST http://localhost:8800 \
  -H "Content-Type: application/json" \
  -d '{"jsonrpc":"2.0","method":"eth_call","params":[{"to":"0x0000000000000000000000000000000000000400"},"pending"],"id":1}'

So eventually we will get these warnings:

2026-01-27 17:36:30.134 DEBUG tokio-runtime-worker eth-pending: Pending runtime API: inherent len = 4    
2026-01-27 17:36:30.136  WARN tokio-runtime-worker sp_state_machine::overlayed_changes::changeset: 1 storage transactions are left open by the runtime. Those will be rolled back.    
2026-01-27 17:36:30.136  WARN tokio-runtime-worker sp_state_machine::overlayed_changes::changeset: 1 storage transactions are left open by the runtime. Those will be rolled back.    
2026-01-27 17:36:30.137 DEBUG tokio-runtime-worker eth-pending: Pending runtime API: extrinsic len = 0

Investigating what could cause those warnings I came up with the following panic stack:

2026-01-27 14:53:18.869 DEBUG tokio-runtime-worker eth-pending: Inherent failed in pending block: CallApi(Application(Execution(AbortedDueToPanic(MessageWithBacktrace { message: "panicked at /workspace/moonkit/pallets/author-inherent/src/lib.rs:146:4:\nBlock invalid, supplied author is not eligible.", backtrace: Some(Backtrace { backtrace_string: "error while executing at wasm backtrace: 
0: 0xb2ca4b - moonbeam_runtime.wasm!__rustc[4794b31dd7191200]::rust_begin_unwind 
1: 0x29a18 - moonbeam_runtime.wasm!core::panicking::panic_fmt::hd534225921b41838 
2: 0x381ddd - moonbeam_runtime.wasm!frame_support::storage::transactional::with_transaction::hecd296169b6f73ce 
3: 0x1f28e2 - moonbeam_runtime.wasm!environmental::local_key::LocalKey<T>::with::hc2fb25c52a181d48 
4: 0x199bb9 - moonbeam_runtime.wasm!<moonbeam_runtime::RuntimeCall as frame_support::traits::dispatch::UnfilteredDispatchable>::dispatch_bypass_filter::h9ce547e4cfaef82f 
5: 0x19918b - moonbeam_runtime.wasm!<moonbeam_runtime::RuntimeCall as sp_runtime::traits::Dispatchable>::dispatch::hef20103d705aac93 
6: 0x81b882 - moonbeam_runtime.wasm!<fp_self_contained::checked_extrinsic::CheckedExtrinsic<AccountId,Call,Extension,SelfContainedSignedInfo> as sp_runtime::traits::Applyable>::apply::hac98aada1c0039d6 
7: 0x8a6559 - moonbeam_runtime.wasm!frame_executive::Executive<System,Block,Context,UnsignedValidator,AllPalletsWithSystem,COnRuntimeUpgrade>::do_apply_extrinsic::h4a004c3059fcd45f 
8: 0x18e88e - moonbeam_runtime.wasm!<moonbeam_runtime::Runtime as sp_block_builder::runtime_decl_for_block_builder::BlockBuilderV6<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,fp_self_contained::unchecked_extrinsic::UncheckedExtrinsic<account::AccountId20,moonbeam_runtime::RuntimeCall,account::EthereumSignature,cumulus_pallet_weight_reclaim::StorageWeightReclaim<moonbeam_runtime::Runtime,(frame_system::extensions::check_non_zero_sender::CheckNonZeroSender<moonbeam_runtime::Runtime>,frame_system::extensions::check_spec_version::CheckSpecVersion<moonbeam_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<moonbeam_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<moonbeam_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<moonbeam_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<moonbeam_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<moonbeam_runtime::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<moonbeam_runtime::Runtime>,moonbeam_runtime::BridgeRejectObsoleteHeadersAndMessages,frame_metadata_hash_extension::CheckMetadataHash<moonbeam_runtime::Runtime>)>>>>>::apply_extrinsic::ha03d0ae2af5d6d33 
9: 0xa867d6 - moonbeam_runtime.wasm!BlockBuilder_apply_extrinsic" }) })))) 

After replacing the assert!() by ensure!() there were no more storage transactions being left open.

@arturgontijo arturgontijo self-assigned this Jan 27, 2026
manuelmauro
manuelmauro previously approved these changes Jan 28, 2026
Copy link
Contributor

@manuelmauro manuelmauro left a comment

Choose a reason for hiding this comment

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

lgtm!


// Now check that the author is valid in this slot
assert!(
ensure!(
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to panic here. The fix for the pending block inherents needs to be on the client side, when we provide the pending_consenus_data_provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch Rodri, shall we comment something like this here then:

Why assert! is critical here

  1. This is a mandatory inherent (DispatchClass::Mandatory at line 138). Mandatory inherents must be included in every block AND must succeed. If validation fails, the block itself is invalid.
  2. Block-level validity, not transaction-level - The comment explicitly says "Block invalid". If an ineligible author produced a block, we don't want that block to exist at all. Using ensure! would merely fail
  the inherent call while potentially allowing the block to be finalized.
  3. Security invariant - Author eligibility is a consensus-critical check. If this fails, something is fundamentally wrong (either a bug or an attack). The block must be rejected entirely, not just have a failed
   transaction in it.
  4. No recovery path - There's no sensible way to "handle" an invalid author gracefully. The block simply shouldn't exist.

and possibly add proper testing for this path?

@manuelmauro manuelmauro self-requested a review January 28, 2026 09:20
@manuelmauro manuelmauro dismissed their stale review January 28, 2026 09:21

Rodri highlighted a vulnerability

@arturgontijo
Copy link
Contributor Author

Close this as we need it to panic here.

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.

3 participants

Comments