fix(ADR-024): bugs in voting power assignment #38
Conversation
gitferry
force-pushed
the
fix/voting-power-assigning
branch
from
8b074cc to
d87f520
Compare
gitferry
force-pushed
the
fix/voting-power-assigning
branch
from
d87f520 to
0588060
Compare
gitferry
changed the title
| // deep copy the previous dist cache if the new dist cache is nil | ||
| if newDc == nil { | ||
| newDc = types.NewVotingPowerDistCache() | ||
| newDc.TotalVotingPower = prevDc.TotalVotingPower | ||
| newDc.NumActiveFps = prevDc.NumActiveFps | ||
| newFps := make([]*types.FinalityProviderDistInfo, len(prevDc.FinalityProviders)) | ||
| for i, prevFp := range prevDc.FinalityProviders { | ||
| newFp := *prevFp | ||
| newFps[i] = &newFp | ||
| } | ||
| newDc.FinalityProviders = newFps | ||
| } |
There was a problem hiding this comment.
Why do we need a deep copy here? It seems that prevDc is only used in L104. Is
it possible to change L103-L110 to that, if newDc != nil then execute
L103-L110, otherwise avoid doing so? To make L85-L91 work we can simply do
newDc := prevDc
There was a problem hiding this comment.
if we don't do deep copy, the change at Line 90 will change the IsTimestamped value in prevDc, which makes no newly activated finality providers at L104. This specifically affects the case where no new delegations are added but new finality providers have timestamped pub rand
| } | ||
|
|
||
| // deep copy the previous dist cache if the new dist cache is nil | ||
| if newDc == nil { |
There was a problem hiding this comment.
I think it would be cleaner to move this copying process to above the call to recordVotingPowerAndCache in case when there is no new events i.e somewehre under // map everything in prev height to this height comment.
Otherwise this nil check just generate questions like:
- oh why we can pass nil here ?
- should we also check
prevDcto be non-nil ?
etc.
Where if this is handled on call site we can add comments along the line that becouse there is no new power changing events, the new dc is just copy of the old one and the deep copy is need due to recordVotingPowerAndCache modifying provided arguments. (or something like that)
There was a problem hiding this comment.
Agreed allowing nil parameter is hacky. Fixed in f3099ee
| k.SetVotingPower(ctx, fp.BtcPk.MustMarshal(), babylonTipHeight, fp.TotalVotingPower) | ||
| } | ||
|
|
||
| // find newly activated finality providers and execute the hooks by comparing | ||
| // the previous dist cache | ||
| newActivatedFinalityProviders := newDc.FindNewActiveFinalityProviders(prevDc) |
There was a problem hiding this comment.
for the case where len(events) == 0 and dc != nil that we do the deep copy, it will never have a set of newActivatedFinalityProviders ?
There was a problem hiding this comment.
I thought the same but turned out there is an issue if we don't take care of this (took me a long time to realize this...). So, even if no new events, some fps could have newly timestamped pub rand and become a new activated fp. For example, we only have one fp and one delegation to it at height 100 but it does not have timestamped pub rand. When it comes to height 101, the fp's pub rand commit is timestamped, so it will be assigned voting power for 101, thus it becomes activated
There was a problem hiding this comment.
got it, thanks for explaining!
In this case, we don't have a way to avoid the deep copy :/
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
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
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
This PR fixed some issues in assigning the voting power, in particular
HasTimestampedPubRand