Introduce BlockGasUsed field in ResponseCheckTx and ResponseDeliverTx

[AdityaSripal]

Proposal to add a BlockGasUsed uint64 field into the ResponseCheckTx and ResponseDeliverTx structs in abci/types package.

The Response structs are created by the application and sent over ABCI to inform Tendermint and clients what the result of a transaction execution was. The GasUsed field is useful information when diagnosing why a transaction failed and can be useful even information even when transaction succeeds.

However, the GasUsed field is currently getting used in two different contexts in the SDK. In normal cases where transaction succeeds or exceeds the tx gas limit, the GasUsed field is set to the amount of gas consumed by the transaction.

However, in the exceptional case where the BlockGasLimit is exceeded on a transaction execution; the GasUsed field reflects the total gas consumed in the block, rather than the gas consumed in this single transaction. This is necessary since it is the only field where we can fit this important information into the response. However, it confuses the semantics of the GasUsed field since it is now being used for two different purposes depending on the internal state of the application.

This makes it hard for clients to programatically depend on the GasUsed field since its value may be representing different things based on the specific state of the application.

To avoid this problem, I propose adding a BlockGasUsed field to the Response structs. This still allows developers to return the total gas consumed in a block after a transaction gets executed, which is valuable information when the BlockGasLimit has been exceeded; while solidifying the semantics of the GasUsed field so that it is always representing the same information regardless of the transaction's execution path.

Ref: cosmos/cosmos-sdk#5421 (comment)

cc: @alexanderbez

[alexanderbez]

Perfect summary 👌. Thoughts @melekes?

[melekes]

This makes sense to me 👍

[alexanderbez]

Shall I work on this?

[melekes]

Could you first write a short ADR in the spec repo https://github.com/tendermint/spec?

[alexanderbez]

@melekes I agree this should be a trivial and concise ADR, not a spec. But it doesn't look like the spec repo has ADRs. I presume ADRs reside in the Tendermint repo? Should I make an ADR there instead and just update the spec here once approved?

[tac0turtle]

It would be preferred that the spec repo be updated first. It seems there is general agreement regarding this change from reading this issue, it may not need an RFC, but we should run it by the shared Tendermint channel to make sure everyone is on the same page.

Not sure it needs an ADR either in the tendermint repo?

[alexanderbez]

This doesn't warrant a spec IMHO, but I do think it warrants an ADR because it's an architecture change which we should document (which ADRs are for). Where do ADRs live?

It feels weird to update the spec before an ADR is reviewed and approved.

[tac0turtle]

This doesn't warrant a spec IMHO, but I do think it warrants an ADR because it's an architecture change which we should document (which ADRs are for). Where do ADRs live?

Sorry for the miscommunication I meant updating of the spec i.e. https://docs.tendermint.com/master/spec/abci/abci.html#checktx & https://docs.tendermint.com/master/spec/abci/abci.html#delivertx, not writing a spec for the change.

What I meant here was:

It seems there is general agreement regarding this change from reading this issue, it may not need an RFC, but we should run it by the shared Tendermint channel to make sure everyone is on the same page.

was based off this image:

.

Since this is a protocol change it should start from the spec repo but can be done in parallel to the work in the repo.

The spec repo only has RFCs, ADRs are generally implementation details.

[alexanderbez]

Yes @marbar3778 that's exactly what I meant. Update the current (ABCI) spec once an ADR is reviewed and approved.

I guess I just don't agree with the flow here. In this context, it should be ADR (review + approve) -> update spec -> implement.

---

I don't want to be disruptive though, so if updating the spec is a must followed by implementation, then I'll go ahead and just update the spec.

[tac0turtle]

I guess I just don't agree with the flow here. In this context, it should be ADR (review + approve) -> update spec -> implement.

We can definitely update the flow, it's not set in stone. I believe the flow you mention is what we are coming from. The reason we are trying to update the spec first is to build habit. We would like to version the spec and implementation teams implement versions of the spec. Currently, the flow is issue/discussion -> RFC in spec repo (if needed) / ADR (in implementation, if needed)/ spec update / implementation. Many things happen in parallel but getting out of this is the goal. The flow in the future may look something like issue/discussion -> RFC (if needed) -> spec update -> spec release -> impl ADR (if needed) -> implmentation

