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

Jump to bottom

eth/catalyst: fix edge case in NewPayload #24955

Merged
merged 4 commits into from
May 30, 2022
Merged

eth/catalyst: fix edge case in NewPayload #24955

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b2ff79
eth/catalyst: fix edge case in NewPayload
MariusVanDerWijden May 25, 2022
319b985
eth/catalyst: fix edge case in NewPayload
MariusVanDerWijden May 25, 2022
3779b53
eth/catalyst: off-by-one errors
MariusVanDerWijden May 25, 2022
f764f18
eth/catalyst: nit
MariusVanDerWijden May 28, 2022
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
19 changes: 14 additions & 5 deletions eth/catalyst/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,14 @@ func (api *ConsensusAPI) ForkchoiceUpdatedV1(update beacon.ForkchoiceStateV1, pa
log.Error("TDs unavailable for TTD check", "number", block.NumberU64(), "hash", update.HeadBlockHash, "td", td, "parent", block.ParentHash(), "ptd", ptd)
return beacon.STATUS_INVALID, errors.New("TDs unavailable for TDD check")
}
if td.Cmp(ttd) < 0 || (block.NumberU64() > 0 && ptd.Cmp(ttd) > 0) {
if td.Cmp(ttd) < 0 {
log.Error("Refusing beacon update to pre-merge", "number", block.NumberU64(), "hash", update.HeadBlockHash, "diff", block.Difficulty(), "age", common.PrettyAge(time.Unix(int64(block.Time()), 0)))
return beacon.ForkChoiceResponse{PayloadStatus: beacon.INVALID_TERMINAL_BLOCK, PayloadID: nil}, nil
}
if block.NumberU64() > 0 && ptd.Cmp(ttd) >= 0 {
log.Error("Parent block is already post-ttd", "number", block.NumberU64(), "hash", update.HeadBlockHash, "diff", block.Difficulty(), "age", common.PrettyAge(time.Unix(int64(block.Time()), 0)))
return beacon.ForkChoiceResponse{PayloadStatus: beacon.INVALID_TERMINAL_BLOCK, PayloadID: nil}, nil
}
}

