core/types: header hashing

[tac0turtle]

Description

Cleanup header hashing in favor of proto encoding the struct and hashing the output.

closes tendermint/tendermint#7915

[tac0turtle]

@ebuchman @milosevic, could I get an approval from one of you

[ebuchman]

Hey @marbar3778 this is a proposed breaking change to the Block Protocol? It should have a referencable RFC or ideally CIP now

[tac0turtle]

Hey @marbar3778 this is a proposed breaking change to the Block Protocol? It should have a referencable RFC or ideally CIP now

How we have RFC's setup is that if there is not an understanding or agreement on the change, an RFC would help explain things related to it. This change was suggested by many people so I went with there is an understanding and agreement with the approach. As for CIP, I can assist in writing a CIP for the hub. Who on the Gaia team is responsible for this?

[ebuchman]

Ok thanks. Right now I'm responsible for the CIPs as they're still in infancy and I'm looking for people to volunteer to be editors and to write the first technical CIPs.

As for the change here, the current header hashing is documented under the BlockID - so probably that needs to be updated in addition to or instead of the change here. Otherwise it looks fine.

I'll also note there still seems to be some mention of amino - eg. here

Also, how do we manage versions for this repo. If this gets merged here before the change in Tendermint, won't ppl reading the spec be misled? Should we start versioning the spec and even including a changelog? Though ideally a changelog would in the end be a list of CIPs added

[tac0turtle]

Also, how do we manage versions for this repo. If this gets merged here before the change in Tendermint, won't ppl reading the spec be misled? Should we start versioning the spec and even including a changelog? Though ideally a changelog would in the end be a list of CIPs added

Version 0.1 should be cut soon. Not sure what procedure is wanted to do this. There are a couple PRs that would be useful to merge before this tho. A PR that falls into this category is #276, but its a nice to have.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ValarDragon]

Why are headers protobuf encoded btw, are all fields of fixed length? If not, won't this be hard to parse on other chains?

[tac0turtle]

Why are headers protobuf encoded btw, are all fields of fixed length? If not, won't this be hard to parse on other chains?

They were previously amino encoded, this was to keep similar design to the previous one. I don't think there is a valid reason.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[liamsi]

They were previously amino encoded, this was to keep similar design to the previous one. I don't think there is a valid reason.

@marbar3778 I think the fixed length encoding is a good thing independent from proto/amino (and if I understand @ValarDragon's comment correctly he is also advocating for that to make the header easily parseable by other chains). If all fields are fixed size, encoding and decoding is simple: just the raw bytes (and documenting the endianness for integers).

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.