Redundant weight subtraction

[dnjscksdn98]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

Issue 1: Redundant Block Initialization Cost Subtraction

Title: Redundant subtraction of initialization costs from max_extrinsic weights

Description:

The current implementation in BlockWeightsBuilder::build() subtracts the block initialization cost from each dispatch class's max_extrinsic weight. This appears to be redundant because:

1. Block initialization costs are already tracked under the Mandatory dispatch class
2. Normal/Operational classes are already limited to their percentage of the remaining block weight (e.g., 75% for Normal by default)
3. Class limits (max_total) are enforced separately from block initialization
   Current code:

// In BlockWeightsBuilder::build
if let Some(init_weight) = init_cost.map(|rate| rate * weights.max_block) {
    for class in DispatchClass::all() {
        let per_class = weights.per_class.get_mut(*class);
        if per_class.max_extrinsic.is_none() && init_cost.is_some() {
            per_class.max_extrinsic = per_class
                .max_total
                .map(|x| x.saturating_sub(init_weight))  // <-- Redundant subtraction
                .map(|x| x.saturating_sub(per_class.base_extrinsic));
        }
    }
}

This double accounting of initialization costs could unnecessarily restrict the maximum size of individual extrinsics.

Issue 2: Incorrect Base Extrinsic Weight Subtraction

Title: max_extrinsic incorrectly includes base_extrinsic subtraction, contradicting documentation

Description:

There's an inconsistency between the documentation and implementation regarding base_extrinsic handling in WeightsPerClass:

1. The documentation explicitly states that max_extrinsic should NOT include the base_extrinsic cost:

pub struct WeightsPerClass {
    /// Maximal weight of single extrinsic. Should NOT include `base_extrinsic` cost.
    pub max_extrinsic: Option<Weight>,
    // ...
}

2. However, the implementation in BlockWeightsBuilder::build() subtracts base_extrinsic:

per_class.max_extrinsic = per_class
    .max_total
    .map(|x| x.saturating_sub(init_weight))
    .map(|x| x.saturating_sub(per_class.base_extrinsic));  // <-- Contradicts documentation

This leads to double-counting of the base cost because:

1. The base cost will be added when executing the extrinsic
2. It's already been subtracted from the max limit

Unexpected Scenario

For example, when a substrate node that consists frontier, and the maximum block gas limit is set to 50,000,000 gas. Due to these cost subtractions, eventually the maximum gas limit for a single transaction can only consume 43,333,333 gas. Which means, even though that a maximum value is set, it can't even reach it. If a transaction's gas limit is set higher than 43,333,333 it responds with an error, exceeds block gas limit

Steps to reproduce

No response

[bkchr]

The current implementation in BlockWeightsBuilder::build() subtracts the block initialization cost from each dispatch class's max_extrinsic weight. This appears to be redundant because:

1. Block initialization costs are already tracked under the Mandatory dispatch class

2. Normal/Operational classes are already limited to their percentage of the remaining block weight (e.g., 75% for Normal by default)

3. Class limits (max_total) are enforced separately from block initialization
   Current code:

If always 1/4 of the block is used for block initialization, this clearly needs to be removed from the max_total. The dispatch classes are layed out this way to ensure that for example you can always include operational extrinsics. If you would not remove the block initialization costs, it would mean that you could fill the entire block with normal transactions and prevent the inclusion of operational extrinsics.

There's an inconsistency between the documentation and implementation regarding base_extrinsic handling in WeightsPerClass:

That looks like its valid.

[dnjscksdn98]

If always 1/4 of the block is used for block initialization, this clearly needs to be removed from the max_total. The dispatch classes are layed out this way to ensure that for example you can always include operational extrinsics. If you would not remove the block initialization costs, it would mean that you could fill the entire block with normal transactions and prevent the inclusion of operational extrinsics.

As I understood, normal extrinsics can only occupy 75% of the block (if we use with_sensible_defaults), and operational will be set to 25% of the block. And mandatory stuff such as block initializations doesn't have any limitations. With all of these constraints how come normal extrinsics can fill the entire block that makes it prevent other extrinsics fail inclusion? For me the constraints looks already fine without the substraction.

[bkchr]

For sure mandatory and block initialization can take more, but this is the problem. If always 30% is used by default in a block and you have configured normal transactions to take up to 75%, they will be able to fill the rest of the entire block. The weight of an entire block is fixed and the 75% will be taken from this fixed weight.

[dnjscksdn98]

Ok as far as I understood, then the average block initialization weight is a sort of an upper cap to let mandatory executions safely include in a block. And to prevent normal transactions fill the entire block, we subtract the average block init weight from the capacity. (In case of mandatory executions take more as expected)
Is this right?

[bkchr]

Ok as far as I understood, then the average block initialization weight is a sort of an upper cap to let mandatory executions safely include in a block

I can not really follow this thought. Mandatory execution are also for example inherents, which are probably not included in the average block initialization costs. But inherents actually should also be included there to prevent the issue of transactions being able to fill the entire block.

CC @ggwpez

And to prevent normal transactions fill the entire block, we subtract the average block init weight from the capacity. (In case of mandatory executions take more as expected)

That is right :)

[ggwpez]

The doc of avg_block_initialization reads:

This is to make sure that extrinsics don't stay forever in the pool, because they could seemingly fit the block (since they are below max_block), but the cost of calling on_initialize always prevents them from being included.

Dont know if the pool still operates like this, but it seems like a bit of a hack to me. Not sure if the TX pool still operates like this?

[bkchr]

Dont know if the pool still operates like this, but it seems like a bit of a hack to me. Not sure if the TX pool still operates like this?

Can you please elaborate? What would you do instead?

[ggwpez]

My comment was a bit vague since i dont know how the TX pool handles this. I though if the runtime rejects an extrinsic for being overweight, then it does not stay in the pool:

polkadot-sdk/substrate/client/basic-authorship/src/basic_authorship.rs

Lines 488 to 489 in 311ea43

	Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() => {
	pending_iterator.report_invalid(&pending_tx);

I guess the only way to decide this is to try to include them as first TX in a block. If that does not work then it should not be retried.