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

Jump to bottom

chore(consensus-types): Make slashings use math.Gwei #2038

Merged
merged 5 commits into from
Oct 3, 2024
Merged

chore(consensus-types): Make slashings use math.Gwei #2038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e879eb4
updated mockery
abi87 Oct 2, 2024
0b80e54
turned slashing to math.Gwei
abi87 Oct 3, 2024
f54531f
Merge branch 'main' into beacon-state-inconsistent-gwei-usage
abi87 Oct 3, 2024
c8a646c
Merge branch 'main' into beacon-state-inconsistent-gwei-usage
abi87 Oct 3, 2024
e4dbb2e
Merge branch 'main' into beacon-state-inconsistent-gwei-usage
abi87 Oct 3, 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
6 changes: 3 additions & 3 deletions mod/consensus-types/pkg/types/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ type BeaconState[
NextWithdrawalValidatorIndex math.ValidatorIndex

// Slashing
Slashings []uint64
Slashings []math.Gwei
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the main point of the PR. Make Slashing and TotalSlashing share the same type

TotalSlashing math.Gwei
}

Expand Down Expand Up @@ -97,7 +97,7 @@ func (st *BeaconState[
randaoMixes []common.Bytes32,
nextWithdrawalIndex uint64,
nextWithdrawalValidatorIndex math.ValidatorIndex,
slashings []uint64,
slashings []math.Gwei,
totalSlashing math.Gwei,
) (*BeaconState[
BeaconBlockHeaderT,
Expand Down Expand Up @@ -378,7 +378,7 @@ func (st *BeaconState[
}
subIndx = hh.Index()
for _, i := range st.Slashings {
hh.AppendUint64(i)
hh.AppendUint64(uint64(i))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

q: should I use i.Unwrap() here instead? It seems the principled way, but it makes a copy, while casting should not. We do use the uint64 cast for TotalSlashing as well (line 392)

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)

LGTM with a suggestion: Consider documenting overflow risk

The casting of math.Gwei to uint64 is consistent with how TotalSlashing is handled and avoids unnecessary copying. However, it's worth considering the following:

  1. Document the assumption that math.Gwei will never exceed the range of uint64.
  2. Consider adding a runtime check to ensure no overflow occurs during the cast.

These steps would help prevent potential issues if math.Gwei's range ever changes in the future.

}
hh.FillUpTo32()
numItems = uint64(len(st.Slashings))
Expand Down
2 changes: 1 addition & 1 deletion mod/consensus-types/pkg/types/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func generateValidBeaconState() *types.BeaconState[
},
Balances: []uint64{32000000000, 31000000000},
RandaoMixes: generateRandomBytes32(65536),
Slashings: []uint64{1000000000, 2000000000},
Slashings: []math.Gwei{1000000000, 2000000000},
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)

LGTM! Consider updating TotalSlashing for consistency.

The change from []uint64 to []math.Gwei for the Slashings field aligns with the PR objective. This improves type safety and clarity in the code.

However, for consistency, consider updating the TotalSlashing field on line 106 to use math.Gwei as well.

Suggested change for line 106:

-		TotalSlashing:                3000000000,
+		TotalSlashing:                math.Gwei(3000000000),
📝 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
Slashings: []math.Gwei{1000000000, 2000000000},
Slashings: []math.Gwei{1000000000, 2000000000},
TotalSlashing: math.Gwei(3000000000),

NextWithdrawalIndex: 7,
NextWithdrawalValidatorIndex: 8,
TotalSlashing: 3000000000,
Expand Down
2 changes: 1 addition & 1 deletion mod/node-api/handlers/proof/merkle/mock/beacon_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func NewBeaconState(
[]common.Bytes32{},
0,
0,
[]uint64{},
[]math.Gwei{},
0,
)
return &BeaconState{BeaconStateMarshallable: bsm}, err
Expand Down
4 changes: 2 additions & 2 deletions mod/node-core/pkg/components/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ type (
randaoMixes []common.Bytes32,
nextWithdrawalIndex uint64,
nextWithdrawalValidatorIndex math.U64,
slashings []uint64, totalSlashing math.U64,
slashings []math.U64, totalSlashing math.U64,
) (T, error)
}

Expand Down Expand Up @@ -894,7 +894,7 @@ type (
// GetRandaoMixAtIndex retrieves the randao mix at the given index.
GetRandaoMixAtIndex(index uint64) (common.Bytes32, error)
// GetSlashings retrieves all slashings.
GetSlashings() ([]uint64, error)
GetSlashings() ([]math.Gwei, error)
// SetSlashingAtIndex sets the slashing at the given index.
SetSlashingAtIndex(index uint64, amount math.Gwei) error
// GetSlashingAtIndex retrieves the slashing at the given index.
Expand Down
2 changes: 1 addition & 1 deletion mod/state-transition/pkg/core/state/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ type KVStore[
// GetRandaoMixAtIndex retrieves the randao mix at the given index.
GetRandaoMixAtIndex(index uint64) (common.Bytes32, error)
// GetSlashings retrieves all slashings.
GetSlashings() ([]uint64, error)
GetSlashings() ([]math.Gwei, error)
// SetSlashingAtIndex sets the slashing at the given index.
SetSlashingAtIndex(index uint64, amount math.Gwei) error
// GetSlashingAtIndex retrieves the slashing at the given index.
Expand Down
2 changes: 1 addition & 1 deletion mod/state-transition/pkg/core/state/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ type BeaconStateMarshallable[
randaoMixes []common.Bytes32,
nextWithdrawalIndex uint64,
nextWithdrawalValidatorIndex math.U64,
slashings []uint64, totalSlashing math.U64,
slashings []math.U64, totalSlashing math.U64,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Inconsistency in the type of slashings across the codebase

After reviewing the occurrences of slashings in the codebase, there is an inconsistency in the types being used:

  • math.U64 in:
    • mod/node-core/pkg/components/interfaces.go
    • mod/state-transition/pkg/core/state/types.go
  • math.Gwei in:
    • mod/storage/pkg/beacondb/slashing.go
    • mod/consensus-types/pkg/types/state.go
  • sdkcollections.Map[uint64, uint64] in:
    • mod/storage/pkg/beacondb/kvstore.go

This inconsistency can lead to confusion and potential type-related issues. It is recommended to standardize the type used for slashings to align with the PR objective of utilizing math.Gwei.

🔗 Analysis chain

Type change improves consistency, but differs from PR objective

The change from []uint64 to []math.U64 for the slashings parameter improves type consistency with totalSlashing. This is a positive change as it likely provides additional functionality or constraints compared to the built-in uint64.

However, there's a discrepancy between the PR objective and the actual change:

  • PR objective: Make slashings use math.Gwei
  • Actual change: Using math.U64

Please clarify if math.U64 is equivalent to math.Gwei or if there's a reason for this difference. If math.Gwei should be used instead, consider updating the type accordingly.

To ensure consistency across the codebase, let's check for other occurrences of slashings and their types:

This will help verify if the type change is consistent with other parts of the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of 'slashings' in Go files
rg --type go 'slashings.*\[?\]?(uint64|math\.U64|math\.Gwei)' -g '!mod/state-transition/pkg/core/state/types.go'

Length of output: 505

) (T, error)
}

Expand Down
6 changes: 3 additions & 3 deletions mod/storage/pkg/beacondb/slashing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import (
func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) GetSlashings() ([]uint64, error) {
var slashings []uint64
]) GetSlashings() ([]math.Gwei, error) {
var slashings []math.Gwei
iter, err := kv.slashings.Iterate(kv.ctx, nil)
if err != nil {
return nil, err
Expand All @@ -41,7 +41,7 @@ func (kv *KVStore[
if err != nil {
return nil, err
}
slashings = append(slashings, slashing)
slashings = append(slashings, math.Gwei(slashing))
iter.Next()
}
return slashings, nil
Expand Down
Loading