fix: set block gas meter on prepare/process proposal #15012

alexanderbez · 2023-02-13T17:53:14Z

Description

Set block gas meter on the Context provided to PrepareProposal and ProcessProposal handlers.

Note, @JayT106, this only allows gas consumption checks up and until the AnteHandler phase, since in the SDK's default handlers of Prepare/Process proposal we do NOT execute messages. So it would be equivalent to calling CheckTx

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

alexanderbez · 2023-02-13T17:53:52Z

baseapp/baseapp.go

@@ -625,8 +625,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
 	}()

 	blockGasConsumed := false


This files contains cosmetic changes only FYI.

github-actions · 2023-02-13T17:54:03Z

baseapp/baseapp.go

 	}()

 	blockGasConsumed := false
-	// consumeBlockGas makes sure block gas is consumed at most once. It must happen after
-	// tx processing, and must be executed even if tx processing fails. Hence, we use trick with `defer`
+
+	// consumeBlockGas makes sure block gas is consumed at most once. It must
+	// happen after tx processing, and must be executed even if tx processing
+	// fails. Hence, it's execution is deferred.
 	consumeBlockGas := func() {
 		if !blockGasConsumed {


Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).runTx (baseapp/baseapp.go:604) (*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).DeliverTx (baseapp/baseapp.go:385)

JayT106 · 2023-02-13T18:26:11Z

Thanks, @alexanderbez. I've tried PR #14984 to address the new handler issue. But I am unsure of the goal of ABCI 1.0 after phase 1. Maybe it needs a broad discussion, but the goal will be to fix #2150 and the block fills issue with gas transactions.

JayT106 · 2023-02-13T18:32:23Z

I don't understand why the gas meter uses deliverState.ctx instead of using prepareproposal.ctx or processproposal.ctx? Isn't the gas meter should relate to the block param of the context's consensus param (so theoretically will be the same between these contexts?)

alexanderbez · 2023-02-13T19:17:16Z

Thanks, @alexanderbez. I've tried PR #14984 to address the new handler issue. But I am unsure of the goal of ABCI 1.0 after phase 1. Maybe it needs a broad discussion, but the goal will be to fix #2150 and the block fills issue with gas transactions.

Yes, so the goal of ABCI++ integration is as follows:

SDK v0.47.x: ABCI++ 1.0, i.e. PrepareProposal and ProcessProposal (RELEASED!)
SDK v0.48.x: ABCI++ 2.0, i.e ExtendVote, VerifyVoteExtension and FinalizeBlock (Soon, maybe end of q1)
SDK v0.49.x: Optimistic execution, i.e. process the entire block in ProcessProposal and slim down FinalizeBlock which will allow for gas refunds and similar features (Soon-ish, most likely q2)

I don't understand why the gas meter uses deliverState.ctx instead of using prepareproposal.ctx or processproposal.ctx? Isn't the gas meter should relate to the block param of the context's consensus param (so theoretically will be the same between these contexts?)

Yes, you're right, it was a bug and I didn't have my coffee yet lol

alexanderbez · 2023-02-13T19:18:01Z

Pushed fixes.

JayT106 · 2023-02-13T19:22:46Z

SDK v0.47.x: ABCI++ 1.0, i.e. PrepareProposal and ProcessProposal (RELEASED!)

SDK v0.48.x: ABCI++ 2.0, i.e ExtendVote, VerifyVoteExtension and FinalizeBlock (Soon, maybe end of q1)

SDK v0.49.x: Optimistic execution, i.e. process the entire block in ProcessProposal and slim down FinalizeBlock which will allow for gas refunds and similar features (Soon-ish, most likely q2)

Thanks for the explanation!

julienrbrt

lgtm!

julienrbrt · 2023-02-13T21:06:19Z

nit: Unrelated to this PR but maybe this can be updated for style consistency:

cosmos-sdk/baseapp/abci.go

Lines 179 to 185 in 80dd55f

    
           // add block gas meter 
        
           var gasMeter storetypes.GasMeter 
        
           if maxGas := app.GetMaximumBlockGas(app.deliverState.ctx); maxGas > 0 { 
        
           	gasMeter = storetypes.NewGasMeter(maxGas) 
        
           } else { 
        
           	gasMeter = storetypes.NewInfiniteGasMeter() 
        
           }

with the current changes.

alexanderbez · 2023-02-13T21:26:30Z

Good idea. I'll be sure to make them consistent!

julienrbrt · 2023-02-13T22:37:50Z

Mmmh, this is still failing: https://github.com/cosmos/cosmos-sdk/actions/runs/4168353062/jobs/7215053213
The logs do not say much.

likhita-809

lgtm

testinginprod

lgtm!

alexanderbez · 2023-02-14T21:29:43Z

I can't run localnet locally to figure out what's up... :(

(cherry picked from commit b3f9506) # Conflicts: # baseapp/abci.go

#15040) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

alexanderbez added 2 commits February 13, 2023 11:53

updates

1e3f16e

updates

226673f

github-actions bot added the C:Store label Feb 13, 2023

alexanderbez commented Feb 13, 2023

View reviewed changes

alexanderbez added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Feb 13, 2023

Merge branch 'main' into bez/14998

33b0927

github-actions bot reviewed Feb 13, 2023

View reviewed changes

alexanderbez marked this pull request as ready for review February 13, 2023 17:54

alexanderbez requested a review from a team as a code owner February 13, 2023 17:54

github-prbot requested review from a team, samricotta and testinginprod and removed request for a team February 13, 2023 17:54

alexanderbez mentioned this pull request Feb 13, 2023

PrepareProposal/ProcessProposal with block gas meter #14998

Closed

updates

26afd27

alexanderbez and others added 2 commits February 13, 2023 14:18

Merge branch 'main' into bez/14998

865e785

Merge branch 'bez/14998' of github.com:cosmos/cosmos-sdk into bez/14998

4b60bd5

JayT106 mentioned this pull request Feb 13, 2023

feat: support BlockGasMeter in prepareproposal/processproposal #14984

Closed

19 tasks

alexanderbez enabled auto-merge (squash) February 13, 2023 20:45

Merge branch 'main' into bez/14998

fd556f2

julienrbrt approved these changes Feb 13, 2023

View reviewed changes

alexanderbez disabled auto-merge February 13, 2023 21:26

updates

5fa36ef

alexanderbez enabled auto-merge (squash) February 13, 2023 21:30

Merge branch 'main' into bez/14998

c161d51

Merge branch 'main' into bez/14998

27e2f57

likhita-809 approved these changes Feb 14, 2023

View reviewed changes

Merge branch 'main' into bez/14998

46f317c

testinginprod approved these changes Feb 14, 2023

View reviewed changes

Merge branch 'main' into bez/14998

17d0e67

updates

aed86b2

github-actions bot added the Type: Build label Feb 15, 2023

Merge branch 'main' into bez/14998

3a6804a

alexanderbez merged commit b3f9506 into main Feb 15, 2023

alexanderbez deleted the bez/14998 branch February 15, 2023 15:55

mergify bot mentioned this pull request Feb 15, 2023

fix: set block gas meter on prepare/process proposal (backport #15012) #15040

Merged

mergify bot pushed a commit that referenced this pull request Feb 15, 2023

fix: set block gas meter on prepare/process proposal (#15012)

cb1289d

(cherry picked from commit b3f9506) # Conflicts: # baseapp/abci.go

alexanderbez added a commit that referenced this pull request Feb 15, 2023

fix: set block gas meter on prepare/process proposal (backport #15012) (

2e80d3b

#15040) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: set block gas meter on prepare/process proposal #15012

fix: set block gas meter on prepare/process proposal #15012

alexanderbez commented Feb 13, 2023

alexanderbez Feb 13, 2023

github-actions bot Feb 13, 2023

JayT106 commented Feb 13, 2023

JayT106 commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

JayT106 commented Feb 13, 2023

julienrbrt left a comment

julienrbrt commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

julienrbrt commented Feb 13, 2023 •

edited

Loading

likhita-809 left a comment

testinginprod left a comment

alexanderbez commented Feb 14, 2023

		@@ -625,8 +625,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
		}()

		blockGasConsumed := false

fix: set block gas meter on prepare/process proposal #15012

fix: set block gas meter on prepare/process proposal #15012

Conversation

alexanderbez commented Feb 13, 2023

Description

Author Checklist

Reviewers Checklist

alexanderbez Feb 13, 2023

Choose a reason for hiding this comment

github-actions bot Feb 13, 2023

Choose a reason for hiding this comment

JayT106 commented Feb 13, 2023

JayT106 commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

JayT106 commented Feb 13, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt commented Feb 13, 2023

alexanderbez commented Feb 13, 2023

julienrbrt commented Feb 13, 2023 • edited Loading

likhita-809 left a comment

Choose a reason for hiding this comment

testinginprod left a comment

Choose a reason for hiding this comment

alexanderbez commented Feb 14, 2023

julienrbrt commented Feb 13, 2023 •

edited

Loading