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.

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

chore(storage): cleanup NotFound errors #2065

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 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
44 changes: 44 additions & 0 deletions mod/storage/pkg/beacondb/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// SPDX-License-Identifier: BUSL-1.1
//
// Copyright (C) 2024, Berachain Foundation. All rights reserved.
// Use of this software is governed by the Business Source License included
// in the LICENSE file of this repository and at www.mariadb.com/bsl11.
//
// ANY USE OF THE LICENSED WORK IN VIOLATION OF THIS LICENSE WILL AUTOMATICALLY
// TERMINATE YOUR RIGHTS UNDER THIS LICENSE FOR THE CURRENT AND ALL OTHER
// VERSIONS OF THE LICENSED WORK.
//
// THIS LICENSE DOES NOT GRANT YOU ANY RIGHT IN ANY TRADEMARK OR LOGO OF
// LICENSOR OR ITS AFFILIATES (PROVIDED THAT YOU MAY USE A TRADEMARK OR LOGO OF
// LICENSOR AS EXPRESSLY REQUIRED BY THIS LICENSE).
//
// TO THE EXTENT PERMITTED BY APPLICABLE LAW, THE LICENSED WORK IS PROVIDED ON
// AN “AS IS” BASIS. LICENSOR HEREBY DISCLAIMS ALL WARRANTIES AND CONDITIONS,
// EXPRESS OR IMPLIED, INCLUDING (WITHOUT LIMITATION) WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE, NON-INFRINGEMENT, AND
// TITLE.

package beacondb

import (
"errors"

"cosmossdk.io/collections"
)

var ErrNotFound = errors.New("beacondb: not found")
abi87 marked this conversation as resolved.
Show resolved Hide resolved

// mapErrors ensure that we replace collections.ErrNotFound error
// with ErrNotFound. This allows beacond clients to implement custom
// logic when items they were quering are not available, while loosening
// dependencies on cosmos sdk package.
func mapErrors(err error) error {
switch {
case err == nil:
return nil
case errors.Is(err, collections.ErrNotFound):
return ErrNotFound
default:
return err
}
}
abi87 marked this conversation as resolved.
Show resolved Hide resolved
70 changes: 60 additions & 10 deletions mod/storage/pkg/beacondb/eth1.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

package beacondb

import (
"fmt"
)

// GetLatestExecutionPayloadHeader retrieves the latest execution payload
// header from the BeaconStore.
func (kv *KVStore[
Expand All @@ -28,13 +32,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 = mapErrors(err)
if err != nil {
var t ExecutionPayloadHeaderT
return t, err
return h, fmt.Errorf(
"failed retrieving latest execution payload header: %w",
err,
)
}
kv.latestExecutionPayloadCodec.SetActiveForkVersion(forkVersion)
return kv.latestExecutionPayloadHeader.Get(kv.ctx)
return h, nil
}

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

// SetLatestExecutionPayloadHeader sets the latest execution payload header in
Expand All @@ -45,9 +76,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 +92,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 = mapErrors(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 +119,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 = mapErrors(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
11 changes: 10 additions & 1 deletion mod/storage/pkg/beacondb/fork.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

package beacondb

import (
"fmt"
)

// SetFork sets the fork version for the given epoch.
func (kv *KVStore[
BeaconBlockHeaderT, Eth1DataT, ExecutionPayloadHeaderT,
Expand All @@ -35,5 +39,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 = mapErrors(err)
if err != nil {
return f, fmt.Errorf("failed retrieving fork: %w", err)
}
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return f, nil
}
30 changes: 26 additions & 4 deletions mod/storage/pkg/beacondb/history.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@

package beacondb

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

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

// UpdateBlockRootAtIndex sets a block root in the BeaconStore.
func (kv *KVStore[
Expand All @@ -41,8 +45,13 @@ func (kv *KVStore[
index uint64,
) (common.Root, error) {
bz, err := kv.blockRoots.Get(kv.ctx, index)
err = mapErrors(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 +73,15 @@ func (kv *KVStore[
]) GetLatestBlockHeader() (
BeaconBlockHeaderT, error,
) {
return kv.latestBlockHeader.Get(kv.ctx)
h, err := kv.latestBlockHeader.Get(kv.ctx)
err = mapErrors(err)
if err != nil {
return h, fmt.Errorf(
"failed retrieving latest block header: %w",
err,
)
}
return h, nil
}

// UpdateStateRootAtIndex updates the state root at the given slot.
Expand All @@ -86,8 +103,13 @@ func (kv *KVStore[
idx uint64,
) (common.Root, error) {
bz, err := kv.stateRoots.Get(kv.ctx, idx)
err = mapErrors(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
}
13 changes: 11 additions & 2 deletions mod/storage/pkg/beacondb/randao.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@

package beacondb

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

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

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