abci: Synchronize FinalizeBlock with the updated specification

[williambanfield]

This change set implements the most recent version of FinalizeBlock.

What does this change actually contain?

• This change set is rather large but fear not! The majority of the files touched and changes are renaming ResponseDeliverTx to ExecTxResult. This should be a pretty inoffensive change since they're effectively the same type but with a different name.
• The execBlockOnProxyApp was totally removed since it served as just a wrapper around the logic that is now mostly encapsulated within FinalizeBlock
• The updateState helper function has been made a public method on State. It was being exposed as a shim through the testing infrastructure, so this seemed innocuous.
• Tests already existed to ensure that the application received the ByzantineValidators and the ValidatorUpdates, but one was fixed up to ensure that LastCommitInfo was being sent across.
• Tests were removed from the psql indexer that seemed to search for an event in the indexer that was not being created.

Questions for reviewers

• We store this ABCIResponses type in the data base as the block results. This type has changed since v0.35 to contain the FinalizeBlock response. I'm wondering if we need to do any shimming to keep the old data retrieveable?
• Similarly, this change is exposed via the RPC through ResultBlockResults changing. Should we somehow shim or notify for this change?

closes: #7658

[creachadair]

The only change to the structure is the Events field is now called TxEvents. The marshalled JSON results an object with field tx_events instead of events.

Before I had read the next paragraph, I was going to suggest just keeping the original JSON field tag. So yeah, let's plan on that. This might be a little tedious because of the way protobuf names map to JSON, but we can shim it in the RPC if we have to.

That reminds me, when we do update the protos we should also make sure the JSON marshal wrappers get updated too. I have been meaning to make sure that gets enforced in CI but haven't done so yet.

Anything I can do to help out with this?

Right now, as far as I can tell, it's entirely manual. In abci/types/result.go we have some hand-written shims for JSON encoding on response types, which we might need to update. (Protobuf annoyingly does not generate these, and the standard encoding/json package does not correctly handle them out of the box). I've got a tool that can automate this, I just need to kick some dust off it and wire it up.

[williambanfield]

TODO:

• Update addTxCmd to finalizeBlockCmd so that users can create a 'finalize block'
• handle tx_event encoding change to use events for the json encoded fields.

[williambanfield]

@sergio-mena, Before I go creating a type to wrap the ExecTxResult fields so as to keep the old json field name, I'd like to get your input: It appears we should keep the same field name of the tx_events field, which used to be called events. This field is used by the relayer, so it looks like we should keep the name the same so as to not break our users.

I'm wondering if you feel strongly about keeping the internal type name as TxEvents?

The field is within a ExecTxResult so I'm not sure we should be too worried about the events being ambiguous in some way if we just dropped the Tx from the field name. This would allow us to keep the old json encoding without having to do any shimming. If you feel strongly about the new name, I can definitely wire up a shim.

[sergio-mena]

@sergio-mena, Before I go creating a type to wrap the ExecTxResult fields so as to keep the old json field name, I'd like to get your input: It appears we should keep the same field name of the tx_events field, which used to be called events. This field is used by the relayer, so it looks like we should keep the name the same so as to not break our users.

I'm wondering if you feel strongly about keeping the internal type name as TxEvents?

The field is within a ExecTxResult so I'm not sure we should be too worried about the events being ambiguous in some way if we just dropped the Tx from the field name. This would allow us to keep the old json encoding without having to do any shimming. If you feel strongly about the new name, I can definitely wire up a shim.

My view is that, if we risk breaking IBC due to renaming "events" to "tx_events", it's not worth it. So I think I'm aligned with you on this. Nevertheless, if you rename it, please adapt the corresponding field name in the spec.

On a somewhat related point. I decided on the ExecTxResult name because TxResult was already existing (this was the name I wanted to use initially). I remember doing an extensive search of TxResult and the situation looked a bit strange to me: TxResult was defined as part of ABCI, but ABCI didn't use it anywhere. IIRC, it was only used somewhere in the rpc code.
In summary, I think it would be cool if we could get rid of current TxResult (e.g., by moving it away from ABCI protobuf definitions) and renaming ExecTxResult to TxResult.

[williambanfield]

My view is that, if we risk breaking IBC due to renaming "events" to "tx_events", it's not worth it. So I think I'm aligned with you on this. Nevertheless, if you rename it, please adapt the corresponding field name in the spec.

Switched to the old name, made sure to update the spec as well.