finality: refactor voting power table and APIs #222
Conversation
First step of #24 This PR moves the voting power distribution update algorithm to `x/finality`. This is a major refactoring that includes - moving `max_active_finality_providers` parameter to `x/finality` - moving many functions in `x/btcstaking/power_dist_change.go` and relevant tests to `x/finality` - moving test utilities under `x/btcstaking/keeper_test.go` to a new module `testutil/btcstaking-helper`, and use it for existing tests of `x/btcstaking` and `x/finality` - removing the cyclic dependency between `x/btcstaking` and `x/finality`, as well as their hooks. Now the hooks in these 2 modules are not necessary, as the only dependency between them is that `x/finality` will invoke `x/btcstaking`, but not the other way TODOs in subsequent PRs: - moving some relevant query APIs to `x/finality` - moving the voting power dist cache and voting power table KV stores to `x/finality`
First step of #24 This PR moves the voting power distribution update algorithm to `x/finality`. This is a major refactoring that includes - moving `max_active_finality_providers` parameter to `x/finality` - moving many functions in `x/btcstaking/power_dist_change.go` and relevant tests to `x/finality` - moving test utilities under `x/btcstaking/keeper_test.go` to a new module `testutil/btcstaking-helper`, and use it for existing tests of `x/btcstaking` and `x/finality` - removing the cyclic dependency between `x/btcstaking` and `x/finality`, as well as their hooks. Now the hooks in these 2 modules are not necessary, as the only dependency between them is that `x/finality` will invoke `x/btcstaking`, but not the other way TODOs in subsequent PRs: - moving some relevant query APIs to `x/finality` - moving the voting power dist cache and voting power table KV stores to `x/finality`
a2dd350 to
32295d2
Compare
| // 0x05 was used for something else in the past | ||
| BTCHeightKey = []byte{0x06} // key prefix for the BTC heights | ||
| // 0x07 was used for something else in the past | ||
| PowerDistUpdateKey = []byte{0x08} // key prefix for power distribution update events |
There was a problem hiding this comment.
NOTE: this is needed for making software upgrade to work. The new version will have trouble accessing KV store in the old version if we change the keys of KV store.
32295d2 to
2b726b2
Compare
There was a problem hiding this comment.
Thanks for the heavy lifting! Great work! No blocker from my side. My major question is given that we have already introduce activation height into the parameter, not sure the old activated height (first height with at least 1 fp with positive voting power) is till useful.
I was thinking about this as well, and my answer is that the BTC staking activation height is still needed. There is actually no guarantee when the first BTC delegation will come to the network, so it's possible that it comes before or after the finality activation height. Probably due to this reason, the |
I think our expectation of introducing this parameter is that after activation, fps can submit votes immediately. Even if there are no delegations or no timestamped pub rand after activation, we should still enable finality-related functionalities. Maybe it's better to move to https://github.com/babylonlabs-io/pm/pull/85 for discussion as this is out of the scope of this PR. Sorry about that 😅 |
|
Merging this PR now. Any further review could go to #24 |
1be8537
into
feat/voting-power-table-refactor
Second half of #24 This PR moves the voting power table/cache and relevant APIs to `x/finality`. This includes the following - [x] move voting power table KV store to `x/finality` - [x] move voting power dist cache to `x/finality` - [x] move 4 APIs `ActiveFinalityProvidersAtHeight`, `FinalityProviderPowerAtHeight`, `FinalityProviderCurrentPower`, `ActivatedHeight` to `x/finality` - [x] fix fuzz tests - [x] fix e2e tests - [x] fix doc - [x] fix changelog Note that this affects many LoCs since it touches a bunch of proto files. The actual modification is much smaller than it looks like.
Second half of #24 This PR moves the voting power table/cache and relevant APIs to `x/finality`. This includes the following - [x] move voting power table KV store to `x/finality` - [x] move voting power dist cache to `x/finality` - [x] move 4 APIs `ActiveFinalityProvidersAtHeight`, `FinalityProviderPowerAtHeight`, `FinalityProviderCurrentPower`, `ActivatedHeight` to `x/finality` - [x] fix fuzz tests - [x] fix e2e tests - [x] fix doc - [x] fix changelog Note that this affects many LoCs since it touches a bunch of proto files. The actual modification is much smaller than it looks like.
Second half of #24
This PR moves the voting power table/cache and relevant APIs to
x/finality. This includes the followingx/finalityx/finalityActiveFinalityProvidersAtHeight,FinalityProviderPowerAtHeight,FinalityProviderCurrentPower,ActivatedHeighttox/finalityNote that this affects many LoCs since it touches a bunch of proto files. The actual modification is much smaller than it looks like.