fix(consensus/XDPoS,core): stop fabricating verify batch blocks, close #2253

consensus/XDPoS/XDPoS.go

@@ -215,7 +215,7 @@ func (x *XDPoS) VerifyHeader(chain consensus.ChainReader, header *types.Header,
 func (x *XDPoS) VerifyHeaders(chain consensus.ChainReader, headers []*types.Header, fullVerifies []bool) (chan<- struct{}, <-chan error) {
 	abort := make(chan struct{})
 	results := make(chan error, len(headers))
-	verifyChain := newVerifyChainReader(chain, headers)
+	verifyChain := NewVerifyHeadersChainReader(chain, headers, nil)
 
 	// Split the headers list into v1 and v2 buckets
 	var v1Headers []*types.Header

consensus/XDPoS/verify_chain_reader.go

@@ -10,6 +10,11 @@ import (
 // verifyChainReader shadows chain lookups with headers in the current verify batch.
 // This keeps verification deterministic when deep consensus paths query ancestors
 // that are in-flight and not written to DB yet.
+//
+// The wrapper intentionally does not fabricate blocks for batch headers. When a
+// caller asks for a block, returning a header-only placeholder would make an
+// unknown block body look like a real empty block and can skew consensus logic
+// that inspects transactions or uncles.
 type verifyChainReader struct {
 	chain           consensus.ChainReader
 	headersByHash   map[common.Hash]*types.Header
@@ -22,12 +27,17 @@ type hashAndNumber struct {
 	number uint64
 }
 
-func newVerifyChainReader(chain consensus.ChainReader, headers []*types.Header) *verifyChainReader {
+var _ consensus.ChainReader = (*verifyChainReader)(nil)
+
+func NewVerifyHeadersChainReader(chain consensus.ChainReader, headers []*types.Header, blocks []*types.Block) consensus.ChainReader {
+	if reader, ok := chain.(*verifyChainReader); ok {
+		return reader
+	}
 	reader := &verifyChainReader{
 		chain:           chain,
 		headersByHash:   make(map[common.Hash]*types.Header, len(headers)),
 		headersByNumber: make(map[uint64]*types.Header, len(headers)),
-		blocksByHashNo:  make(map[hashAndNumber]*types.Block, len(headers)),
+		blocksByHashNo:  make(map[hashAndNumber]*types.Block, len(blocks)),
 	}
 	for _, header := range headers {
 		if header == nil || header.Number == nil {
@@ -39,7 +49,12 @@ func newVerifyChainReader(chain consensus.ChainReader, headers []*types.Header)
 		if _, exists := reader.headersByNumber[n]; !exists {
 			reader.headersByNumber[n] = header
 		}
-		reader.blocksByHashNo[hashAndNumber{hash: h, number: n}] = types.NewBlockWithHeader(header)
+	}
+	for _, block := range blocks {
+		if block == nil {
+			continue
+		}
+		reader.blocksByHashNo[hashAndNumber{hash: block.Hash(), number: block.NumberU64()}] = block
 	}
 	return reader
 }

consensus/XDPoS/verify_chain_reader_test.go

@@ -16,8 +16,16 @@ type stubChainReader struct {
 	config          *params.ChainConfig
 	headersByHash   map[common.Hash]*types.Header
 	headersByNumber map[uint64]*types.Header
+	blocksByHashNo  map[blockKey]*types.Block
 }
 
+type blockKey struct {
+	hash   common.Hash
+	number uint64
+}
+
+var _ consensus.ChainReader = (*stubChainReader)(nil)
+
 func (s *stubChainReader) Config() *params.ChainConfig { return s.config }
 
 func (s *stubChainReader) CurrentHeader() *types.Header { return nil }
@@ -44,10 +52,15 @@ func (s *stubChainReader) GetHeaderByHash(hash common.Hash) *types.Header {
 	return s.headersByHash[hash]
 }
 
-func (s *stubChainReader) GetBlock(common.Hash, uint64) *types.Block { return nil }
+func (s *stubChainReader) GetBlock(hash common.Hash, number uint64) *types.Block {
+	if s.blocksByHashNo == nil {
+		return nil
+	}
+	return s.blocksByHashNo[blockKey{hash: hash, number: number}]
+}
 
 func TestNewVerifyChainReaderWithNilChainReturnsNilSafeReader(t *testing.T) {
-	reader := newVerifyChainReader(nil, []*types.Header{{Number: big.NewInt(1)}})
+	reader := NewVerifyHeadersChainReader(nil, []*types.Header{{Number: big.NewInt(1)}}, nil).(*verifyChainReader)
 	assert.NotNil(t, reader)
 	assert.Nil(t, reader.Config())
 	assert.Nil(t, reader.CurrentHeader())
@@ -67,7 +80,7 @@ func TestVerifyChainReaderShadowsHeaderByNumber(t *testing.T) {
 		headersByNumber: map[uint64]*types.Header{100: baseHeader},
 	}
 
-	reader := newVerifyChainReader(base, []*types.Header{batchHeader})
+	reader := NewVerifyHeadersChainReader(base, []*types.Header{batchHeader}, nil).(*verifyChainReader)
 	resolved := reader.GetHeaderByNumber(100)
 	assert.NotNil(t, resolved)
 	assert.Equal(t, batchHeader.Hash(), resolved.Hash())
@@ -83,12 +96,70 @@ func TestVerifyChainReaderResolvesParentFromBatch(t *testing.T) {
 		headersByNumber: map[uint64]*types.Header{},
 	}
 
-	reader := newVerifyChainReader(base, []*types.Header{parent, child})
+	reader := NewVerifyHeadersChainReader(base, []*types.Header{parent, child}, nil).(*verifyChainReader)
 	resolved := reader.GetHeader(child.ParentHash, child.Number.Uint64()-1)
 	assert.NotNil(t, resolved)
 	assert.Equal(t, parent.Hash(), resolved.Hash())
 }
 
+func TestVerifyChainReaderDoesNotFabricateBatchBlocks(t *testing.T) {
+	batchHeader := &types.Header{Number: big.NewInt(100), Time: 2}
+
+	base := &stubChainReader{
+		config:          params.TestXDPoSMockChainConfig,
+		headersByHash:   map[common.Hash]*types.Header{},
+		headersByNumber: map[uint64]*types.Header{},
+	}
+
+	reader := NewVerifyHeadersChainReader(base, []*types.Header{batchHeader}, nil).(*verifyChainReader)
+	assert.Nil(t, reader.GetBlock(batchHeader.Hash(), batchHeader.Number.Uint64()))
+}
+
+func TestVerifyChainReaderExposesRealBatchBlocks(t *testing.T) {
+	batchHeader := &types.Header{Number: big.NewInt(100), Time: 2}
+	batchBlock := types.NewBlockWithHeader(batchHeader)
+
+	base := &stubChainReader{
+		config:          params.TestXDPoSMockChainConfig,
+		headersByHash:   map[common.Hash]*types.Header{},
+		headersByNumber: map[uint64]*types.Header{},
+	}
+
+	reader := NewVerifyHeadersChainReader(base, []*types.Header{batchHeader}, []*types.Block{batchBlock}).(*verifyChainReader)
+	assert.Same(t, batchBlock, reader.GetBlock(batchHeader.Hash(), batchHeader.Number.Uint64()))
+}
+
+func TestVerifyChainReaderDelegatesBlockLookupToBaseChain(t *testing.T) {
+	baseHeader := &types.Header{Number: big.NewInt(100), Time: 1}
+	baseBlock := types.NewBlockWithHeader(baseHeader)
+
+	base := &stubChainReader{
+		config:          params.TestXDPoSMockChainConfig,
+		headersByHash:   map[common.Hash]*types.Header{baseHeader.Hash(): baseHeader},
+		headersByNumber: map[uint64]*types.Header{100: baseHeader},
+		blocksByHashNo:  map[blockKey]*types.Block{{hash: baseHeader.Hash(), number: 100}: baseBlock},
+	}
+
+	reader := NewVerifyHeadersChainReader(base, nil, nil).(*verifyChainReader)
+	assert.Same(t, baseBlock, reader.GetBlock(baseHeader.Hash(), 100))
+}
+
+func TestVerifyChainReaderReusesExistingWrapper(t *testing.T) {
+	firstHeader := &types.Header{Number: big.NewInt(100), Time: 1}
+	secondHeader := &types.Header{Number: big.NewInt(101), Time: 2}
+	firstBlock := types.NewBlockWithHeader(firstHeader)
+	secondBlock := types.NewBlockWithHeader(secondHeader)
+
+	reader := NewVerifyHeadersChainReader(nil, []*types.Header{firstHeader}, []*types.Block{firstBlock})
+	reused := NewVerifyHeadersChainReader(reader, []*types.Header{secondHeader}, []*types.Block{secondBlock})
+
+	assert.Same(t, reader, reused)
+	wrapped := reused.(*verifyChainReader)
+	assert.Same(t, firstBlock, wrapped.GetBlock(firstHeader.Hash(), firstHeader.Number.Uint64()))
+	assert.Nil(t, wrapped.GetHeaderByNumber(secondHeader.Number.Uint64()))
+	assert.Nil(t, wrapped.GetBlock(secondHeader.Hash(), secondHeader.Number.Uint64()))
+}
+
 func TestVerifyHeadersMixedWithNilChainDoesNotPanic(t *testing.T) {
 	database := rawdb.NewMemoryDatabase()
 	config := params.TestXDPoSMockChainConfig
@@ -139,5 +210,3 @@ func TestVerifyHeadersMixedEmitsV1ThenV2(t *testing.T) {
 	assert.NoError(t, err1)
 	assert.Error(t, err2)
 }
-
-var _ consensus.ChainReader = (*stubChainReader)(nil)

consensus/tests/engine_v2_tests/verify_header_test.go

@@ -2,6 +2,7 @@ package engine_v2_tests
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"math/big"
 	"testing"
@@ -603,3 +604,77 @@ func TestShouldVerifyPureV2EpochSwitchHeadersEvenIfParentNotYetWrittenIntoDB(t *
 		}
 	}
 }
+
+func TestVerifyHeadersDoesNotFabricateBatchBlocksForHookPenalty(t *testing.T) {
+	skipLongInShortMode(t)
+	b, err := json.Marshal(params.TestXDPoSMockChainConfig)
+	assert.Nil(t, err)
+
+	var config params.ChainConfig
+	err = json.Unmarshal(b, &config)
+	assert.Nil(t, err)
+
+	blockchain, _, block1798, signer, signFn, _ := PrepareXDCTestBlockChainForV2Engine(t, 1798, &config, nil)
+	adaptor := blockchain.Engine().(*XDPoS.XDPoS)
+
+	originalHookPenalty := adaptor.EngineV2.HookPenalty
+	t.Cleanup(func() {
+		adaptor.EngineV2.HookPenalty = originalHookPenalty
+	})
+	expectedErr := errors.New("batch block body must not be fabricated")
+	adaptor.EngineV2.HookPenalty = func(chain consensus.ChainReader, number *big.Int, parentHash common.Hash, candidates []common.Address) ([]common.Address, error) {
+		parentNumber := number.Uint64() - 1
+		parentBlock := chain.GetBlock(parentHash, parentNumber)
+		if parentBlock != nil {
+			return nil, expectedErr
+		}
+		if originalHookPenalty == nil {
+			return []common.Address{}, nil
+		}
+		return originalHookPenalty(chain, number, parentHash, candidates)
+	}
+
+	block1799 := CreateBlock(
+		blockchain,
+		&config,
+		block1798,
+		1799,
+		int64(1799)-config.XDPoS.V2.SwitchBlock.Int64(),
+		signer.Hex(),
+		signer,
+		signFn,
+		nil,
+		nil,
+		"",
+	)
+	block1800 := CreateBlock(
+		blockchain,
+		&config,
+		block1799,
+		1800,
+		int64(1800)-config.XDPoS.V2.SwitchBlock.Int64(),
+		signer.Hex(),
+		signer,
+		signFn,
+		nil,
+		nil,
+		"",
+	)
+
+	headers := []*types.Header{block1799.Header(), block1800.Header()}
+	fullVerifies := []bool{true, true}
+
+	_, results := adaptor.VerifyHeaders(blockchain, headers, fullVerifies)
+	for i := 0; i < len(headers); i++ {
+		select {
+		case result := <-results:
+			if i == 0 {
+				assert.Nil(t, result)
+				continue
+			}
+			assert.False(t, errors.Is(result, expectedErr), "VerifyHeaders should not observe fabricated batch blocks")
+		case <-time.After(5 * time.Second):
+			t.Fatalf("timed out waiting for verify result %d", i)
+		}
+	}
+}

core/blockchain.go

@@ -1525,7 +1525,11 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals bool) (int, []
 		seals[i] = verifySeals
 		bc.downloadingBlock.Add(block.Hash(), struct{}{})
 	}
-	abort, results := bc.engine.VerifyHeaders(bc, headers, seals)
+	verifier := consensus.ChainReader(bc)
+	if _, ok := bc.engine.(*XDPoS.XDPoS); ok {
+		verifier = XDPoS.NewVerifyHeadersChainReader(bc, headers, chain)
+	}
+	abort, results := bc.engine.VerifyHeaders(verifier, headers, seals)
 	defer close(abort)
 
 	// Peek the error for the first block to decide the directing import logic

core/headerchain.go

@@ -29,6 +29,7 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/common"
 	"github.com/XinFinOrg/XDPoSChain/common/lru"
 	"github.com/XinFinOrg/XDPoSChain/consensus"
+	"github.com/XinFinOrg/XDPoSChain/consensus/XDPoS"
 	"github.com/XinFinOrg/XDPoSChain/core/rawdb"
 	"github.com/XinFinOrg/XDPoSChain/core/types"
 	"github.com/XinFinOrg/XDPoSChain/ethdb"
@@ -246,7 +247,11 @@ func (hc *HeaderChain) ValidateHeaderChain(chain []*types.Header, checkFreq int)
 		seals[len(seals)-1] = true
 	}
 
-	abort, results := hc.engine.VerifyHeaders(hc, chain, seals)
+	verifier := consensus.ChainReader(hc)
+	if _, ok := hc.engine.(*XDPoS.XDPoS); ok {
+		verifier = XDPoS.NewVerifyHeadersChainReader(hc, chain, nil)
+	}
+	abort, results := hc.engine.VerifyHeaders(verifier, chain, seals)
 	defer close(abort)
 
 	// Iterate over the headers and ensure they all check out