Skip to content

fix: report absolute expected deposit index in ErrDepositIndexOutOfOrder #3091

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

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

fix: report absolute expected deposit index in ErrDepositIndexOutOfOrder #3091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5e0075
report absolute deposit index in out-of-order error
rachit77 Apr 23, 2026
1e9f395
fix import alias
rachit77 Apr 23, 2026
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
2 changes: 1 addition & 1 deletion state-transition/core/validation_deposits.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func ValidateNonGenesisDeposits(
//#nosec:G115 // won't overflow in practice.
if blkDepositIndex != depositIndex+uint64(i) {
return errors.Wrapf(ErrDepositIndexOutOfOrder,
"deposit index: %d, expected index: %d", blkDepositIndex, i,
"deposit index: %d, expected index: %d", blkDepositIndex, depositIndex+uint64(i),
)
}

Expand Down
55 changes: 55 additions & 0 deletions state-transition/core/validation_deposits_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@
package core_test

import (
"fmt"
"testing"

"github.com/berachain/beacon-kit/chain"
"github.com/berachain/beacon-kit/config/spec"
"github.com/berachain/beacon-kit/consensus-types/types"
"github.com/berachain/beacon-kit/primitives/common"
"github.com/berachain/beacon-kit/primitives/constants"
core "github.com/berachain/beacon-kit/state-transition/core"
statetransition "github.com/berachain/beacon-kit/testing/state-transition"
Comment on lines 33 to 35

Copilot AI Apr 23, 2026

Copy link

Choose a reason for hiding this comment

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

Redundant import alias: the imported package path ends in /state-transition/core and the package name is already core, so core ".../core" adds an unnecessary alias. This also deviates from existing usage in this repo (e.g., state-transition/core/core_test.go:38 imports it without an alias) and may be flagged by the importas linter in .golangci.yaml. Consider importing without the alias and keeping usages as core.<...> (they will continue to compile).

Copilot uses AI. Check for mistakes.

@rachit77 rachit77 Apr 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed 1e9f395

"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -253,6 +255,59 @@ func TestLocalDepositsExceedBlockDeposits(t *testing.T) {
require.NoError(t, err)
}

// TestDepositIndexOutOfOrderErrorMessage verifies that the error message produced by
// ErrDepositIndexOutOfOrder reports the absolute expected index (depositIndex+i),
// not just the loop counter i. This is a regression test for a bug where a chain
// 100 deposits in would report "expected index: 0" instead of "expected index: 100".
//
//nolint:paralleltest // uses envars
func TestDepositIndexOutOfOrderErrorMessage(t *testing.T) {

@fridrik01 fridrik01 Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is no need to write a test for this

cs := setupChain(t)
_, st, ds, ctx, _, _ := statetransition.SetupTestState(t, cs)

credentials0 := types.NewCredentialsFromExecutionAddress(common.ExecutionAddress{})

// Simulate a chain that has already processed 5 deposits (indices 0–4).
const processedDeposits = uint64(5)

// Seed the deposit store with processedDeposits+1 entries so that the length
// check passes when we submit a single-deposit block.
allDeposits := make(types.Deposits, processedDeposits+1)
for i := range allDeposits {
allDeposits[i] = &types.Deposit{
Pubkey: [48]byte{byte(i)},
Credentials: credentials0,
Amount: cs.MinActivationBalance(),
Index: uint64(i),
}
}
require.NoError(t, ds.EnqueueDeposits(ctx.ConsensusCtx(), allDeposits))

// Advance the state's eth1 deposit index to reflect the already-processed deposits.
require.NoError(t, st.SetEth1DepositIndex(processedDeposits))

// Build a deposit whose index is wrong (0 instead of the expected processedDeposits).
wrongDeposit := &types.Deposit{
Pubkey: [48]byte{0xff},
Credentials: credentials0,
Amount: cs.MinActivationBalance(),
Index: 0, // should be processedDeposits
}

err := core.ValidateNonGenesisDeposits(
ctx.ConsensusCtx(),
st,
ds,
cs.MaxDepositsPerBlock(),
types.Deposits{wrongDeposit},
common.Root{}, // root check is never reached; index check fires first
)
require.Error(t, err)
require.ErrorIs(t, err, core.ErrDepositIndexOutOfOrder)
// The error must report the absolute expected index, not just the loop counter (0).
require.ErrorContains(t, err, fmt.Sprintf("expected index: %d", processedDeposits))
}

func TestLocalDepositsExceedBlockDepositsBadRoot(t *testing.T) {
t.Parallel()
csData := spec.DevnetChainSpecData()
Expand Down
Loading