ADR 17: Historical header module

[cwgoes]

Closes #5078

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/)
• Re-reviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #5340 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5340      +/-   ##
==========================================
+ Coverage    54.8%   54.81%   +0.01%     
==========================================
  Files         311      311              
  Lines       18385    18385              
==========================================
+ Hits        10075    10077       +2     
+ Misses       7539     7537       -2     
  Partials      771      771

[cwgoes]

Decided to scrap this in favour of just duplicating state. I will revise.

[AdityaSripal]

Seems like we're scrapping this idea for a new one? Is there a short description of the new approach anywhere?

Seems pretty trivial to handle this completely on the application side rather than making any changes to ABCI.

Since n is an application-parameter and the current BlockHeader and ValidatorSet is introspectable by application using the Context, we can simply save ctx.BlockHeader and the current ValidatorSet on each BeginBlock under the current height and then prune the store to ensure we always have n most recent historical information. So we have some module history with a BeginBlocker like:

func BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
    h := HistoricalInfo{
         Header: ctx.BlockHeader(),
         ValSet: keeper.StakingKeeper.GetAllValidators(ctx),
    }
    keeper.SetHistoricalInfo(ctx, key=ctx.BlockHeight(), val=encode(h))
    keeper.PruneHistoricalInfo(ctx, key = ctx.BlockHeight() - params.N)
}

This can then be queried by the IBC modules. If this is already the new approach we're going for, I'm in favor of it 👍

[cwgoes]

Seems like we're scrapping this idea for a new one? Is there a short description of the new approach anywhere?

Seems pretty trivial to handle this completely on the application side rather than making any changes to ABCI.

Since n is an application-parameter and the current BlockHeader and ValidatorSet is introspectable by application using the Context, we can simply save ctx.BlockHeader and the current ValidatorSet on each BeginBlock under the current height and then prune the store to ensure we always have n most recent historical information. So we have some module history with a BeginBlocker like:

func BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
    h := HistoricalInfo{
         Header: ctx.BlockHeader(),
         ValSet: keeper.StakingKeeper.GetAllValidators(ctx),
    }
    keeper.SetHistoricalInfo(ctx, key=ctx.BlockHeight(), val=encode(h))
    keeper.PruneHistoricalInfo(ctx, key = ctx.BlockHeight() - params.N)
}

This can then be queried by the IBC modules. If this is already the new approach we're going for, I'm in favor of it

Yes; precisely. I might just copy your code!

(I had written the previous version in an attempt to save space, since we will now be storing headers on disk in both Tendermint and the SDK - duplicating data - but after discussion with the Tendermint team we agreed that the complexity wasn't worth it.)

[cwgoes]

Technically we don't need to store the validator set; we can just store the hash and have the handshake require the validator set be provided as an additional parameter (store abstraction in rc5 of the IBC spec makes this easier). We can store it if we want though; it makes the life of a relayer a wee bit easier.

Thoughts @AdityaSripal?

[AdityaSripal]

Agreed, we can just store the hash if there's a simple way to retrieve the validator set for a given height. How would the relayer get the validator set. Is there already a query for it on the staking store?

[fedekunze]

ACK

[cwgoes]

Agreed, we can just store the hash if there's a simple way to retrieve the validator set for a given height. How would the relayer get the validator set. Is there already a query for it on the staking store?

It needs to be queried from Tendermint, I expect, as the SDK can't necessarily calculate it (information is lost if a validator unbonds, rebonds, for example).

[AdityaSripal]

OK, so long as its queryable by the relayer as-needed rather than forcing the relayer to keep track of the validator sets themselves in their own local db; I am ok with storing hash

[AdityaSripal]

ACK

[cwgoes]

I think I've addressed all the comments - not sure what the SDK merge process is these days - @alexanderbez did you want to quickly review (+ merge if satisfactory)?

[alexanderbez]

ACK.

1. Can you add the ADR to the TOC in docs/architecture/README.MD?
2. Since this doesn't process any txs, is it worth having it as a new module or can we retrofit into x/staking? Not a blocker, mainly just a question of best design.

[cwgoes]

ACK.

1. Can you add the ADR to the TOC in docs/architecture/README.MD?
2. Since this doesn't process any txs, is it worth having it as a new module or can we retrofit into x/staking? Not a blocker, mainly just a question of best design.

1. Done.
2. Noted that x/staking would be a good existing module to use.

[fedekunze]

what about storing it on the canonical IBC Keeper? I don't see the need for blockchains that are not using IBC to store the extra headers. @alexanderbez

[cwgoes]

Sure, added x/ibc as another suggestion.

[cwgoes]

(also, why does the simulation fail in some of the above CI runs? this PR only alters docs)

[alexanderbez]

ACK 🎉

[alexanderbez]

@cwgoes most likely because each successive commit kills any previous CI pipeline (so we only have 1 CI pipeline running per commit/per PR).