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

fix(state-transitions): verify deposits against contract #2115

Merged
merged 63 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 60 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
f4a7d79
wip: adding UTs to state transition package
abi87 Oct 29, 2024
0f46172
wip: completed simple UT for state transition package
abi87 Oct 29, 2024
22c7717
wip: minimal execution engine stub
abi87 Oct 29, 2024
05cba80
extended asserts
abi87 Oct 29, 2024
443ac1b
added test case
abi87 Oct 29, 2024
10efd5e
nits
abi87 Oct 29, 2024
099716d
tests for helpers in state transition using mock
nidhi-singh02 Oct 30, 2024
151a533
Revert "tests for helpers in state transition using mock"
nidhi-singh02 Oct 30, 2024
9818c7e
tests with only mock for execution engine
nidhi-singh02 Oct 30, 2024
160cc88
removed test for VerifyAndNotifyNewPayload
nidhi-singh02 Oct 30, 2024
6a191d1
validate deposits against deposit store ones
abi87 Oct 30, 2024
64d19e5
cleaned up UTs
abi87 Oct 30, 2024
d94bf97
nits
abi87 Oct 30, 2024
4a9fe1c
nit
abi87 Oct 31, 2024
67f2597
Merge branch 'state-transition-add-UTs' into verify-deposits-against-…
abi87 Oct 31, 2024
e048be4
improved unit tests asserts
abi87 Oct 31, 2024
8bf34db
appease linter
abi87 Oct 31, 2024
d90a95a
fix(state-transition): fix deposit index upon genesis processing (#2116)
abi87 Oct 31, 2024
e17d29c
fixed bad merge
abi87 Oct 31, 2024
7b2bf91
Merge branch 'state-transition-add-UTs' into verify-deposits-against-…
abi87 Oct 31, 2024
6286b20
Merge branch 'main' into state-transition-add-UTs
abi87 Nov 1, 2024
df81bae
Merge branch 'state-transition-add-UTs' into verify-deposits-against-…
abi87 Nov 1, 2024
af8c5e0
fix(build): erigon repo
gummybera Nov 1, 2024
023ebfd
fix(build): bump erigon to recent version
gummybera Nov 1, 2024
d66b298
nits from code review
abi87 Nov 1, 2024
18ba094
Merge branch 'state-transition-add-UTs' into verify-deposits-against-…
abi87 Nov 4, 2024
e47219a
fixed deposit index use + UTs
abi87 Nov 4, 2024
a3cd2d9
replace DeelEqual with Equal method
abi87 Nov 5, 2024
3a0923e
nits
abi87 Nov 5, 2024
712d3fe
Merge branch 'fix-erigon' into verify-deposits-against-contract
abi87 Nov 5, 2024
6b90b87
added logger to state processor
abi87 Nov 5, 2024
83ad2fd
duly incremented build block deposit index
abi87 Nov 5, 2024
69d568b
tmp debugging
abi87 Nov 5, 2024
a7143e8
improved error expressivity
abi87 Nov 5, 2024
6ce250d
Merge branch 'main' into verify-deposits-against-contract
abi87 Nov 8, 2024
0fc3868
Merge branch 'main' into verify-deposits-against-contract
abi87 Nov 19, 2024
10bd597
improved logging
abi87 Nov 24, 2024
a15eaa3
added logging to deposit store
abi87 Nov 24, 2024
b45654b
added checks on deposit fetcher
abi87 Nov 24, 2024
463489b
reset Eth1DepositIndex
abi87 Nov 24, 2024
333764d
Boonet: fixed Bera token produced at Genesis (#2161)
calbera Nov 20, 2024
8b2e72d
Merge branch 'main' into mockery-2-49-0
abi87 Nov 24, 2024
19c311a
some more log fixes
abi87 Nov 24, 2024
4480026
comment nit
abi87 Nov 24, 2024
9285445
Merge branch 'mockery-2-49-0' into verify-deposits-against-contract
abi87 Nov 24, 2024
4512758
tmp: debugging
abi87 Nov 24, 2024
1aafb3d
Merge branch 'main' into verify-deposits-against-contract
abi87 Nov 25, 2024
11c2688
nit
abi87 Nov 25, 2024
80bb656
special cased Bartio for backward compatibility
abi87 Nov 26, 2024
628dbba
made bug fix backward compatible
abi87 Nov 26, 2024
386087d
nits + properly guarding fix
abi87 Nov 26, 2024
c087141
minor hardening of genesis deposits validation
abi87 Nov 26, 2024
2ad5fc1
Update mod/state-transition/pkg/core/deposits_validation.go
calbera Nov 26, 2024
7a2813b
Update mod/state-transition/pkg/core/deposits_validation.go
calbera Nov 26, 2024
099c5ae
Update mod/state-transition/pkg/core/deposits_validation.go
calbera Nov 26, 2024
94ff29d
guarded block building
abi87 Nov 26, 2024
a75a0c2
deposits validation errors cleanup
calbera Nov 27, 2024
9f47011
nit renaming
calbera Nov 27, 2024
8cf2154
gen
calbera Nov 27, 2024
7d22a92
anchored mockery to v2.49.0
abi87 Nov 27, 2024
84cf57d
appease gosec
abi87 Nov 27, 2024
14f9416
nit
abi87 Nov 27, 2024
8f8a1b3
fixed block building logic
abi87 Nov 27, 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 beacond/cmd/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func DefaultComponents() []any {
*DepositContract, *DepositStore, *ExecutionPayload,
*ExecutionPayloadHeader, *Logger,
],
components.ProvideDepositStore[*Deposit],
components.ProvideDepositStore[*Deposit, *Logger],
components.ProvideDispatcher[
*ConsensusBlock, *BeaconBlock,
*ConsensusSidecars, *BlobSidecars,
Expand Down Expand Up @@ -121,8 +121,8 @@ func DefaultComponents() []any {
],
components.ProvideStateProcessor[
*Logger, *BeaconBlock, *BeaconBlockBody, *BeaconBlockHeader,
*BeaconState, *BeaconStateMarshallable, *Deposit, *ExecutionPayload,
*ExecutionPayloadHeader, *KVStore,
*BeaconState, *BeaconStateMarshallable, *Deposit, *DepositStore,
*ExecutionPayload, *ExecutionPayloadHeader, *KVStore,
abi87 marked this conversation as resolved.
Show resolved Hide resolved
],
components.ProvideKVStore[*BeaconBlockHeader, *ExecutionPayloadHeader],
components.ProvideStorageBackend[
Expand Down
2 changes: 1 addition & 1 deletion build/scripts/codegen.mk
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ generate: ## generate all the code
(cd $$module && \
GETH_PKG_INCLUDE=$(GETH_PKG_INCLUDE) go generate ./...) || exit 1; \
done
@go run github.com/vektra/mockery/v2@latest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

anchored mockery to avoid frequent updates. We can revisit say once per month

@go run github.com/vektra/mockery/v2@v2.49.0
calbera marked this conversation as resolved.
Show resolved Hide resolved

generate-check:
@$(MAKE) forge-build
Expand Down
7 changes: 6 additions & 1 deletion mod/beacon/validator/block_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"time"

payloadtime "github.com/berachain/beacon-kit/mod/beacon/payload-time"
"github.com/berachain/beacon-kit/mod/config/pkg/spec"
engineprimitives "github.com/berachain/beacon-kit/mod/engine-primitives/pkg/engine-primitives"
"github.com/berachain/beacon-kit/mod/primitives/pkg/bytes"
"github.com/berachain/beacon-kit/mod/primitives/pkg/common"
Expand Down Expand Up @@ -288,12 +289,16 @@ func (s *Service[
// Set the KZG commitments on the block body.
body.SetBlobKzgCommitments(blobsBundle.GetCommitments())

// Dequeue deposits from the state.
depositIndex, err := st.GetEth1DepositIndex()
if err != nil {
return ErrNilDepositIndexStart
}

calbera marked this conversation as resolved.
Show resolved Hide resolved
// Dequeue deposits from the state.
if s.chainSpec.DepositEth1ChainID() == spec.BoonetEth1ChainID &&
blk.GetSlot() > math.U64(spec.BoonetFork2Height) {
depositIndex++
}
deposits, err := s.sb.DepositStore().GetDepositsByIndex(
depositIndex,
s.chainSpec.MaxDepositsPerBlock(),
Expand Down
8 changes: 8 additions & 0 deletions mod/config/pkg/spec/special_cases.go
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 @@ -28,3 +30,9 @@ const (
//nolint:lll // temporary.
BArtioValRoot = "0x9147586693b6e8faa837715c0f3071c2000045b54233901c2e7871b15872bc43"
)

const ( // Planned hard-fork upgrades on boonet.
BoonetFork1Height uint64 = 69420

BoonetFork2Height uint64 = math.MaxUint64
)
8 changes: 8 additions & 0 deletions mod/consensus-types/pkg/types/deposit.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ func (d *Deposit) GetTree() (*fastssz.Node, error) {
/* -------------------------------------------------------------------------- */
/* Getters and Setters */
/* -------------------------------------------------------------------------- */
// Equals returns true if the Deposit is equal to the other.
func (d *Deposit) Equals(rhs *Deposit) bool {
return d.Pubkey == rhs.Pubkey &&
d.Credentials == rhs.Credentials &&
d.Amount == rhs.Amount &&
d.Signature == rhs.Signature &&
d.Index == rhs.Index
abi87 marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +191 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

for the fields that are special types, should we use bytes.Equal or the type's Equals method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would say their equals, but we should be good now

}
abi87 marked this conversation as resolved.
Show resolved Hide resolved

// GetAmount returns the deposit amount in gwei.
func (d *Deposit) GetAmount() math.Gwei {
Expand Down
9 changes: 9 additions & 0 deletions mod/execution/pkg/deposit/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ func (s *Service[
BeaconBlockT, _, _, _, _,
]) depositFetcher(ctx context.Context, event async.Event[BeaconBlockT]) {
blockNum := event.Data().GetBody().GetExecutionPayload().GetNumber()
if blockNum < s.eth1FollowDistance {
Copy link
Contributor

Choose a reason for hiding this comment

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

correct me If I'm wrong, but just confirming, this case is only triggered when blockNum is 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as the values are set right now yes. However that is not really hit, since we don't emit a finalized block event on genesis

Copy link
Contributor

@calbera calbera Nov 26, 2024

Choose a reason for hiding this comment

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

Suggested change
if blockNum < s.eth1FollowDistance {
if blockNum < s.eth1FollowDistance + 1 {

Will never need to query the logs from block height 0. So the first block to query for should be 1.

s.logger.Info(
"depositFetcher, nothing to fetch",
"block num", blockNum,
"eth1FollowDistance", s.eth1FollowDistance,
)
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return
}

s.fetchAndStoreDeposits(ctx, blockNum-s.eth1FollowDistance)
}

Expand Down
13 changes: 10 additions & 3 deletions mod/node-core/pkg/components/deposit_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,11 @@ import (
)

// DepositStoreInput is the input for the dep inject framework.
type DepositStoreInput struct {
type DepositStoreInput[
LoggerT log.AdvancedLogger[LoggerT],
] struct {
abi87 marked this conversation as resolved.
Show resolved Hide resolved
depinject.In
Logger LoggerT
AppOpts config.AppOptions
}

Expand All @@ -48,8 +51,9 @@ func ProvideDepositStore[
DepositT Deposit[
DepositT, *ForkData, WithdrawalCredentials,
],
LoggerT log.AdvancedLogger[LoggerT],
](
in DepositStoreInput,
in DepositStoreInput[LoggerT],
) (*depositstore.KVStore[DepositT], error) {
name := "deposits"
dir := cast.ToString(in.AppOpts.Get(flags.FlagHome)) + "/data"
Expand All @@ -58,7 +62,10 @@ func ProvideDepositStore[
return nil, err
}

return depositstore.NewStore[DepositT](storage.NewKVStoreProvider(kvp)), nil
return depositstore.NewStore[DepositT](
storage.NewKVStoreProvider(kvp),
in.Logger.With("service", "deposit-store"),
), nil
abi87 marked this conversation as resolved.
Show resolved Hide resolved
}

// DepositPrunerInput is the input for the deposit pruner.
Expand Down
2 changes: 2 additions & 0 deletions mod/node-core/pkg/components/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,8 @@ type (
crypto.BLSSignature,
uint64,
) T
// Equals returns true if the Deposit is equal to the other.
Equals(T) bool
// GetIndex returns the index of the deposit.
GetIndex() math.U64
// GetAmount returns the amount of the deposit.
Expand Down
8 changes: 6 additions & 2 deletions mod/node-core/pkg/components/state_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type StateProcessorInput[
ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT,
],
ExecutionPayloadHeaderT ExecutionPayloadHeader[ExecutionPayloadHeaderT],
DepositT Deposit[DepositT, *ForkData, WithdrawalCredentials],
WithdrawalT Withdrawal[WithdrawalT],
WithdrawalsT Withdrawals[WithdrawalT],
] struct {
Expand All @@ -50,7 +51,8 @@ type StateProcessorInput[
PayloadID,
WithdrawalsT,
]
Signer crypto.BLSSigner
DepositStore DepositStore[DepositT]
Signer crypto.BLSSigner
}

// ProvideStateProcessor provides the state processor to the depinject
Expand All @@ -70,6 +72,7 @@ func ProvideStateProcessor[
],
BeaconStateMarshallableT any,
DepositT Deposit[DepositT, *ForkData, WithdrawalCredentials],
DepositStoreT DepositStore[DepositT],
ExecutionPayloadT ExecutionPayload[
ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT,
],
Expand All @@ -84,7 +87,7 @@ func ProvideStateProcessor[
in StateProcessorInput[
LoggerT,
ExecutionPayloadT, ExecutionPayloadHeaderT,
WithdrawalT, WithdrawalsT,
DepositT, WithdrawalT, WithdrawalsT,
],
) *core.StateProcessor[
BeaconBlockT, BeaconBlockBodyT, BeaconBlockHeaderT,
Expand Down Expand Up @@ -114,6 +117,7 @@ func ProvideStateProcessor[
in.Logger.With("service", "state-processor"),
in.ChainSpec,
in.ExecutionEngine,
in.DepositStore,
in.Signer,
crypto.GetAddressFromPubKey,
abi87 marked this conversation as resolved.
Show resolved Hide resolved
)
Expand Down
158 changes: 158 additions & 0 deletions mod/state-transition/pkg/core/deposits_validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
// 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 core

import (
"fmt"

"github.com/berachain/beacon-kit/mod/config/pkg/spec"
"github.com/berachain/beacon-kit/mod/errors"
"github.com/berachain/beacon-kit/mod/primitives/pkg/math"
)

func (sp *StateProcessor[
_, _, _, BeaconStateT, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {
Comment on lines +31 to +37
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)

Add function documentation

Please add a documentation comment explaining the purpose of this function, its parameters, and return values. This will help other developers understand the validation rules for genesis deposits.

Add this documentation above the function:

+// validateGenesisDeposits validates the deposits provided at genesis according to chain-specific rules.
+// For non-Bartio/Boonet chains, it ensures:
+// 1. No Eth1DepositIndex exists before the first genesis deposit
+// 2. At least one validator exists in genesis
+// 3. Deposit indices are contiguous
+// Parameters:
+//   - st: The beacon state
+//   - deposits: List of deposits to validate
+// Returns an error if validation fails
 func (sp *StateProcessor[
📝 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, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {
// validateGenesisDeposits validates the deposits provided at genesis according to chain-specific rules.
// For non-Bartio/Boonet chains, it ensures:
// 1. No Eth1DepositIndex exists before the first genesis deposit
// 2. At least one validator exists in genesis
// 3. Deposit indices are contiguous
// Parameters:
// - st: The beacon state
// - deposits: List of deposits to validate
// Returns an error if validation fails
func (sp *StateProcessor[
_, _, _, BeaconStateT, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {

switch {
case sp.cs.DepositEth1ChainID() == spec.BartioChainID:
// Bartio does not properly validate deposits index
// We skip checks for backward compatibility
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return nil

case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID:
// Boonet inherited the bug from Bartio and it may have added some
// validators before we activate the fork. So we skip validation
// before fork activation
return nil
abi87 marked this conversation as resolved.
Show resolved Hide resolved

default:
if _, err := st.GetEth1DepositIndex(); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what error is exactly returned here? Should we explicity check that the error contains "not found" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't do now without importing cosmos sdk module here, which we really don't want. I wrote a comment about this but maybe got lost in updates. Let me find it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the very first deposit, where Eth1DepositIndex has not been set yet, we have a ErrNotFound kind of error. This is fine (we haven't even set a single deposit) but it should only happen for the very first deposit (so the check on deposit index)
On the other hand, I agree that we should check the kind of error we receive, and we should behave like that only for ErrNotFound and not others but:

  • ErrNotFound is a cosmosSDK error and we don't want that dependency here (discussed with Dev at some point)
  • We do this in another place (see applyDeposit and how we do create validators
  • I have an old PR that start fixing it (chore(storage): cleanup NotFound errors #2065) and I am willing to refresh that and resubmit for review as soon as this in in

// there should not be Eth1DepositIndex stored before
// genesis first deposit
return errors.Wrap(
ErrDepositMismatch,
"Eth1DepositIndex should be unset at genesis",
)
}
if len(deposits) == 0 {
// there should be at least a validator in genesis
return errors.Wrap(
ErrDepositsLengthMismatch,
"at least one validator should be in genesis",
)
}
for i, deposit := range deposits {
// deposit indices should be contiguous
if deposit.GetIndex() != math.U64(i) {
abi87 marked this conversation as resolved.
Show resolved Hide resolved
return errors.Wrapf(
ErrDepositIndexOutOfOrder,
"genesis deposit index: %d, expected index: %d",
deposit.GetIndex().Unwrap(), i,
)
}
}
return nil
}
}

func (sp *StateProcessor[
_, _, _, BeaconStateT, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateNonGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {
Comment on lines +80 to +86
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)

Add function documentation

Please add a documentation comment explaining the purpose of this function, its parameters, and return values.

Add this documentation above the function:

+// validateNonGenesisDeposits validates non-genesis deposits by comparing them with
+// deposits from the deposit contract. For non-Bartio/Boonet chains (or post-fork),
+// it ensures:
+// 1. Number of deposits matches the contract
+// 2. Deposit indices are contiguous
+// 3. Deposits match exactly with contract deposits
+// Parameters:
+//   - st: The beacon state
+//   - deposits: List of deposits to validate
+// Returns an error if validation fails
 func (sp *StateProcessor[
📝 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, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateNonGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {
// validateNonGenesisDeposits validates non-genesis deposits by comparing them with
// deposits from the deposit contract. For non-Bartio/Boonet chains (or post-fork),
// it ensures:
// 1. Number of deposits matches the contract
// 2. Deposit indices are contiguous
// 3. Deposits match exactly with contract deposits
// Parameters:
// - st: The beacon state
// - deposits: List of deposits to validate
// Returns an error if validation fails
func (sp *StateProcessor[
_, _, _, BeaconStateT, _, DepositT,
_, _, _, _, _, _, _, _, _, _, _,
]) validateNonGenesisDeposits(
st BeaconStateT,
deposits []DepositT,
) error {

slot, err := st.GetSlot()
if err != nil {
return fmt.Errorf(
"failed loading slot while processing deposits: %w", err,
)
}
switch {
case sp.cs.DepositEth1ChainID() == spec.BartioChainID:
// Bartio does not properly validate deposits index
// We skip checks for backward compatibility
return nil

case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID &&
slot < math.U64(spec.BoonetFork2Height):
// Boonet inherited the bug from Bartio and it may have added some
// validators before we activate the fork. So we skip validation
// before fork activation
return nil

default:
// Verify that outstanding deposits match those listed by contract
var depositIndex uint64
depositIndex, err = st.GetEth1DepositIndex()
if err != nil {
return err
calbera marked this conversation as resolved.
Show resolved Hide resolved
}
expectedStartIdx := depositIndex + 1

var localDeposits []DepositT
localDeposits, err = sp.ds.GetDepositsByIndex(
expectedStartIdx,
sp.cs.MaxDepositsPerBlock(),
)
if err != nil {
return err
}
abi87 marked this conversation as resolved.
Show resolved Hide resolved

sp.logger.Info(
"processOperations",
"Expected deposit start index", expectedStartIdx,
"Expected deposits length", len(localDeposits),
)
abi87 marked this conversation as resolved.
Show resolved Hide resolved

if len(localDeposits) != len(deposits) {
return errors.Wrapf(
ErrDepositsLengthMismatch,
"local: %d, payload: %d", len(localDeposits), len(deposits),
)
}

for i, sd := range localDeposits {
// Deposit indices should be contiguous.
if sd.GetIndex().Unwrap() != expectedStartIdx+uint64(i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

order should always be maintained. From spec:

def process_deposit(state: BeaconState, deposit: Deposit) -> None:
    ...

    # Deposits must be processed in order
    state.eth1_deposit_index += 1

    apply_deposit(
        state=state,
        pubkey=deposit.data.pubkey,
        withdrawal_credentials=deposit.data.withdrawal_credentials,
        amount=deposit.data.amount,
        signature=deposit.data.signature,
    )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These checks may be redundant, since these data come from our EVM state and I think they are guaranteed to be in-order. Keeping this for the time being, we will revisit all once merkle root checks are in

return errors.Wrapf(
ErrDepositIndexOutOfOrder,
"local deposit index: %d, expected index: %d",
sd.GetIndex().Unwrap(), expectedStartIdx+uint64(i),
)
}

if !sd.Equals(deposits[i]) {
return errors.Wrapf(
ErrDepositMismatch,
"local deposit: %d, payload deposit: %d",
sd, deposits[i],
)
calbera marked this conversation as resolved.
Show resolved Hide resolved
}
}

return nil
}
}
14 changes: 14 additions & 0 deletions mod/state-transition/pkg/core/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,22 @@ var (
// match the expected value.
ErrSlotMismatch = errors.New("slot mismatch")

// ErrProposerMismatch is returned when proposer referenced in block does
// not match with proposer reported by consensus.
ErrProposerMismatch = errors.New("proposer key mismatch")

// ErrDepositsLengthMismatch is returned when length of deposits
// listed in block is different from deposits from store.
ErrDepositsLengthMismatch = errors.New("deposits lengths mismatched")

// ErrDepositMismatch is returned when a specific deposit listed in
// block is different from the corrispondent one from store.
ErrDepositMismatch = errors.New("deposit mismatched")

// ErrDepositIndexOutOfOrder is returned when deposits are not in
// contiguous order.
ErrDepositIndexOutOfOrder = errors.New("deposit index out of order")
calbera marked this conversation as resolved.
Show resolved Hide resolved

// ErrParentRootMismatch is returned when the parent root in an execution
// payload does not match the expected value.
ErrParentRootMismatch = errors.New("parent root mismatch")
Expand Down
Loading
Loading