Skip to content

Commit

Permalink
polygon/sync: fix parent td missing when forking at the tip (#11867)
Browse files Browse the repository at this point in the history 
fixes #11818

issue was:
- when at tip we receive new block hashes and new block events
- we had an if statement which checked if the canonical chain builder
tip changed after connecting new headers to the tree
- that if statement was used to determine whether we should call
`InsertBlocks` for the blocks we've just connected and also to
`commitExecution` (call `UpdateForkChoice`)
- this meant that when at the tip, we would not insert new blocks which
would not change the tip of the canonical chain builder
- this is wrong because we should be inserting these blocks as they may
end up being on the canonical path several blocks later in case the
forks change in their favour based on the connected ancestors

fix is:
- augment `canonicalChainBuilder.Connect` to return the newly connected
headers to the tree
- always insert newly connected headers (upon successful connection to
the root)
  • Loading branch information
@taratorio
taratorio authored Sep 4, 2024
1 parent c6a47ad commit ed2f5f8
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 64 deletions.
28 changes: 16 additions & 12 deletions polygon/sync/canonical_chain_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type CanonicalChainBuilder interface {
Root() *types.Header
HeadersInRange(start uint64, count uint64) []*types.Header
Prune(newRootNum uint64) error
Connect(ctx context.Context, headers []*types.Header) error
Connect(ctx context.Context, headers []*types.Header) (newConnectedHeaders []*types.Header, err error)
}

type producerSlotIndex uint64
Expand Down Expand Up @@ -195,17 +195,20 @@ func (ccb *canonicalChainBuilder) updateTipIfNeeded(tipCandidate *forkTreeNode)
}
}

