feat!: run min gas pfb decorator in process proposal

[cmwaters]

Closes: #1971

Yes, I'm aware that there's a lot of redundancy in the checks across the different stages in committing and executing a block. My plan is to start at a point of restrictiveness and then slowly remove redundancy rather than add more restrictiveness (which would be breaking)

[codecov-commenter]

Codecov Report

Merging #1985 (4af8999) into main (d326697) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1985   +/-   ##
=======================================
  Coverage   21.60%   21.60%           
=======================================
  Files         122      122           
  Lines       13724    13724           
=======================================
  Hits         2965     2965           
  Misses      10470    10470           
  Partials      289      289           

Impacted Files	Coverage Δ
x/blob/ante/ante.go	88.46% <0.00%> (ø)

[rootulp]

[non blocking][question] how does modifying this line make the antehandler run in ProcessProposal?

[cmwaters]

ProcessProposal uses a context that has isCheckTx as false. Prior this would only run if the context was CheckTx now that this is removed it will run the handler in ProcessProposal. The reason for changing it to IsReCheckTx is that it doesn't make sense to re run this in ReCheckTx as nothing has changed

[evan-forbes]

how does modifying this line make the antehandler run in ProcessProposal?

in #1984 we use the same antehandler for all things, which will then call the above handler as well if I'm not mistaken

[rootulp]

[no change needed][for my understanding] is there a downside to running this ante handler when ctx.IsReCheckTx? Is there a significant performance impact?

The reason for changing it to IsReCheckTx is that it doesn't make sense to re run this in ReCheckTx as nothing has changed

To clarify, this ante handler is checking that the gas included the transaction exceeds the amount of gas expected to be consumed. Since it does not check that an account actually has the the amount of gas included in the transaction, this ante handler doesn't depend on state. Therefore the boolean result from this ante handler should be consistent before / after a new block is added even if that new block included a transaction that spent all of an account's funds.

[evan-forbes]

that's a good point @rootulp, but this ante handler works in tandem with the DeductFeeDecorator which runs before this one. So we actually first check if the account has enough funds, then we check that the fee is large enough for the PFB

[rootulp]

So the deduct fee decorator should be re-executed on ReCheckTx but the MinGasPFBDecorator in this PR shouldn't?

[evan-forbes]

perhpaps that's just prematurely optimal, but yeah

[cmwaters]

Is there a significant performance impact?

Probably not

[evan-forbes]

I'm okay with merging this on the condition that we also get consensus over adding a currently unplanned block validity rule.

we will need each pfb to be able to pay for gas should we enable fee burning or some sort of min fee, which was originally planned for #49 #49 (comment) #48 (comment), but has since been removed from mainnet.

after merging this, there will be an extra check that each PFB will have to have enough funds to pay for gas. While this is trivial for a validator to bypass by simply accepting 0 fee transactions (which is also a valid behavior), this is preparing us for whenever we do implement fee burning.

cc @musalbas @adlerjohn @liamsi

[musalbas]

Is this a bug fix, or a new feature?

I feel it's important that we're disciplined with release candidates, and make sure that new release candidates only have bug fixes, and do not introduce new features that need to be additionally tested.

This looks like a trivial change - is there a reason that we cannot simply add this when we implement fee burning?

[musalbas]

My plan is to start at a point of restrictiveness and then slowly remove redundancy rather than add more restrictiveness (which would be breaking)

If there is an argument that this is actually a bug fix or security improvement due to the above - then I'm also fine with this being included, as that rationale makes sense.

[cmwaters]

I would classify this as a bug fix but not an important one. Currently a validator could propose a block with a PFB that fills the entire block and instead of specifying the 20 million gas it would require to execute, only setting 100k. The tx would pass the validity checks in ProcessProposal but would run out of gas when it was executed in DeliverTx. However it doesn't matter if a PFB doesn't execute successfully, the data has already been published and the block proposer has got DA basically for free. The reason why this is not important is because regardless if the gas is correctly calculated or not, a user can always set a gas price of 0, and thus a fee of 0 (fee = gas * gas price) so long as the proposers min gas price that it will accept is 0 (default is non-zero). In other words you can get DA for free anyway. My understanding is that we will want to implement a proper and robust fee market relatively soon. Having this bug fixed will mean it's done now instead of later but given that a fee market implementation will likely be breaking in any case I think it's also fine if we punt on this change until v2.

[evan-forbes]

per sync discussion: 👍