lastChurnHeight is currently calculated (in the querier) from variable BlockHeight rather than from accurate StatusSince
Based on this discussion:
queryNode
for getNodeCurrentRewards
(similarly in queryNodes
):
https://gitlab.com/thorchain/thornode/-/blob/v1.118.0/x/thorchain/querier.go#L709
lastChurnHeight := vaults[0].BlockHeight
Taking
https://thornode-v1.ninerealms.com/thorchain/vaults/asgard?height=12100726
as an example, two vaults have 12053913, one has 12053914, and two have 12053915 as the block_height
field, which is when their own keygen succeeded and they first became an InitVault.
Every one of them has 12053915 as the status_since
field, which is when the final keygen succeeded, rewards were distributed, and vaulte statuses were updated with RotateVault
.
By contrast:
getLastChurnHeight
:
https://gitlab.com/thorchain/thornode/-/blob/v1.118.0/x/thorchain/manager_validator_current.go#L1153-1157
for _, vault := range vaults {
if vault.BlockHeight > lastChurnHeight {
lastChurnHeight = vault.BlockHeight
}
}
By taking the highest BlockHeight, the StatusSince value is reached.
While a minor matter, my current impression is that it would be more appropriate if both instances used .StatusSince
rather than .BlockHeight
.
While keeping the range for the validator manager may seem redundant,
it currently seems prudent to me to keep it in case an older vault were kept
(perhaps possible if the members of a keygen were coincidentally exactly the same?).
As the highest BlockHeight is in practice equivalent to the StatusSince,
I have no objection to only changing it in the querier if requested.
My current code proposal: !3066
'Use StatusSince rather than BlockHeight for lastChurnHeight identification'.
2024-05-07:
Notably, StatusSince is already used here (since V87):
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/x/thorchain/handler_node_pause_chain.go#L84-89
lastChurn := int64(-1)
for _, vault := range active {
if vault.StatusSince > lastChurn {
lastChurn = vault.StatusSince
}
}