Skip to content

[FIX] Update last block when appending full new block to queue#665

Merged
KonradStaniec merged 4 commits intodevelopfrom
fix/fix-block-fetcher-bug
Sep 17, 2020
Merged

[FIX] Update last block when appending full new block to queue#665
KonradStaniec merged 4 commits intodevelopfrom
fix/fix-block-fetcher-bug

Conversation

@KonradStaniec
Copy link
Copy Markdown
Contributor

@KonradStaniec KonradStaniec commented Sep 11, 2020 •
edited
Loading

Description

When appending lasst full block to ready blocks queue we should update lastblock field otherwise following situation is possilbe:

  1. Receive newBlock X and append it as last to ready block queue
  2. In next fetch cycle lastBlock field is not updated to we ask for headers from the block that we already have
  3. Receive header X and append it to headers queue and download its body and then append it to ready block queue
  4. Send chaing with duplicated X block to importer
  5. It ends up with error InvalidBranch error due the the fact that chains do not form error, peer is unjustly blacklisted.

Such situation can be see here:

10:14:22 [i.i.ethereum.ledger.BranchResolution] - Resolving branch for 4 headers
10:14:22 [i.i.e.b.sync.regular.BlockImporter] - Attempting to import blocks starting from 11180380
10:14:22 [i.i.ethereum.ledger.BranchResolution] - Last headers from new batch is 11180382: a7945532c8a39ccd240b74a916a56fbb6cea051cdcf8b484aa8544636e1dfc6a, current best block is 11180380
10:14:22 [i.i.ethereum.ledger.BranchResolution] - HeadersFormChain is false
10:14:22 [i.i.ethereum.ledger.BranchResolution] - header: BlockHeader {
parentHash: 233d9d5850b5bd3ffac2b11910683b58b442d0b2434b32efe04ded7188e4fe61
ommersHash: 1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347
beneficiary: df7d7e053933b5cc24372f878c90e62dadad5d42
stateRoot: ff559aadd2947de6dd0d1186529f929a693635516a88e3490496c10bc633d1e3
transactionsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
receiptsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
logsBloom: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
difficulty: 29693464424370,
number: 11180380,
gasLimit: 8078373,
gasUsed: 0,
unixTimestamp: 1599811990,
extraData: 505059452d7374726174756d2d65752d31
mixHash: d9f9ec873493c08b6b32ab056d9860b12aaa00e98d218c35d3303d02ec7e252a
nonce: a8b75bf865c60f4a
} with tag: 11180380: 4505d1d223995f36067c8d8eafae56cada04946e31c56b691b1537e05da86116
10:14:22 [i.i.ethereum.ledger.BranchResolution] - header: BlockHeader {
parentHash: 4505d1d223995f36067c8d8eafae56cada04946e31c56b691b1537e05da86116
ommersHash: 1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347
beneficiary: df7d7e053933b5cc24372f878c90e62dadad5d42
stateRoot: 7b2db6076c82bb3cc8ba3e664b731fc3176e6611e26ec7abb359c96c26b0d7dd
transactionsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
receiptsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
logsBloom: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
difficulty: 29620970614745,
number: 11180381,
gasLimit: 8086261,
gasUsed: 0,
unixTimestamp: 1599812047,
extraData: 505059452d7374726174756d2d65752d32
mixHash: d119967b4eca074b48da1fb3472e60fd034203c74da699ecbe4375dabb8ae69d
nonce: 37b3408007372272
} with tag: 11180381: 03dc4fda31064165159cdf13f66bbba250133bcbf0163a4c4d83be0034430b0e
10:14:22 [i.i.ethereum.ledger.BranchResolution] - header: BlockHeader {
parentHash: 4505d1d223995f36067c8d8eafae56cada04946e31c56b691b1537e05da86116
ommersHash: 1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347
beneficiary: df7d7e053933b5cc24372f878c90e62dadad5d42
stateRoot: 7b2db6076c82bb3cc8ba3e664b731fc3176e6611e26ec7abb359c96c26b0d7dd
transactionsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
receiptsRoot: 56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421
logsBloom: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
difficulty: 29620970614745,
number: 11180381,
gasLimit: 8086261,
gasUsed: 0,
unixTimestamp: 1599812047,
extraData: 505059452d7374726174756d2d65752d32
mixHash: d119967b4eca074b48da1fb3472e60fd034203c74da699ecbe4375dabb8ae69d
nonce: 37b3408007372272
} with tag: 11180381: 03dc4fda31064165159cdf13f66bbba250133bcbf0163a4c4d83be0034430b0e
10:14:22 [i.i.ethereum.ledger.BranchResolution] - header: BlockHeader {
parentHash: 03dc4fda31064165159cdf13f66bbba250133bcbf0163a4c4d83be0034430b0e
ommersHash: 1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347
beneficiary: 9eab4b0fc468a7f5d46228bf5a76cb52370d068d
stateRoot: 9556bd37db3c67ecd8574f1f58b81303cfba39cd34e86aac145b72e65b564f1a
transactionsRoot: 72e320320e38af40bb6b62d8e5bf19aab21c62299153bc538ff90b5e78c6215f
receiptsRoot: d9eefb9417bb0d634992b85095f6e6eda92c7ad594c4eefefe5e28e74427f9ac
logsBloom: 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
difficulty: 29635433979302,
number: 11180382,
gasLimit: 8078366,
gasUsed: 338048,
unixTimestamp: 1599812051,
extraData: 6e616e6f706f6f6c2e6f7267
mixHash: 7898a36fd0f47625063e5067c0ab07a1b10a638c842dcdbbf8ce63dfb417fd2c
nonce: 5d1bc100070b93b9
} with tag: 11180382: a7945532c8a39ccd240b74a916a56fbb6cea051cdcf8b484aa8544636e1dfc6a
10:14:22 [i.i.e.b.sync.regular.BlockImporter] - Invalid branch, going back to 11180380
10:14:22 [i.i.e.b.sync.regular.BlockImporter] - Imported no blocks

where block 11180381: 03dc4fda31064165159cdf13f66bbba250133bcbf0163a4c4d83be0034430b0e is duplicated

@ntallar
Copy link
Copy Markdown

ntallar commented Sep 14, 2020

Should we maybe create a test with the situation you described?

Apart from that LGTM!

@KonradStaniec
Copy link
Copy Markdown
Contributor Author

KonradStaniec commented Sep 15, 2020

imo there is no easy way to simulate this particular timing, as new block must arrive just in between the fetch cycle, thats why i only added test to BlockFetcherStateSpec to make sure that appending to state increment this lastblock value.

mmrozek
mmrozek approved these changes Sep 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mmrozek mmrozek left a comment

Choose a reason for hiding this comment

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

LGTM

ntallar
ntallar approved these changes Sep 15, 2020
View reviewed changes
Copy link
Copy Markdown

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

@KonradStaniec KonradStaniec merged commit d788290 into develop Sep 17, 2020
@KonradStaniec KonradStaniec deleted the fix/fix-block-fetcher-bug branch September 17, 2020 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@mmrozek mmrozek mmrozek approved these changes

@ntallar ntallar ntallar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KonradStaniec @ntallar @mmrozek