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(storage): cleanup NotFound errors #2065

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

chore(storage): cleanup NotFound errors #2065

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a19b0d1
cleanup block.ErrSlotNotFound
abi87 Oct 11, 2024
06aee0d
some more errors cleanup
abi87 Oct 11, 2024
7bfea6b
wip: fixing err not found in beacondb package
abi87 Oct 11, 2024
bba5071
wip: more fixing err not found in beacondb package
abi87 Oct 11, 2024
3afed77
wip: some more fixing err not found in beacondb package
abi87 Oct 11, 2024
3917a9a
wip: yet some more fixing err not found in beacondb package
abi87 Oct 11, 2024
64d2f3e
reverted
abi87 Oct 11, 2024
dc50cc1
reducing code duplication
abi87 Oct 13, 2024
034f7d1
nit
abi87 Oct 13, 2024
b0f9050
nit
abi87 Oct 13, 2024
55ce71e
reverted faulty changes to slashings
abi87 Oct 13, 2024
b32f9d8
nit
abi87 Oct 13, 2024
86da4a8
Merge branch 'main' into storage-cleanup-err-not-found
nidhi-singh02 Oct 14, 2024
c9b579a
updated mockery to v 2.46.3
abi87 Oct 14, 2024
b998227
chores(engine-primitives): nit (#2068)
abi87 Oct 15, 2024
ec89f97
consolidated ErrNotFound
abi87 Oct 15, 2024
7c5e4ad
Merge branch 'update-mockery-v2-46-3' into storage-cleanup-err-not-found
abi87 Oct 15, 2024
23e5a13
nit
abi87 Oct 15, 2024
35e893c
Merge branch 'main' into storage-cleanup-err-not-found
abi87 Oct 15, 2024
30b680f
nits
abi87 Oct 15, 2024
fbd9772
repackaged storage errors
abi87 Oct 15, 2024
4acb893
Merge branch 'main' into storage-cleanup-err-not-found
abi87 Oct 15, 2024
3e88d11
chore(storage): No errors silencing (#2076)
abi87 Oct 15, 2024
db3bee6
Merge branch 'main' into storage-cleanup-err-not-found
abi87 Oct 17, 2024
c2d9108
Merge branch 'main' into storage-cleanup-err-not-found
abi87 Oct 19, 2024
c1a1986
Merge branch 'main' into storage-cleanup-err-not-found
abi87 Oct 29, 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
72 changes: 62 additions & 10 deletions mod/storage/pkg/beacondb/eth1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@

package beacondb

import (
"fmt"

storage "github.com/berachain/beacon-kit/mod/storage/pkg"
)

// GetLatestExecutionPayloadHeader retrieves the latest execution payload
// header from the BeaconStore.
func (kv *KVStore[
Expand All @@ -28,13 +34,40 @@ func (kv *KVStore[
]) GetLatestExecutionPayloadHeader() (
ExecutionPayloadHeaderT, error,
) {
forkVersion, err := kv.latestExecutionPayloadVersion.Get(kv.ctx)
var h ExecutionPayloadHeaderT
v, err := kv.getLatestExecutionPayloadVersion()
if err != nil {
return h, fmt.Errorf(
"failed retrieving latest execution payload header: %w",
err,
)
}

kv.latestExecutionPayloadCodec.SetActiveForkVersion(v)
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

SetActiveForkVersion Does Not Validate Version Inputs

The SetActiveForkVersion method in mod/storage/pkg/encoding/ssz.go assigns the provided version directly without validating whether it is valid or supported. This lack of validation may lead to unexpected behavior if an invalid or unsupported version is set.

  • File: mod/storage/pkg/encoding/ssz.go
    • SetActiveForkVersion lacks error handling for invalid versions.
🔗 Analysis chain

Ensure SetActiveForkVersion handles invalid versions

When setting the active fork version with kv.latestExecutionPayloadCodec.SetActiveForkVersion(v), it's important to verify that invalid or unsupported versions are properly handled. Lack of validation may lead to unexpected behavior elsewhere in the code.

Run the following script to check the implementation of SetActiveForkVersion:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that SetActiveForkVersion handles invalid versions.

# Search for the SetActiveForkVersion method implementation and check for error handling.
rg --type go 'func\s+\([^)]+\)\s+SetActiveForkVersion\s*\(' -A 15

# Review the implementation to ensure it includes validation logic.

Length of output: 1081

h, err = kv.latestExecutionPayloadHeader.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
return h, fmt.Errorf(
"failed retrieving latest execution payload header: %w",
err,
)
}
return h, nil
}

func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) getLatestExecutionPayloadVersion() (uint32, error) {
v, err := kv.latestExecutionPayloadVersion.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
var t ExecutionPayloadHeaderT
return t, err
return 0, fmt.Errorf(
"failed retrieving latest execution payload version: %w",
err,
)
}
kv.latestExecutionPayloadCodec.SetActiveForkVersion(forkVersion)
return kv.latestExecutionPayloadHeader.Get(kv.ctx)
return v, nil
}

// SetLatestExecutionPayloadHeader sets the latest execution payload header in
Expand All @@ -45,9 +78,11 @@ func (kv *KVStore[
]) SetLatestExecutionPayloadHeader(
payloadHeader ExecutionPayloadHeaderT,
) error {
if err := kv.latestExecutionPayloadVersion.Set(
kv.ctx, payloadHeader.Version(),
); err != nil {
var (
ctx = kv.ctx
version = payloadHeader.Version()
)
if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil {
Comment on lines +81 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify SetLatestExecutionPayloadHeader by removing unnecessary variable

The variable ctx is assigned kv.ctx but is used only once. You can simplify the code by using kv.ctx directly, eliminating the need for the extra variable.

Apply this diff to streamline the function:

 func (kv *KVStore[
     BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
     ForkT, ValidatorT, ValidatorsT,
 ]) SetLatestExecutionPayloadHeader(
     payloadHeader ExecutionPayloadHeaderT,
 ) error {
-    var (
-        ctx     = kv.ctx
-        version = payloadHeader.Version()
-    )
+    version := payloadHeader.Version()
-    if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil {
+    if err := kv.latestExecutionPayloadVersion.Set(kv.ctx, version); err != nil {
         return err
     }
     kv.latestExecutionPayloadCodec.SetActiveForkVersion(payloadHeader.Version())
     return kv.latestExecutionPayloadHeader.Set(kv.ctx, payloadHeader)
 }
📝 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
var (
ctx = kv.ctx
version = payloadHeader.Version()
)
if err := kv.latestExecutionPayloadVersion.Set(ctx, version); err != nil {
version := payloadHeader.Version()
if err := kv.latestExecutionPayloadVersion.Set(kv.ctx, version); err != nil {

return err
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}
abi87 marked this conversation as resolved.
Show resolved Hide resolved
kv.latestExecutionPayloadCodec.SetActiveForkVersion(payloadHeader.Version())
Expand All @@ -59,7 +94,16 @@ func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) GetEth1DepositIndex() (uint64, error) {
return kv.eth1DepositIndex.Get(kv.ctx)
idx, err := kv.eth1DepositIndex.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
return 0, fmt.Errorf(
"failed retrieving eth1 deposit index %d, %w",
idx,
err,
)
}
return idx, nil
}

// SetEth1DepositIndex sets the eth1 deposit index in the beacon state.
Expand All @@ -77,7 +121,15 @@ func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) GetEth1Data() (Eth1DataT, error) {
return kv.eth1Data.Get(kv.ctx)
d, err := kv.eth1Data.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
return d, fmt.Errorf(
"failed retrieving eth1 data: %w",
err,
)
}
return d, nil
}

// SetEth1Data sets the eth1 data in the beacon state.
Expand Down
13 changes: 12 additions & 1 deletion mod/storage/pkg/beacondb/fork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@

package beacondb

import (
"fmt"

storage "github.com/berachain/beacon-kit/mod/storage/pkg"
)

// SetFork sets the fork version for the given epoch.
func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
Expand All @@ -35,5 +41,10 @@ func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
ForkT, ValidatorT, ValidatorsT,
]) GetFork() (ForkT, error) {
return kv.fork.Get(kv.ctx)
f, err := kv.fork.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
return f, fmt.Errorf("failed retrieving fork: %w", err)
}
return f, nil
}
31 changes: 27 additions & 4 deletions mod/storage/pkg/beacondb/history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@

