Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(state-transition): restore state sync over boonet #2219

Merged
merged 4 commits into from
Dec 6, 2024
Merged

fix(state-transition): restore state sync over boonet #2219

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a4cb28
restore state sync over boonet
abi87 Dec 5, 2024
e06f91a
Merge branch 'main' into fix-boonet-sync
calbera Dec 5, 2024
bc45673
make processRewardsAndPenalties backward compatible
abi87 Dec 6, 2024
4e99f74
nit
abi87 Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/spec/special_cases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

package spec

import "math"

// Special cased Bartio for some ad-hoc handling due to the way
// some bugs were handled on Bartio. To be removed.
const (
Expand All @@ -34,4 +36,6 @@ const (
BoonetFork1Height uint64 = 69420

BoonetFork2Height uint64 = 1722000

BoonetFork3Height uint64 = math.MaxUint64
)
64 changes: 60 additions & 4 deletions state-transition/core/state_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/berachain/beacon-kit/errors"
"github.com/berachain/beacon-kit/log"
"github.com/berachain/beacon-kit/primitives/common"
"github.com/berachain/beacon-kit/primitives/constants"
"github.com/berachain/beacon-kit/primitives/crypto"
"github.com/berachain/beacon-kit/primitives/math"
"github.com/berachain/beacon-kit/primitives/transition"
Expand Down Expand Up @@ -374,15 +375,36 @@ func (sp *StateProcessor[
]) processEpoch(
st BeaconStateT,
) (transition.ValidatorUpdates, error) {
// currently no processRewardsAndPenalties
slot, err := st.GetSlot()
if err != nil {
return nil, err
}

if err := sp.processEffectiveBalanceUpdates(st); err != nil {
switch {
case sp.cs.DepositEth1ChainID() == spec.BartioChainID:
if err = sp.hollowProcessRewardsAndPenalties(st); err != nil {
return nil, err
}
case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID &&
slot != 0 && slot < math.U64(spec.BoonetFork3Height):
// We cannot simply drop hollowProcessRewardsAndPenalties because
// appHash accounts for the list of operations carried out
// over the state even if the operations does not affect the state
// (rewards and penalties are always zero at this stage of beaconKit)
if err = sp.hollowProcessRewardsAndPenalties(st); err != nil {
return nil, err
}
default:
// no real need to perform hollowProcessRewardsAndPenalties
}

if err = sp.processEffectiveBalanceUpdates(st); err != nil {
return nil, err
}
if err := sp.processSlashingsReset(st); err != nil {
if err = sp.processSlashingsReset(st); err != nil {
return nil, err
}
if err := sp.processRandaoMixesReset(st); err != nil {
if err = sp.processRandaoMixesReset(st); err != nil {
return nil, err
}
return sp.processValidatorsSetUpdates(st)
Expand Down Expand Up @@ -470,6 +492,40 @@ func (sp *StateProcessor[
return st.SetLatestBlockHeader(lbh)
}

func (sp *StateProcessor[
_, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _,
]) hollowProcessRewardsAndPenalties(st BeaconStateT) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would just add a comment above this function explaining it came from getAttestationDeltas which was just returning 0s for rewards and 0s for penalties.

slot, err := st.GetSlot()
if err != nil {
return err
}

if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) {
return nil
}

// this has been simplified to make clear that
// we are not really doing anything here
valCount, err := st.GetTotalValidators()
if err != nil {
return err
}

for i := range valCount {
// Increase the balance of the validator.
if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}

// Decrease the balance of the validator.
if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
}

Comment on lines +495 to +525
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the incorrect loop iteration over validators in 'hollowProcessRewardsAndPenalties'

The loop for i := range valCount is incorrect because valCount is a numeric value representing the total number of validators, not a slice or array. Using range on an integer will result in a compilation error.

Apply this diff to fix the loop:

func (sp *StateProcessor[
    _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _,
]) hollowProcessRewardsAndPenalties(st BeaconStateT) error {
    slot, err := st.GetSlot()
    if err != nil {
        return err
    }

    if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) {
        return nil
    }

    // this has been simplified to make clear that
    // we are not really doing anything here
    valCount, err := st.GetTotalValidators()
    if err != nil {
        return err
    }

-   for i := range valCount {
+   for i := uint64(0); i < valCount; i++ {
        // Increase the balance of the validator.
        if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil {
            return err
        }

        // Decrease the balance of the validator.
        if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil {
            return err
        }
    }

    return nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (sp *StateProcessor[
_, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _,
]) hollowProcessRewardsAndPenalties(st BeaconStateT) error {
slot, err := st.GetSlot()
if err != nil {
return err
}
if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) {
return nil
}
// this has been simplified to make clear that
// we are not really doing anything here
valCount, err := st.GetTotalValidators()
if err != nil {
return err
}
for i := range valCount {
// Increase the balance of the validator.
if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
// Decrease the balance of the validator.
if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
}
func (sp *StateProcessor[
_, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _,
]) hollowProcessRewardsAndPenalties(st BeaconStateT) error {
slot, err := st.GetSlot()
if err != nil {
return err
}
if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) {
return nil
}
// this has been simplified to make clear that
// we are not really doing anything here
valCount, err := st.GetTotalValidators()
if err != nil {
return err
}
for i := uint64(0); i < valCount; i++ {
// Increase the balance of the validator.
if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
// Decrease the balance of the validator.
if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
}

return nil
}
Comment on lines +495 to +527
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding unit tests for 'hollowProcessRewardsAndPenalties'

Adding unit tests for the new hollowProcessRewardsAndPenalties function will ensure its correctness across different scenarios.


// processEffectiveBalanceUpdates as defined in the Ethereum 2.0 specification.
// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#effective-balances-updates
//
Expand Down
2 changes: 1 addition & 1 deletion state-transition/core/state_processor_staking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func (sp *StateProcessor[
// We keep it for backward compatibility.
depositIndex++
case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID &&
slot < math.U64(spec.BoonetFork2Height):
slot != 0 && slot < math.U64(spec.BoonetFork2Height):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

genesis indexes were set right, so we don't correct them.

// Boonet pre fork2 has a bug which makes DepositEth1ChainID point to
// next deposit index, not latest processed deposit index.
// We keep it for backward compatibility.
Expand Down
Loading