Remove irrelevant height branching after FCC #1324
Conversation
prasannavl
requested review from
uzyn,
Bushstar,
Mixa84 and
surangap
as code owners
|
This shouldn't be removed from consensus as it is possible to modify prefork state (for example from bad snapshot or etc.). The only fork checks that are safe to remove after hardfork kicks in is from RPC and part that are not in consensus! |
|
How so? The mainnet does not have these TXs. Corrupt snapshot are exactly that - corrupt (different network/fork). It's irrelevant. It's just dead code and extra branches, that's unnecessary to maintain. It is being intentionally removed. A bug prevented valid TXs from going in there. Now the bug is fixed. While it seems like a retroactive fix, it is not, since the mainnet TXs are already done. If someone launches a new network, it's bug-free (while the current network is still compatible) TL;DR - There's no value in keeping these code branches. |
| if (height >= static_cast<uint32_t>(consensus.FortCanningCrunchHeight)) { | ||
| CalculateOwnerRewards(obj.owner); | ||
| } | ||
| CalculateOwnerRewards(obj.owner); |
There was a problem hiding this comment.
This needs to remain guarded. Otherwise full sync nodes will end up with some accounts that performed a FutureSwap before FCC having a higher balance than others which could lead to consensus issues.
There was a problem hiding this comment.
But each action that does require a balance in consensus (except for this bug), should already be expected to do
CalculateOwnerRewards before such an action anyway?
Is there anything else I'm missing - otherwise, let's just do a confirmation anyway with full a sync and proceed to remove?
/kind chore