func (ccb *canonicalChainBuilder) Connect(ctx context.Context, headers []*types.Header) error {
// Connect connects a list of headers to the canonical chain builder tree.
// Returns the list of newly connected headers (filtering out headers that already exist in the tree)
// or an error in case the header is invalid or the header chain cannot reach any of the nodes in the tree.
func (ccb *canonicalChainBuilder) Connect(ctx context.Context, headers []*types.Header) ([]*types.Header, error) {
if (len(headers) > 0) && (headers[0].Number != nil) && (headers[0].Number.Cmp(ccb.root.header.Number) == 0) {
headers = headers[1:]
}
if len(headers) == 0 {
return nil
return nil, nil
}

parent := ccb.nodeByHash(headers[0].ParentHash)
if parent == nil {
return errors.New("canonicalChainBuilder.Connect: can't connect headers")
return nil, errors.New("canonicalChainBuilder.Connect: can't connect headers")
}

headersHashes := libcommon.SliceMap(headers, func(header *types.Header) libcommon.Hash {
Expand All @@ -215,7 +218,7 @@ func (ccb *canonicalChainBuilder) Connect(ctx context.Context, headers []*types.
// check if headers are linked by ParentHash
for i, header := range headers[1:] {
if header.ParentHash != headersHashes[i] {
return errors.New("canonicalChainBuilder.Connect: invalid headers slice ParentHash")
return nil, errors.New("canonicalChainBuilder.Connect: invalid headers slice ParentHash")
}
}

Expand All @@ -239,35 +242,36 @@ func (ccb *canonicalChainBuilder) Connect(ctx context.Context, headers []*types.

// if all headers are already inserted
if len(headers) == 0 {
return nil
return nil, nil
}

// attach nodes for the new headers
for i, header := range headers {
if (header.Number == nil) || (header.Number.Uint64() != parent.header.Number.Uint64()+1) {
return errors.New("canonicalChainBuilder.Connect: invalid header.Number")
return nil, errors.New("canonicalChainBuilder.Connect: invalid header.Number")
}

if err := ccb.headerValidator.ValidateHeader(ctx, header, parent.header, time.Now()); err != nil {
return fmt.Errorf("canonicalChainBuilder.Connect: invalid header error %w", err)
return nil, fmt.Errorf("canonicalChainBuilder.Connect: invalid header error %w", err)
}

difficulty, err := ccb.difficultyCalc.HeaderDifficulty(ctx, header)
if err != nil {
return fmt.Errorf("canonicalChainBuilder.Connect: header difficulty error %w", err)
return nil, fmt.Errorf("canonicalChainBuilder.Connect: header difficulty error %w", err)
}
if (header.Difficulty == nil) || (header.Difficulty.Uint64() != difficulty) {
return &bor.WrongDifficultyError{
err := &bor.WrongDifficultyError{
Number: header.Number.Uint64(),
Expected: difficulty,
Actual: header.Difficulty.Uint64(),
Signer: []byte{},
}
return nil, err
}

slot := producerSlotIndex(difficulty)
if _, ok := parent.children[slot]; ok {
return errors.New("canonicalChainBuilder.Connect: producer slot is already filled by a different header")
return nil, errors.New("canonicalChainBuilder.Connect: producer slot is already filled by a different header")
}

node := &forkTreeNode{
Expand All @@ -285,5 +289,5 @@ func (ccb *canonicalChainBuilder) Connect(ctx context.Context, headers []*types.
ccb.updateTipIfNeeded(node)
}

return nil
return headers, nil
}
89 changes: 54 additions & 35 deletions polygon/sync/canonical_chain_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,14 @@ func (test *connectCCBTest) testConnect(
headers []*types.Header,
expectedTip *types.Header,
expectedHeaders []*types.Header,
expectedNewConnectedHeaders []*types.Header,
) {
t := test.t
builder := test.builder

err := builder.Connect(ctx, headers)
require.Nil(t, err)
newConnectedHeaders, err := builder.Connect(ctx, headers)
require.NoError(t, err)
require.Equal(t, expectedNewConnectedHeaders, newConnectedHeaders)

newTip := builder.Tip()
assert.Equal(t, expectedTip.Hash(), newTip.Hash())
Expand Down Expand Up @@ -139,15 +141,15 @@ func TestCCBConnectEmpty(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
test.testConnect(ctx, []*types.Header{}, root, []*types.Header{root})
test.testConnect(ctx, []*types.Header{}, root, []*types.Header{root}, nil)
}

// connect 0 to 0
func TestCCBConnectRoot(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
test.testConnect(ctx, []*types.Header{root}, root, []*types.Header{root})
test.testConnect(ctx, []*types.Header{root}, root, []*types.Header{root}, nil)
}

// connect 1 to 0
Expand All @@ -156,7 +158,7 @@ func TestCCBConnectOneToRoot(t *testing.T) {
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
newTip := test.makeHeader(root, 1)
test.testConnect(ctx, []*types.Header{newTip}, newTip, []*types.Header{root, newTip})
test.testConnect(ctx, []*types.Header{newTip}, newTip, []*types.Header{root, newTip}, []*types.Header{newTip})
}

// connect 1-2-3 to 0
Expand All @@ -165,7 +167,7 @@ func TestCCBConnectSomeToRoot(t *testing.T) {
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
headers := test.makeHeaders(root, []uint64{1, 2, 3})
test.testConnect(ctx, headers, headers[len(headers)-1], append([]*types.Header{root}, headers...))
test.testConnect(ctx, headers, headers[len(headers)-1], append([]*types.Header{root}, headers...), headers)
}

// connect any subset of 0-1-2-3 to 0-1-2-3
Expand All @@ -174,15 +176,17 @@ func TestCCBConnectOverlapsFull(t *testing.T) {
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
headers := test.makeHeaders(root, []uint64{1, 2, 3})
require.Nil(t, test.builder.Connect(ctx, headers))
newConnectedHeaders, err := test.builder.Connect(ctx, headers)
require.NoError(t, err)
require.Equal(t, headers, newConnectedHeaders)

expectedTip := headers[len(headers)-1]
expectedHeaders := append([]*types.Header{root}, headers...)

for subsetLen := 1; subsetLen <= len(headers); subsetLen++ {
for i := 0; i+subsetLen-1 < len(expectedHeaders); i++ {
headers := expectedHeaders[i : i+subsetLen]
test.testConnect(ctx, headers, expectedTip, expectedHeaders)
test.testConnect(ctx, headers, expectedTip, expectedHeaders, nil)
}
}
}
Expand All @@ -193,7 +197,7 @@ func TestCCBConnectOverlapPartialOne(t *testing.T) {
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
newTip := test.makeHeader(root, 1)
test.testConnect(ctx, []*types.Header{root, newTip}, newTip, []*types.Header{root, newTip})
test.testConnect(ctx, []*types.Header{root, newTip}, newTip, []*types.Header{root, newTip}, []*types.Header{newTip})
}

// connect 2-3-4-5 to 0-1-2-3
Expand All @@ -202,12 +206,15 @@ func TestCCBConnectOverlapPartialSome(t *testing.T) {
t.Cleanup(cancel)
test, root := newConnectCCBTest(t)
headers := test.makeHeaders(root, []uint64{1, 2, 3})
require.Nil(t, test.builder.Connect(ctx, headers))
newConnectedHeaders, err := test.builder.Connect(ctx, headers)
require.NoError(t, err)
require.Equal(t, headers, newConnectedHeaders)

overlapHeaders := append(headers[1:], test.makeHeaders(headers[len(headers)-1], []uint64{4, 5})...)
headers45 := test.makeHeaders(headers[len(headers)-1], []uint64{4, 5})
overlapHeaders := append(headers[1:], headers45...)
expectedTip := overlapHeaders[len(overlapHeaders)-1]
expectedHeaders := append([]*types.Header{root, headers[0]}, overlapHeaders...)
test.testConnect(ctx, overlapHeaders, expectedTip, expectedHeaders)
test.testConnect(ctx, overlapHeaders, expectedTip, expectedHeaders, headers45)
}

// connect 2 to 0-1 at 0, then connect 10 to 0-1
Expand All @@ -217,13 +224,15 @@ func TestCCBConnectAltMainBecomesFork(t *testing.T) {
test, root := newConnectCCBTest(t)
header1 := test.makeHeader(root, 1)
header2 := test.makeHeader(root, 2)
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header1}))
newConnectedHeaders, err := test.builder.Connect(ctx, []*types.Header{header1})
require.NoError(t, err)
require.Equal(t, []*types.Header{header1}, newConnectedHeaders)

// the tip changes to header2
test.testConnect(ctx, []*types.Header{header2}, header2, []*types.Header{root, header2})
test.testConnect(ctx, []*types.Header{header2}, header2, []*types.Header{root, header2}, []*types.Header{header2})

header10 := test.makeHeader(header1, 10)
test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10})
test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10}, []*types.Header{header10})
}

// connect 1 to 0-2 at 0, then connect 10 to 0-1
Expand All @@ -233,13 +242,15 @@ func TestCCBConnectAltForkBecomesMain(t *testing.T) {
test, root := newConnectCCBTest(t)
header1 := test.makeHeader(root, 1)
header2 := test.makeHeader(root, 2)
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header2}))
newConnectedHeaders, err := test.builder.Connect(ctx, []*types.Header{header2})
require.NoError(t, err)
require.Equal(t, []*types.Header{header2}, newConnectedHeaders)

