diff --git a/consensus/XDPoS/XDPoS.go b/consensus/XDPoS/XDPoS.go index 7d8376eee648..9ae22530f9f1 100644 --- a/consensus/XDPoS/XDPoS.go +++ b/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 diff --git a/consensus/XDPoS/verify_chain_reader.go b/consensus/XDPoS/verify_chain_reader.go index ccd35d1f208d..ff0e493ae66a 100644 --- a/consensus/XDPoS/verify_chain_reader.go +++ b/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 } diff --git a/consensus/XDPoS/verify_chain_reader_test.go b/consensus/XDPoS/verify_chain_reader_test.go index 8f1b91a81b86..32fe394b92cd 100644 --- a/consensus/XDPoS/verify_chain_reader_test.go +++ b/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) diff --git a/consensus/tests/engine_v2_tests/verify_header_test.go b/consensus/tests/engine_v2_tests/verify_header_test.go index fa5428cc0cbb..0ae2066eda39 100644 --- a/consensus/tests/engine_v2_tests/verify_header_test.go +++ b/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) + } + } +} diff --git a/core/blockchain.go b/core/blockchain.go index bf8e1aa49324..69cc5e9be03f 100644 --- a/core/blockchain.go +++ b/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 diff --git a/core/headerchain.go b/core/headerchain.go index c1d6150d6da0..f69f25a88a43 100644 --- a/core/headerchain.go +++ b/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