consensus: Add extra strict checks and logs for requests config and processing #14024
Conversation
somnathb1
changed the title
consensus/merge/merge.go
Outdated
| @@ -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)) | |||
| @@ -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()) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
| @@ -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()) | |||
There was a problem hiding this comment.
Ditto, better return the error rather than panic.
No description provided.