frame-executive: Reject invalid inherents in the executive #12365

bkchr · 2022-09-27T11:20:18Z

We already had support for making a block fail if an inherent returned, but it was part of the signed extension CheckWeight. Rejecting blocks with invalid inherents should happen on the frame-executive level without requiring any special signed extension. This is crucial to prevent any kind of spamming of the network that could may happen with blocks that include failing inherents.

We already had support for making a block fail if an inherent returned, but it was part of the signed extension `CheckWeight`. Rejecting blocks with invalid inherents should happen on the `frame-executive` level without requiring any special signed extension. This is crucial to prevent any kind of spamming of the network that could may happen with blocks that include failing inherents.

muharem · 2022-11-07T15:25:01Z

Just verifying my understanding,

every signed or unsigned transactions added to the pool has to be valid for TaggedTransactionQueue::validate_transaction (no inherents expected)
inherents added by an author to the block when its being build, using BlockBuilder, and checking them with BlockBuilder::check_inherents.

trying to find where this all orchestrated in the codebase, somewhere on the node level

bkchr · 2022-11-16T18:13:13Z

every signed or unsigned transactions added to the pool has to be valid for TaggedTransactionQueue::validate_transaction (no inherents expected)

Exactly! Inherents will normally be rejected by validate_transaction.

inherents added by an author to the block when its being build, using BlockBuilder, and checking them with BlockBuilder::check_inherents.

check_inherents is only called by nodes on importing a block, not while producing a block.

bkchr · 2022-11-25T14:57:51Z

bot rebase

…cks-move

paritytech-processbot · 2022-11-25T14:57:58Z

Rebased

frame/executive/src/lib.rs

primitives/runtime/src/transaction_validity.rs

Co-authored-by: Keith Yeung <[email protected]>

kianenigma · 2022-11-30T02:58:17Z

frame/executive/src/lib.rs