// the tip stays at header2
test.testConnect(ctx, []*types.Header{header1}, header2, []*types.Header{root, header2})
test.testConnect(ctx, []*types.Header{header1}, header2, []*types.Header{root, header2}, []*types.Header{header1})

header10 := test.makeHeader(header1, 10)
test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10})
test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10}, []*types.Header{header10})
}

// connect 10 and 11 to 1, then 20 and 22 to 2 one by one starting from a [0-1, 0-2] tree
Expand All @@ -253,13 +264,17 @@ func TestCCBConnectAltForksAtLevel2(t *testing.T) {
header2 := test.makeHeader(root, 2)
header20 := test.makeHeader(header2, 20)
header22 := test.makeHeader(header2, 22)
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header1}))
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header2}))

test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10})
test.testConnect(ctx, []*types.Header{header11}, header11, []*types.Header{root, header1, header11})
test.testConnect(ctx, []*types.Header{header20}, header20, []*types.Header{root, header2, header20})
test.testConnect(ctx, []*types.Header{header22}, header22, []*types.Header{root, header2, header22})
newConnectedHeaders, err := test.builder.Connect(ctx, []*types.Header{header1})
require.NoError(t, err)
require.Equal(t, []*types.Header{header1}, newConnectedHeaders)
newConnectedHeaders, err = test.builder.Connect(ctx, []*types.Header{header2})
require.NoError(t, err)
require.Equal(t, []*types.Header{header2}, newConnectedHeaders)

test.testConnect(ctx, []*types.Header{header10}, header10, []*types.Header{root, header1, header10}, []*types.Header{header10})
test.testConnect(ctx, []*types.Header{header11}, header11, []*types.Header{root, header1, header11}, []*types.Header{header11})
test.testConnect(ctx, []*types.Header{header20}, header20, []*types.Header{root, header2, header20}, []*types.Header{header20})
test.testConnect(ctx, []*types.Header{header22}, header22, []*types.Header{root, header2, header22}, []*types.Header{header22})
}

// connect 11 and 10 to 1, then 22 and 20 to 2 one by one starting from a [0-1, 0-2] tree
Expand All @@ -276,14 +291,18 @@ func TestCCBConnectAltForksAtLevel2Reverse(t *testing.T) {
header22 := test.makeHeader(header2, 22)
header100 := test.makeHeader(header10, 100)
header200 := test.makeHeader(header20, 200)
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header1}))
require.Nil(t, test.builder.Connect(ctx, []*types.Header{header2}))

