Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

polygon/sync: fix parent td missing when forking at the tip #11867

Merged
merged 5 commits into from
Sep 4, 2024
Merged

polygon/sync: fix parent td missing when forking at the tip #11867

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20b61be
polygon/sync: fix parent td missing when forking at the tip
taratorio Sep 4, 2024
a0545a0
Merge branch 'main' of github.com:ledgerwatch/erigon into astrid-pare…
taratorio Sep 4, 2024
caca460
rename
taratorio Sep 4, 2024
9d9653b
correct comment
taratorio Sep 4, 2024
671ad82
correction to comment and removing unnecessary call
taratorio Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions polygon/sync/canonical_chain_builder.go
View file
Open in desktop
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
Open in desktop
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})
}
39 changes: 22 additions & 17 deletions polygon/sync/sync.go
View file
Open in desktop
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,38 @@ 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 {
block.Hash()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is block.Hash() called here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, I accidentally left it over after debugging

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(blockChain) is always <= len(newConnectedHeaders)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch, corrected

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
Loading