feat(ADR-024): Enable BTC-timestamping public randomness #51
Conversation
…ed pub rand (#23) This closes the second part of #8. In particular, this PR: * Implemented `HasTimestampedPubRand` in the finality keeper to check whether a pub rand at a given height is BTC-timestamped * Added expected finality keeper to btcstaking keeper so that `HasTimestampedPubRand` can be called within the module * When signing voting power to fps in `btcstaking`'s begin blocker, if the fp does not have timestamped pub rand, voting power will not be assigned Note that this PR introduced circular dependency between the btc staking module and finality module, which should be addressed by moving the voting power table to the finality module, tracked by issue #24
This PR fixed some issues in assigning the voting power, in particular * even if no new delegations are included, we should also update the cache about the timestamping status * fixed return value of `HasTimestampedPubRand` * e2e tests are fixed * added table-driven tests for the voting table
gitferry
requested review from
KonradStaniec and
RafilxTenfen
and removed request for
a team
KonradStaniec
added
consensus breaking
| uint64 total_voting_power = 1; | ||
| // finality_providers is a list of finality providers' voting power information | ||
| repeated FinalityProviderDistInfo finality_providers = 2; | ||
| // num_active_fps is the number of finality providers that have positive voting power |
There was a problem hiding this comment.
| // num_active_fps is the number of finality providers that have positive voting power | |
| // num_active_fps is the number of finality providers that have active BTC delegations |
a FP has positive voting power at a height only when it has active BTC
delegations at this height AND has timestamped public randomness at this height
There was a problem hiding this comment.
Agreed. The two conditions combined lead to fp having voting power
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| return nil, err | |
| } | |
| require.NoError(n.t, err) |
Failing to query the node should not happen so we can panick here
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| var resp bstypes.QueryActivatedHeightResponse | ||
| err = util.Cdc.UnmarshalJSON(bz, &resp) | ||
| require.NoError(n.t, err) | ||
| if err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
We can do require.NoError(n.t, err) at L99 and L105 as this means there is something fishy in the setup
|
I have validated that the db schema change introduced by this PR will not affect upgrade via https://github.com/babylonlabs-io/babylon-deployment/pull/4. This is done by adding the start of |
This PR collects sub-PRs to close #8 and complete the whole feature defined in ADR-024
TODOs