[alexanderbez]

Ohhh so the current flow is exactly what I want to do then. ADR -> spec update -> implementation. What am I missing?

[melekes]

Let's discuss this in the meeting. I don't think the image reflects the desired flow well (e.g. if it's a protocol change and solution certainty is high, we nevertheless want to discuss the change in the spec repo). From what I can see

• all changes to the protocol and/or core data structures should go through the spec repo (RFC as a document or an issue on Github)
• implementation only / architecture changes should go through the Tendermint repo (ADR in /docs/adr if change is substantial; we may want to define what substantial is or decide as we go)

sorry for confusion @alexanderbez

[melekes]

Ohhh so the current flow is exactly what I want to do then. ADR -> spec update -> implementation. What am I missing?

good to go imho

[erikgrinaker]

Sorry, I'm late to the game here, but why does the SDK need to return this? If we make GasUsed always contain the amount of gas used by the transaction, then surely Tendermint can figure out the total gas used by the block by simply summing up the tx gas usage?

[alexanderbez]

Two things @erikgrinaker

1. BeginBlock & EndBlock both consume gas as well, although not relevant to txs.
2. The SDK state-machine already natively keeps track and tally of gas costs via its Context, why put extra burden on Tendermint to do this?

[erikgrinaker]

BeginBlock & EndBlock both consume gas as well, although not relevant to txs.

So maybe these should return GasUsed?

The SDK state-machine already natively keeps track and tally of gas costs via its Context, why put extra burden on Tendermint to do this?

By adding this field, we're putting the burden on all applications using Tendermint (not just the SDK).

[alexanderbez]

By adding this field, we're putting the burden on all applications using Tendermint (not just the SDK).

Not necessarily, this is purely diagnostic information. An ABCI app isn't required to fill out GasUsed (AFAIK), so it's not required to fill out BlockGasUsed. So there isn't a burden per se.

In the SDK context, we already have this info so we'll populate it. For other apps, well they're either doing the exact same thing the SDK is doing or nothing at all.

[alexanderbez]

Also, re querying, where would Tendermint put this information if it were to track it itself? How would it be stored, proved, and queried against?

[erikgrinaker]

Yeah, I don't really know the details surrounding gas, so maybe I'm wrong - just a thought. But if Tendermint doesn't care about gas then why do we have these fields at all?

[alexanderbez]

I think it's because those fields are very common in the blockchain context (although not all chains use the nomenclature "gas"). But for example, some chains may not use it all if their gas model is drastically different?

[cmwaters]

I've been reading through this and wanted to ask why we necessarily wanted to create a new field. Isn't possible to use either the existing events or data fields to store this kind of information. By having all these gas related fields aren't we over specialising towards applications that actually utilise some gas model in a certain way rather than perhaps building Tendermint as a generalized BFT SMR protocol

[alexanderbez]

I've been reading through this and wanted to ask why we necessarily wanted to create a new field. Isn't possible to use either the existing events or data fields to store this kind of information. By having all these gas related fields aren't we over specialising towards applications that actually utilise some gas model in a certain way rather than perhaps building Tendermint as a generalized BFT SMR protocol

The notion of "gas" is already baked into Tendermint. Why would having a tx-scoped gas used be OK but not having a block-scoped gas used be NOT OK? Putting it into events doesn't make much sense and seems like a poor design choice -- one field is a first class citizen, while the other is some blob in a series of events that need to be decoded. RE to data, same argument applies.

ABCI applications that do not have a notion of gas would either omit these field(s) entirely or map them unto something else that replaces a gas model in their context/domain.

[cmwaters]

Why would having a tx-scoped gas used be OK but not having a block-scoped gas used be NOT OK?

Well aditya was saying that this only happens in exceptional cases. In any case my concern is in making sure that these variables in the response generalize well / make sense across the range of applications that build on top of Tendermint.

[alexanderbez]

Yes, the bug happens in exceptional cases, but regardless if its a normal block or one that exhibits this behavior, it's diagnostic info.

[cmwaters]

The team have recently had a discussion around the role of gas in Tendermint. I thought I'd link the RFC here for relevancy: https://github.com/tendermint/tendermint/blob/master/docs/rfc/rfc-011-delete-gas.md