implementing generic header interface for rollmint header

[gupadhyaya]

Resolves#641

[tzdybal]

We can't just remove Time from this interface. Header.Time should be renamed (to Header.time?) and Header.Time() method should just return it (of course we need to update usages in codebase).

[gupadhyaya]

it will be odd to just rename the two fields (Height and Time) to lower-case. Either we uniformly rename all fields (even then other structs like Block, Data, etc, have upper-case field names) or find derived names for header interface methods. e.g., Time() => Timestamp() and Height() => Number(). Basically, if you consider the Ethereum header, these alternatives are used. Is it possible to change the header interface this way?

[gupadhyaya]

@tzdybal what is your opinion on this?

[tzdybal]

Let's start with mixing getters with fields to match the header interface from https://github.com/celestiaorg/go-header. We plan to re-work the header in the near future anyways - then we will resolve this issue.

[codecov-commenter]

Codecov Report

Merging #652 (a0c9a40) into main (653d4bf) will decrease coverage by 0.01%.
The diff coverage is 42.85%.

@@ Coverage Diff @@## main #652 +/- ##
==========================================
- Coverage 55.37% 55.35% -0.02% 
==========================================
Files 50 51 +1 Lines 10293 10405 +112 ==========================================
+ Hits 5700 5760 +60 - Misses 3728 3774 +46 - Partials 865 871 +6 

Impacted Files	Coverage Δ
types/header.go	42.85% <42.85%> (ø)
block/manager.go	67.92% <0.00%> (+1.01%)	⬆️
types/validation.go	43.18% <0.00%> (+6.81%)	⬆️
rpc/json/ws.go	61.90% <0.00%> (+7.93%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[gupadhyaya]

@tzdybal while the PR is ready for re-review here are some open issues that i need your suggestions on:

1. to avoid name collision for rollkit header struct fields (Height & Time) and Header interface methods, i have changed the Header interface methods to Number() instead of Height() and Timestamp() instead of Time(). i believe this is the clean way to resolve this. let me know your opinion.
2. In VerifyAdjacent logic, we need to check that validator hashes of next validators of trusted header is same as validator hashes of untrusted adjacent header. Since we don't have access to store (to grab the validatorSet), i am directly checking the AggregatorsHash, however this may be incorrect when the adjacent headers have different validator sets? can you clarify?
3. In VerifyNonAdjacent, we need to check that untrusted commit has enough of trusted commit's power. Not sure how to accomplish this without accessing store. So, i have commented it out. let me know if you have any suggestions here.

[tzdybal]

1. to avoid name collision for rollkit header struct fields (Height & Time) and Header interface methods, i have changed the Header interface methods to `Number()` instead of `Height()` and `Timestamp()` instead of `Time()`. i believe this is the clean way to resolve this. let me know your opinion.

While Timestamp looks OK, we never use Number. There will also be ChainID which is hard to just "rename". I think, that later on we will refactor our header package anyways (see: #579), but I would prefer to stick to original interface (also see: celestiaorg/celestia-node#1304).

2. In `VerifyAdjacent` logic, we need to check that validator hashes of next validators of trusted header is same as validator hashes of untrusted adjacent header. Since we don't have access to store (to grab the validatorSet), i am directly checking the `AggregatorsHash`, however this may be incorrect when the adjacent headers have different validator sets? can you clarify?

Ah, we started a bit prematurely working on this - see: celestiaorg/celestia-node#1304 (comment)

Yes - it seems that we'll need NextAggregatorsHash or something similar. But right now we don't support multiple aggregators, so this will be OK.

3. In `VerifyNonAdjacent`, we need to check that untrusted commit has enough of trusted commit's power. Not sure how to accomplish this without accessing store. So, i have commented it out. let me know if you have any suggestions here.

In case of rollmint, it doesn't work like that - in current "status quo" we just need to check that aggregatorsHash is valid/same.

[nashqueue]

[non-blocking] Comment Rollkit might have a different shorter or longer trusting period than node. The fraud proof window time might depend on this as well, if you are an optimistic Rollup. This should be toggleable in the future and the default value should be a deliberate value.

[tzdybal]

Replaced by #685.