From d141ff06c36b0ddc8469d75f82344f2ece2f5853 Mon Sep 17 00:00:00 2001 From: buddh0 Date: Wed, 4 Sep 2024 16:36:37 +0800 Subject: [PATCH 1/2] Revert "eth/handler: check lists in body before broadcast blocks (#2461)" This reverts commit 0c0958ff8709eab1d5d4d0adaa81c09a89ec75d9. --- core/block_validator.go | 46 ++++++++++++++++++----------------------- eth/handler.go | 34 ++++++++++++++++-------------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/core/block_validator.go b/core/block_validator.go index b82965a99d..d15e2cd786 100644 --- a/core/block_validator.go +++ b/core/block_validator.go @@ -66,31 +66,6 @@ func NewBlockValidator(config *params.ChainConfig, blockchain *BlockChain, engin return validator } -// ValidateListsInBody validates that UncleHash, WithdrawalsHash, and WithdrawalsHash correspond to the lists in the block body, respectively. -func ValidateListsInBody(block *types.Block) error { - header := block.Header() - if hash := types.CalcUncleHash(block.Uncles()); hash != header.UncleHash { - return fmt.Errorf("uncle root hash mismatch (header value %x, calculated %x)", header.UncleHash, hash) - } - if hash := types.DeriveSha(block.Transactions(), trie.NewStackTrie(nil)); hash != header.TxHash { - return fmt.Errorf("transaction root hash mismatch: have %x, want %x", hash, header.TxHash) - } - // Withdrawals are present after the Shanghai fork. - if header.WithdrawalsHash != nil { - // Withdrawals list must be present in body after Shanghai. - if block.Withdrawals() == nil { - return errors.New("missing withdrawals in block body") - } - if hash := types.DeriveSha(block.Withdrawals(), trie.NewStackTrie(nil)); hash != *header.WithdrawalsHash { - return fmt.Errorf("withdrawals root hash mismatch (header value %x, calculated %x)", *header.WithdrawalsHash, hash) - } - } else if block.Withdrawals() != nil { // Withdrawals turn into empty from nil when BlockBody has Sidecars - // Withdrawals are not allowed prior to shanghai fork - return errors.New("withdrawals present in block body") - } - return nil -} - // ValidateBody validates the given block's uncles and verifies the block // header's transaction and uncle roots. The headers are assumed to be already // validated at this point. @@ -108,12 +83,31 @@ func (v *BlockValidator) ValidateBody(block *types.Block) error { if err := v.engine.VerifyUncles(v.bc, block); err != nil { return err } + if hash := types.CalcUncleHash(block.Uncles()); hash != header.UncleHash { + return fmt.Errorf("uncle root hash mismatch (header value %x, calculated %x)", header.UncleHash, hash) + } validateFuns := []func() error{ func() error { - return ValidateListsInBody(block) + if hash := types.DeriveSha(block.Transactions(), trie.NewStackTrie(nil)); hash != header.TxHash { + return fmt.Errorf("transaction root hash mismatch: have %x, want %x", hash, header.TxHash) + } + return nil }, func() error { + // Withdrawals are present after the Shanghai fork. + if header.WithdrawalsHash != nil { + // Withdrawals list must be present in body after Shanghai. + if block.Withdrawals() == nil { + return errors.New("missing withdrawals in block body") + } + if hash := types.DeriveSha(block.Withdrawals(), trie.NewStackTrie(nil)); hash != *header.WithdrawalsHash { + return fmt.Errorf("withdrawals root hash mismatch (header value %x, calculated %x)", *header.WithdrawalsHash, hash) + } + } else if block.Withdrawals() != nil { // Withdrawals turn into empty from nil when BlockBody has Sidecars + // Withdrawals are not allowed prior to shanghai fork + return errors.New("withdrawals present in block body") + } // Blob transactions may be present after the Cancun fork. var blobs int for i, tx := range block.Transactions() { diff --git a/eth/handler.go b/eth/handler.go index cc3bf382b4..f65515166d 100644 --- a/eth/handler.go +++ b/eth/handler.go @@ -320,22 +320,26 @@ func newHandler(config *handlerConfig) (*handler, error) { } broadcastBlockWithCheck := func(block *types.Block, propagate bool) { + // All the block fetcher activities should be disabled + // after the transition. Print the warning log. + if h.merger.PoSFinalized() { + log.Warn("Unexpected validation activity", "hash", block.Hash(), "number", block.Number()) + return + } + // Reject all the PoS style headers in the first place. No matter + // the chain has finished the transition or not, the PoS headers + // should only come from the trusted consensus layer instead of + // p2p network. + if beacon, ok := h.chain.Engine().(*beacon.Beacon); ok { + if beacon.IsPoSHeader(block.Header()) { + log.Warn("unexpected post-merge header") + return + } + } if propagate { - checkErrs := make(chan error, 2) - - go func() { - checkErrs <- core.ValidateListsInBody(block) - }() - go func() { - checkErrs <- core.IsDataAvailable(h.chain, block) - }() - - for i := 0; i < cap(checkErrs); i++ { - err := <-checkErrs - if err != nil { - log.Error("Propagating invalid block", "number", block.Number(), "hash", block.Hash(), "err", err) - return - } + if err := core.IsDataAvailable(h.chain, block); err != nil { + log.Error("Propagating block with invalid sidecars", "number", block.Number(), "hash", block.Hash(), "err", err) + return } } h.BroadcastBlock(block, propagate) From 5289ecdfe2185a76c310e1c424b2177f3de43034 Mon Sep 17 00:00:00 2001 From: buddh0 Date: Wed, 4 Sep 2024 16:39:15 +0800 Subject: [PATCH 2/2] eth/protocols: add Withdrawals check before broadcastBlock --- eth/handler.go | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/eth/handler.go b/eth/handler.go index f65515166d..f26162024b 100644 --- a/eth/handler.go +++ b/eth/handler.go @@ -320,23 +320,12 @@ func newHandler(config *handlerConfig) (*handler, error) { } broadcastBlockWithCheck := func(block *types.Block, propagate bool) { - // All the block fetcher activities should be disabled - // after the transition. Print the warning log. - if h.merger.PoSFinalized() { - log.Warn("Unexpected validation activity", "hash", block.Hash(), "number", block.Number()) - return - } - // Reject all the PoS style headers in the first place. No matter - // the chain has finished the transition or not, the PoS headers - // should only come from the trusted consensus layer instead of - // p2p network. - if beacon, ok := h.chain.Engine().(*beacon.Beacon); ok { - if beacon.IsPoSHeader(block.Header()) { - log.Warn("unexpected post-merge header") + if propagate { + if !(block.Header().WithdrawalsHash == nil && block.Withdrawals() == nil) && + !(block.Header().EmptyWithdrawalsHash() && block.Withdrawals() != nil && len(block.Withdrawals()) == 0) { + log.Error("Propagated block has invalid withdrawals") return } - } - if propagate { if err := core.IsDataAvailable(h.chain, block); err != nil { log.Error("Propagating block with invalid sidecars", "number", block.Number(), "hash", block.Hash(), "err", err) return