abci++: only include meaningful header fields in data passed-through to application

[williambanfield]

closes: #7950

[sergio-mena]

Thanks for this! Looking great!

[cmwaters]

The one piece of information that I think is missing that applications may want is to know who the proposer for the block was

[sergio-mena]

The one piece of information that I think is missing that applications may want is to know who the proposer for the block was

@cmwaters are you thinking of its possible use by slashing and/or distribution modules?

[cmwaters]

Yes in the past, the SDK have allowed the proposer to receive an additional reward. I think generally it makes sense because there's nothing else that could inform the application of the proposer elected without the application having to run the algorithm themselves

[tychoish]

should this flip with the previous line?

[sergio-mena]

Yes in the past, the SDK have allowed the proposer to receive an additional reward

I'm aware of that. That's what I'm concerned with, although I don't have hard proof today. We're having preliminary discussions in the Protocol Desgin team within Informal, but no results yet.

So, as there isn't any hard proof at the moment (about asymmetric rewards encouraging byzantine behavior), I'm OK with including this field. We can always deprecate it and/or discourage its use in the future.

[cmwaters]

Ref: https://github.com/cosmos/cosmos-sdk/blob/90f61870e69b7cc07ac8537949715638af62f386/x/distribution/abci.go#L36

[williambanfield]

Ref: https://github.com/cosmos/cosmos-sdk/blob/90f61870e69b7cc07ac8537949715638af62f386/x/distribution/abci.go#L36

Happy to add it as part of this pull request. I did a pass through the SDK and this appears to the main field missed. I also see the SDK make many references to the ChainID. @cmwaters do you know if the SDK stores the chain ID anywhere and doesn't rely strictly on getting it from the block header?

cc: @marbar3778 as well.

[tac0turtle]

Id prefer to keep the entire header being passed to the application in at least one phase since we don't know all the use cases an application may have for this.

Is there any benefit to not passing it, it doesn't seem like there is an added overhead to the message, In 99.9% cases the application and tendermint will run on the same machine so the over head is very very minimal.

The sdk doesn't have complex use cases for most things, all the modules are simple implementations, there are chains looking at doing more complex things that may need this data, but passing it each time may not be needed.

[sergio-mena]

Id prefer to keep the entire header being passed to the application in at least one phase since we don't know all the use cases an application may have for this.

Is there any benefit to not passing it, it doesn't seem like there is an added overhead to the message, In 99.9% cases the application and tendermint will run on the same machine so the over head is very very minimal.

The sdk doesn't have complex use cases for most things, all the modules are simple implementations, there are chains looking at doing more complex things that may need this data, but passing it each time may not be needed.

I'm not convinced that exposing the whole header to the App (with all the hashes, etc) is the cleanest API. Since this API is defined using protobufs, we can easily add any field that might be missing, without breaking backwards compatibility.

[creachadair]

The sdk doesn't have complex use cases for most things, all the modules are simple implementations, there are chains looking at doing more complex things that may need this data, but passing it each time may not be needed.

I'm not convinced that exposing the whole header to the App (with all the hashes, etc) is the cleanest API. Since this API is defined using protobufs, we can easily add any field that might be missing, without breaking backwards compatibility.

Adding to this: The concern about existing use is a very reasonable one. Because ABCI++ will require all applications to make changes, however, this is one of the rare cases where we can get meaningful feedback about what applications are actually doing.

If an app needs header data that we aren't exposing in ABCI++, I'd like them to come talk to us about it. Minimizing the header to start will allow us to have that conversation (and we could add more fields in a point release, it wouldn't require a major update).

[tac0turtle]

Either way works with me, I don't have the full scope of what people are doing, and the cosmos-sdk repo is not a good representation. The ibc team may have better insight here

[williambanfield]

Either way works with me, I don't have the full scope of what people are doing, and the cosmos-sdk repo is not a good representation. The ibc team may have better insight here

Thanks for the info marko we pinged the IBC-rs team to see if they have more info.

[williambanfield]

This now matches the .intermediate file. It appears that at some point these files skewed.

[williambanfield]

Updated to match the .intermediate file.

[cmwaters]

LGTM thanks!