test.testConnect(ctx, []*types.Header{header11}, header11, []*types.Header{root, header1, header11})
test.testConnect(ctx, []*types.Header{header10}, header11, []*types.Header{root, header1, header11})
test.testConnect(ctx, []*types.Header{header22}, header22, []*types.Header{root, header2, header22})
test.testConnect(ctx, []*types.Header{header20}, header22, []*types.Header{root, header2, header22})

test.testConnect(ctx, []*types.Header{header100}, header100, []*types.Header{root, header1, header10, header100})
test.testConnect(ctx, []*types.Header{header200}, header200, []*types.Header{root, header2, header20, header200})
newConnectedHeaders, err := test.builder.Connect(ctx, []*types.Header{header1})
require.NoError(t, err)
require.Equal(t, []*types.Header{header1}, newConnectedHeaders)
newConnectedHeaders, err = test.builder.Connect(ctx, []*types.Header{header2})
require.NoError(t, err)
require.Equal(t, []*types.Header{header2}, newConnectedHeaders)

test.testConnect(ctx, []*types.Header{header11}, header11, []*types.Header{root, header1, header11}, []*types.Header{header11})
test.testConnect(ctx, []*types.Header{header10}, header11, []*types.Header{root, header1, header11}, []*types.Header{header10})
test.testConnect(ctx, []*types.Header{header22}, header22, []*types.Header{root, header2, header22}, []*types.Header{header22})
test.testConnect(ctx, []*types.Header{header20}, header22, []*types.Header{root, header2, header22}, []*types.Header{header20})

test.testConnect(ctx, []*types.Header{header100}, header100, []*types.Header{root, header1, header10, header100}, []*types.Header{header100})
test.testConnect(ctx, []*types.Header{header200}, header200, []*types.Header{root, header2, header20, header200}, []*types.Header{header200})
}
38 changes: 21 additions & 17 deletions polygon/sync/sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,9 @@ func (s *Sync) applyNewBlockOnTip(
return nil
}

var newBlocks []*types.Block
var blockChain []*types.Block
if ccBuilder.ContainsHash(newBlockHeader.ParentHash) {
newBlocks = []*types.Block{event.NewBlock}
blockChain = []*types.Block{event.NewBlock}
} else {
blocks, err := s.p2pService.FetchBlocks(ctx, rootNum, newBlockHeaderNum+1, event.PeerId)
if err != nil {
Expand All @@ -195,10 +195,10 @@ func (s *Sync) applyNewBlockOnTip(
return err
}

newBlocks = blocks.Data
blockChain = blocks.Data
}

if err := s.blocksVerifier(newBlocks); err != nil {
if err := s.blocksVerifier(blockChain); err != nil {
s.logger.Debug(
syncLogPrefix("applyNewBlockOnTip: invalid new block event from peer, penalizing and ignoring"),
"err", err,
Expand All @@ -211,33 +211,37 @@ func (s *Sync) applyNewBlockOnTip(
return nil
}

newHeaders := make([]*types.Header, len(newBlocks))
for i, block := range newBlocks {
newHeaders[i] = block.HeaderNoCopy()
headerChain := make([]*types.Header, len(blockChain))
for i, block := range blockChain {
headerChain[i] = block.HeaderNoCopy()
}

oldTip := ccBuilder.Tip()
if err := ccBuilder.Connect(ctx, newHeaders); err != nil {
newConnectedHeaders, err := ccBuilder.Connect(ctx, headerChain)
if err != nil {
s.logger.Debug(
syncLogPrefix("applyNewBlockOnTip: couldn't connect a header to the local chain tip, ignoring"),
"err", err,
)

return nil
}
if len(newConnectedHeaders) == 0 {
return nil
}

newTip := ccBuilder.Tip()
if newTip != oldTip {
if err := s.store.InsertBlocks(ctx, newBlocks); err != nil {
return err
}
// len(newConnectedHeaders) is always <= len(blockChain)
newConnectedBlocks := blockChain[len(blockChain)-len(newConnectedHeaders):]
if err := s.store.InsertBlocks(ctx, newConnectedBlocks); err != nil {
return err
}

if err := s.commitExecution(ctx, newTip, ccBuilder.Root()); err != nil {
return err
}
newTip := ccBuilder.Tip()
if newTip == oldTip {
return nil
}

return nil
return s.commitExecution(ctx, newTip, ccBuilder.Root())
}

func (s *Sync) applyNewBlockHashesOnTip(
Expand Down

0 comments on commit ed2f5f8

Please sign in to comment.