+		//
+		// The entire block should be discarded if an inherent fails to apply. Otherwise
+		// it may open an attack vector.
+		if r.is_err() && dispatch_info.class == DispatchClass::Mandatory {


I hope there's some doc on top of DispatchClass::Mandatory saying this can only be applied to inherents.

Perhaps also a doc in the frame macros saying you cannot annotate any transaction with DispatchClass::Mandatory.

Or, we should just call this DispatchClass::Inherent 🤷

Kinda confusing now.

substrate/frame/support/src/dispatch.rs

Lines 145 to 158 in 78164d9

/// A mandatory dispatch. These kinds of dispatch are always included regardless of their

/// weight, therefore it is critical that they are separately validated to ensure that a

/// malicious validator cannot craft a valid but impossibly heavy block. Usually this just

/// means ensuring that the extrinsic can only be included once and that it is always very

/// light.

///

/// Do *NOT* use it for extrinsics that can be heavy.

///

/// The only real use case for this is inherent extrinsics that are required to execute in a

/// block for the block to be valid, and it solves the issue in the case that the block

/// initialization is sufficiently heavy to mean that those inherents do not fit into the

/// block. Essentially, we assume that in these exceptional circumstances, it is better to

/// allow an overweight block to be created than to not allow any block at all to be created.

Mandatory,

looks good or? :D

bkchr · 2022-12-04T19:19:42Z

bot rebase

…cks-move

paritytech-processbot · 2022-12-04T19:19:50Z

Rebased

bkchr · 2022-12-04T19:20:07Z

bot merge

paritytech-processbot · 2022-12-04T19:20:14Z

Waiting for commit status.

…h#12365) * frame-executive: Reject invalid inherents in the executive We already had support for making a block fail if an inherent returned, but it was part of the signed extension `CheckWeight`. Rejecting blocks with invalid inherents should happen on the `frame-executive` level without requiring any special signed extension. This is crucial to prevent any kind of spamming of the network that could may happen with blocks that include failing inherents. * FMT * Update frame/executive/src/lib.rs Co-authored-by: Keith Yeung <[email protected]> * Update primitives/runtime/src/transaction_validity.rs Co-authored-by: Keith Yeung <[email protected]> Co-authored-by: parity-processbot <> Co-authored-by: Keith Yeung <[email protected]>

FMT

3fa4620

muharem approved these changes Nov 7, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into bkchr-mandatory-che…

f153fab

…cks-move

bkchr requested review from KiChjang, ggwpez, kianenigma and shawntabrizi November 25, 2022 14:59

ggwpez approved these changes Nov 25, 2022

View reviewed changes

KiChjang reviewed Nov 26, 2022

View reviewed changes

frame/executive/src/lib.rs Outdated Show resolved Hide resolved

KiChjang reviewed Nov 26, 2022

View reviewed changes

primitives/runtime/src/transaction_validity.rs Outdated Show resolved Hide resolved

KiChjang approved these changes Nov 26, 2022

View reviewed changes

bkchr and others added 2 commits November 26, 2022 10:22

Update frame/executive/src/lib.rs

3edcfae

Co-authored-by: Keith Yeung <[email protected]>

Update primitives/runtime/src/transaction_validity.rs

c0dce71

Co-authored-by: Keith Yeung <[email protected]>

kianenigma reviewed Nov 30, 2022

View reviewed changes

kianenigma approved these changes Nov 30, 2022

View reviewed changes

louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 1, 2022

Merge remote-tracking branch 'origin/master' into bkchr-mandatory-che…

69f093f

…cks-move

paritytech-processbot bot merged commit cb3eaf2 into master Dec 4, 2022

paritytech-processbot bot deleted the bkchr-mandatory-checks-move branch December 4, 2022 19:40

aurexav mentioned this pull request Jan 10, 2023

To polkadot-v0.9.36 darwinia-network/darwinia-2.0#193

Closed

kacperzuk-neti mentioned this pull request Jan 23, 2023

Update to polkadot 0.9.34 liberland/liberland_substrate#225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

frame-executive: Reject invalid inherents in the executive #12365

frame-executive: Reject invalid inherents in the executive #12365

Uh oh!

bkchr commented Sep 27, 2022

Uh oh!

muharem commented Nov 7, 2022

Uh oh!

bkchr commented Nov 16, 2022

Uh oh!

bkchr commented Nov 25, 2022

Uh oh!

paritytech-processbot bot commented Nov 25, 2022

Uh oh!

Uh oh!

Uh oh!

kianenigma Nov 30, 2022

Uh oh!

bkchr Nov 30, 2022

Uh oh!

bkchr commented Dec 4, 2022

Uh oh!

paritytech-processbot bot commented Dec 4, 2022

Uh oh!

bkchr commented Dec 4, 2022

Uh oh!

paritytech-processbot bot commented Dec 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	/// A mandatory dispatch. These kinds of dispatch are always included regardless of their
	/// weight, therefore it is critical that they are separately validated to ensure that a
	/// malicious validator cannot craft a valid but impossibly heavy block. Usually this just
	/// means ensuring that the extrinsic can only be included once and that it is always very
	/// light.
	///
	/// Do NOT use it for extrinsics that can be heavy.
	///
	/// The only real use case for this is inherent extrinsics that are required to execute in a
	/// block for the block to be valid, and it solves the issue in the case that the block
	/// initialization is sufficiently heavy to mean that those inherents do not fit into the
	/// block. Essentially, we assume that in these exceptional circumstances, it is better to
	/// allow an overweight block to be created than to not allow any block at all to be created.
	Mandatory,

frame-executive: Reject invalid inherents in the executive #12365

frame-executive: Reject invalid inherents in the executive #12365

Uh oh!

Conversation

bkchr commented Sep 27, 2022

Uh oh!

muharem commented Nov 7, 2022

Uh oh!

bkchr commented Nov 16, 2022

Uh oh!

bkchr commented Nov 25, 2022

Uh oh!

paritytech-processbot bot commented Nov 25, 2022

Uh oh!

Uh oh!

Uh oh!

kianenigma Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

bkchr Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

bkchr commented Dec 4, 2022

Uh oh!

paritytech-processbot bot commented Dec 4, 2022

Uh oh!

bkchr commented Dec 4, 2022

Uh oh!

paritytech-processbot bot commented Dec 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants