Audit BaseApp v0.46..v0.47 prepare & process proposal

[amaury1093]

No description provided.

[julienrbrt]

Just assigning myself here to have a look at prepare & process proposal (but not auditing baseapp).

[amaury1093]

Sharing my notes here: https://hackmd.io/_Aj3jciNTRiNVwLxcZ5K2A#Baseapp (scroll down to baseapp), I had mostly questions on why things were the way they were, probably nothing alarming. Also i'm going on holidays and won't be available until Jan 3rd.

[facundomedica]

@JeancarloBarrios @kocubinski could you please take a look at Amaury's notes?

[facundomedica]

I'm reopening @alexanderbez , there are some stuff in Amaury's notes that needs more eyes imo (points 2 through 5)

[tac0turtle]

Lets complete this issue this week, what else is left to do

[facundomedica]

imo we didn't get enough feedback on some points in Amaury's notes yet.

2. Why does PrepareProposal call ctx := app.getContextForTx(runTxPrepareProposal, []byte{}) and ProcessProposal not?

3. Should we set ctx.{Height,Time} in PrepareProposal? (currently not:

   cosmos-sdk/baseapp/abci.go

   Line 250 in d247e7a

   func (app *BaseApp) PrepareProposal(req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {

   )

4. Should we also consume block gas in Prepare and Process proposal? (currently not:

   cosmos-sdk/baseapp/baseapp.go

   Line 658 in d247e7a

   if mode == runTxModeDeliver {

   )

5. Also, why do we call consumeBlockGas twice in runTx?

[JeancarloBarrios]

4. Should we also consume block gas in Prepare and Process proposal? (currently not:

   cosmos-sdk/baseapp/baseapp.go

   Line 658 in d247e7a

   if mode == runTxModeDeliver {

When we did it, we assumed that only during deliver( when tx is executed) we consume gas, but I don't know if anybody else thinks it should also happen in prepare and process.

[amaury1093]

My doubt was if it was possible to have a proposal with a set of tx whose gas sum is greater than the block gas limit, passes prepare & process, gets voted on, but ultimately fails on deliver?

[kocubinski]

My doubt was if it was possible to have a proposal with a set of tx whose gas sum is greater than the block gas limit, passes prepare & process, gets voted on, but ultimately fails on deliver?

I guess we could execute consumeBlockGas in mode runTxPrepareProposal and runTxProccessProposal. I think the state branching already in place would prevent gas consumption from being committed, right? No this isn't tracked in store, just in memory with a meter set in BeginBlock. I think we'd need a different meter for prepare/process, or only consume/check blockGas in one of these steps? Does that make sense?

[alexanderbez]

Should we also consume block gas in Prepare and Process proposal?

I do NOT think so. Certainly not in PrepareProposal since that is executed by the proposer only.

Now as for ProcessProposal, you could make an argument, especially since in the future the entire block proposal can be executed there. So I vote:

• PrepareProposal No
• ProcessProposal Yes

[kocubinski]

5. Also, why do we call consumeBlockGas twice in runTx?

It looks like we should remove the deferred consumeBlockGas. The inline call prevents state from being committed when consumeBlockGas panics.

This wasn't modified by the scope of the changes in ABCI 1.0 though, so maybe this can be handled separately.

[tac0turtle]

can this be closed now?

[alexanderbez]

@facundomedica are you done with the audit, correct?

[facundomedica]

Yes, points 2 and 3 @AmauryM pointed out were part of the issue reported by dydx and Celestia. That's fixed. Then there are issues tracking gas consumption. So I'll close this as done ✅