package beacondb

import "github.com/berachain/beacon-kit/mod/primitives/pkg/common"
import (
"fmt"

"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
storage "github.com/berachain/beacon-kit/mod/storage/pkg"
)

// UpdateBlockRootAtIndex sets a block root in the BeaconStore.
func (kv *KVStore[
Expand All @@ -41,8 +46,13 @@ func (kv *KVStore[
index uint64,
) (common.Root, error) {
bz, err := kv.blockRoots.Get(kv.ctx, index)
err = storage.MapError(err)
if err != nil {
return common.Root{}, err
return common.Root{}, fmt.Errorf(
"failed retrieving block root at index %d: %w",
index,
err,
)
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}
return common.Root(bz), nil
}
Expand All @@ -64,7 +74,15 @@ func (kv *KVStore[
]) GetLatestBlockHeader() (
BeaconBlockHeaderT, error,
) {
return kv.latestBlockHeader.Get(kv.ctx)
h, err := kv.latestBlockHeader.Get(kv.ctx)
err = storage.MapError(err)
if err != nil {
return h, fmt.Errorf(
"failed retrieving latest block header: %w",
err,
)
}
return h, nil
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

// UpdateStateRootAtIndex updates the state root at the given slot.
Expand All @@ -86,8 +104,13 @@ func (kv *KVStore[
idx uint64,
) (common.Root, error) {
bz, err := kv.stateRoots.Get(kv.ctx, idx)
err = storage.MapError(err)
if err != nil {
return common.Root{}, err
return common.Root{}, fmt.Errorf(
"failed retrieving state root at index %d: %w",
idx,
err,
)
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}
return common.Root(bz), nil
}
14 changes: 12 additions & 2 deletions mod/storage/pkg/beacondb/randao.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@

package beacondb

import "github.com/berachain/beacon-kit/mod/primitives/pkg/common"
import (
"fmt"

"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
storage "github.com/berachain/beacon-kit/mod/storage/pkg"
)

// UpdateRandaoMixAtIndex sets the current RANDAO mix in the store.
func (kv *KVStore[
Expand All @@ -41,8 +46,13 @@ func (kv *KVStore[
index uint64,
) (common.Bytes32, error) {
bz, err := kv.randaoMix.Get(kv.ctx, index)
err = storage.MapError(err)
if err != nil {
return common.Bytes32{}, err
return common.Bytes32{}, fmt.Errorf(
"failed retrieving randao mix at index %d: %w",
index,
err,
)
}
return common.Bytes32(bz), nil
}
Loading
Loading