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

consensus: Add extra strict checks and logs for requests config and processing #14024

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+13 −14
Open

consensus: Add extra strict checks and logs for requests config and processing #14024

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
2db1cde
consensus: Add extra strict checks for requests config and processing
somnathb1 Mar 1, 2025
a14f4c3
Fix spelling
somnathb1 Mar 5, 2025
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
16 changes: 10 additions & 6 deletions consensus/merge/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,18 +198,22 @@ func (s *Merge) Finalize(config *chain.Config, header *types.Header, state *stat
}
if depositReqs != nil {
rs = append(rs, *depositReqs)
log.Debug("Parsed Depsoit Requests", "len", len(depositReqs.RequestData), "data", fmt.Sprintf("0x%x", depositReqs.RequestData))
Copy link
Member

Choose a reason for hiding this comment

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

typo in Depsoit

}
withdrawalReq := misc.DequeueWithdrawalRequests7002(syscall)
if withdrawalReq != nil {
rs = append(rs, *withdrawalReq)
withdrawalReqs := misc.DequeueWithdrawalRequests7002(syscall)
if withdrawalReqs != nil {
rs = append(rs, *withdrawalReqs)
log.Debug("Parsed Withdrawal Requests", "len", len(withdrawalReqs.RequestData), "data", fmt.Sprintf("0x%x", withdrawalReqs.RequestData))
}
consolidations := misc.DequeueConsolidationRequests7251(syscall)
if consolidations != nil {
rs = append(rs, *consolidations)
consolidationReqs := misc.DequeueConsolidationRequests7251(syscall)
if consolidationReqs != nil {
rs = append(rs, *consolidationReqs)
log.Debug("Parsed Consolidation Requests", "len", len(consolidationReqs.RequestData), "data", fmt.Sprintf("0x%x", consolidationReqs.RequestData))
}
if header.RequestsHash != nil {
rh := rs.Hash()
if *header.RequestsHash != *rh {
log.Error(fmt.Sprintf("error: invalid requests root hash in header, expected: %v, got :%v, requestsData: %x", header.RequestsHash, rh, rs))
return nil, nil, nil, fmt.Errorf("error: invalid requests root hash in header, expected: %v, got :%v", header.RequestsHash, rh)
}
}
Expand Down
3 changes: 1 addition & 2 deletions consensus/misc/eip6110.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"

libcommon "github.com/erigontech/erigon-lib/common"
"github.com/erigontech/erigon-lib/log/v3"
"github.com/erigontech/erigon/accounts/abi"
"github.com/erigontech/erigon/core/types"
)
Expand Down Expand Up @@ -73,7 +72,7 @@ func unpackDepositLog(data []byte) ([]byte, error) {
// BeaconDepositContract and returns a FlatRequest object ptr
func ParseDepositLogs(logs []*types.Log, depositContractAddress libcommon.Address) (*types.FlatRequest, error) {
if depositContractAddress == (libcommon.Address{}) {
log.Warn("Error in ParseDepositLogs - depositContractAddress is 0x0")
panic("Error in ParseDepositLogs - depositContractAddress is 0x0")
}
reqData := make([]byte, 0, len(logs)*types.DepositRequestDataLen)
for _, l := range logs {
Expand Down
4 changes: 1 addition & 3 deletions consensus/misc/eip7002.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package misc

import (
"github.com/erigontech/erigon-lib/log/v3"
"github.com/erigontech/erigon/consensus"
"github.com/erigontech/erigon/core/types"
"github.com/erigontech/erigon/params"
Expand All @@ -26,8 +25,7 @@ import (
func DequeueWithdrawalRequests7002(syscall consensus.SystemCall) *types.FlatRequest {
res, err := syscall(params.WithdrawalRequestAddress, nil)
if err != nil {
log.Warn("Err with syscall to WithdrawalRequestAddress", "err", err)
return nil
panic("Err with syscall to WithdrawalRequestAddress " + err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

This is underspecified in the EIP, but I think it's better to return the error if the syscall fails instead of panicking. Theoretically there could be cases that the system contract fails on some blocks but not on others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only think of these scenarios

  • Contract not deployed (should fail)
  • Contract deployed but fails
  • Some other unknown failure in a post-pectra chain

In all of these scenarios, I am proposing to make it stop the node. The error handling that will trickle down will be handled by execution stage, and will loop the error. No value in it, because there is no way forward.

This is to feature a logic that a system contract dependent consensus cannot run half-hearted.

What do you think?

Copy link
Member

@yperbasis yperbasis Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's fine to panic when the contract is not deployed because it's probably a misconfiguration issue. But I'm less sure about when it's deployed but fails. Maybe it's legit that a system contract fails for some calls but not others. Perhaps we should treat such blocks as invalid. A.t.m. it's unspecified. I'd like to clarify it at the ACDE call on 13 Mar: ethereum/pm#1346 (comment)

}
if res != nil {
// Just append the contract output
Expand Down
4 changes: 1 addition & 3 deletions consensus/misc/eip7251.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package misc

import (
"github.com/erigontech/erigon-lib/log/v3"
"github.com/erigontech/erigon/consensus"
"github.com/erigontech/erigon/core/types"
"github.com/erigontech/erigon/params"
Expand All @@ -26,8 +25,7 @@ import (
func DequeueConsolidationRequests7251(syscall consensus.SystemCall) *types.FlatRequest {
res, err := syscall(params.ConsolidationRequestAddress, nil)
if err != nil {
log.Warn("Err with syscall to ConsolidationRequestAddress", "err", err)
return nil
panic("Err with syscall to ConsolidationRequestAddress err " + err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, better return the error rather than panic.

}
if res != nil {
// Just append the contract output as the request data
Expand Down
Loading