if rawdb.ReadCanonicalHash(api.eth.ChainDb(), block.NumberU64()) != update.HeadBlockHash {
Expand Down Expand Up @@ -295,11 +299,16 @@ func (api *ConsensusAPI) NewPayloadV1(params beacon.ExecutableDataV1) (beacon.Pa
// We have an existing parent, do some sanity checks to avoid the beacon client
// triggering too early
var (
td = api.eth.BlockChain().GetTd(parent.Hash(), parent.NumberU64())
ttd = api.eth.BlockChain().Config().TerminalTotalDifficulty
ptd = api.eth.BlockChain().GetTd(parent.Hash(), parent.NumberU64())
ttd = api.eth.BlockChain().Config().TerminalTotalDifficulty
gptd = api.eth.BlockChain().GetTd(parent.ParentHash(), parent.NumberU64()-1)
)
if td.Cmp(ttd) < 0 {
log.Warn("Ignoring pre-merge payload", "number", params.Number, "hash", params.BlockHash, "td", td, "ttd", ttd)
if ptd.Cmp(ttd) < 0 {
log.Warn("Ignoring pre-merge payload", "number", params.Number, "hash", params.BlockHash, "td", ptd, "ttd", ttd)
return beacon.INVALID_TERMINAL_BLOCK, nil
}
if parent.Difficulty().BitLen() > 0 && gptd != nil && gptd.Cmp(ttd) >= 0 {
log.Error("Parent block is already post-ttd", "number", params.Number, "hash", params.BlockHash, "td", ptd, "ttd", ttd)
Copy link
Member

Choose a reason for hiding this comment

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

The log is confusing. Should we just log Ignoring pre-merge parent block?

Perhaps my suggestion is also horrible, but please correct the log.

return beacon.INVALID_TERMINAL_BLOCK, nil
}
if block.Time() <= parent.Time() {
Expand Down
56 changes: 48 additions & 8 deletions eth/catalyst/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ func checkLogEvents(t *testing.T, logsCh <-chan []*types.Log, rmLogsCh <-chan co
func TestInvalidPayloadTimestamp(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()
var (
api = NewConsensusAPI(ethservice)
Expand Down Expand Up @@ -250,7 +249,6 @@ func TestInvalidPayloadTimestamp(t *testing.T) {
func TestEth2NewBlock(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()

var (
Expand Down Expand Up @@ -427,7 +425,6 @@ func startEthService(t *testing.T, genesis *core.Genesis, blocks []*types.Block)
func TestFullAPI(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()
var (
parent = ethservice.BlockChain().CurrentBlock()
Expand Down Expand Up @@ -480,7 +477,6 @@ func setupBlocks(t *testing.T, ethservice *eth.Ethereum, n int, parent *types.Bl
func TestExchangeTransitionConfig(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()
var (
api = NewConsensusAPI(ethservice)
Expand Down Expand Up @@ -543,7 +539,6 @@ CommonAncestor◄─▲── P1 ◄── P2 ◄─ P3 ◄─ ... ◄─ Pn
func TestNewPayloadOnInvalidChain(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()

var (
Expand Down Expand Up @@ -618,7 +613,6 @@ func assembleBlock(api *ConsensusAPI, parentHash common.Hash, params *beacon.Pay
func TestEmptyBlocks(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
ethservice.Merger().ReachTTD()
defer n.Close()

commonAncestor := ethservice.BlockChain().CurrentBlock()
Expand Down Expand Up @@ -734,8 +728,6 @@ func TestTrickRemoteBlockCache(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(10)
nodeA, ethserviceA := startEthService(t, genesis, preMergeBlocks)
nodeB, ethserviceB := startEthService(t, genesis, preMergeBlocks)
ethserviceA.Merger().ReachTTD()
ethserviceB.Merger().ReachTTD()
defer nodeA.Close()
defer nodeB.Close()
for nodeB.Server().NodeInfo().Ports.Listener == 0 {
Expand Down Expand Up @@ -794,3 +786,51 @@ func TestTrickRemoteBlockCache(t *testing.T) {
time.Sleep(100 * time.Millisecond)
}
}

func TestNewPayloadOnInvalidTerminalBlock(t *testing.T) {
genesis, preMergeBlocks := generatePreMergeChain(100)
fmt.Println(genesis.Config.TerminalTotalDifficulty)
genesis.Config.TerminalTotalDifficulty = preMergeBlocks[0].Difficulty() //.Sub(genesis.Config.TerminalTotalDifficulty, preMergeBlocks[len(preMergeBlocks)-1].Difficulty())

fmt.Println(genesis.Config.TerminalTotalDifficulty)
n, ethservice := startEthService(t, genesis, preMergeBlocks)
defer n.Close()

var (
api = NewConsensusAPI(ethservice)
parent = preMergeBlocks[len(preMergeBlocks)-1]
)

// Test parent already post TTD in FCU
fcState := beacon.ForkchoiceStateV1{
HeadBlockHash: parent.Hash(),
SafeBlockHash: common.Hash{},
FinalizedBlockHash: common.Hash{},
}
resp, err := api.ForkchoiceUpdatedV1(fcState, nil)
if err != nil {
t.Fatalf("error sending forkchoice, err=%v", err)
}
if resp.PayloadStatus != beacon.INVALID_TERMINAL_BLOCK {
t.Fatalf("error sending invalid forkchoice, invalid status: %v", resp.PayloadStatus.Status)
}

// Test parent already post TTD in NewPayload
params := beacon.PayloadAttributesV1{
Timestamp: parent.Time() + 1,
Random: crypto.Keccak256Hash([]byte{byte(1)}),
SuggestedFeeRecipient: parent.Coinbase(),
}
empty, err := api.eth.Miner().GetSealingBlockSync(parent.Hash(), params.Timestamp, params.SuggestedFeeRecipient, params.Random, true)
if err != nil {
t.Fatalf("error preparing payload, err=%v", err)
}
data := *beacon.BlockToExecutableData(empty)
resp2, err := api.NewPayloadV1(data)
if err != nil {
t.Fatalf("error sending NewPayload, err=%v", err)
}
if resp2 != beacon.INVALID_TERMINAL_BLOCK {
t.Fatalf("error sending invalid forkchoice, invalid status: %v", resp.PayloadStatus.Status)
}
}