Skip to content

Commit

Permalink
eth/catalyst: fix edge case in NewPayload (#24955)
Browse files Browse the repository at this point in the history 
Fixes an issue where we would accept a NewPayload where the grandparent is already post ttd, and the parent still has a Difficulty
  • Loading branch information
@MariusVanDerWijden
MariusVanDerWijden authored May 30, 2022
1 parent 8845227 commit 93fe175
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 13 deletions.
19 changes: 14 additions & 5 deletions eth/catalyst/api.go
View file
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("Ignoring pre-merge parent block", "number", params.Number, "hash", params.BlockHash, "td", ptd, "ttd", ttd)
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
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)
}
}

0 comments on commit 93fe175

Please sign in to comment.