nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Fix for #6960 and new unit tests for testing dependent batches

Open ak88 opened this issue 1 year ago • 2 comments

Fixes #6960

InsertHeaders now properly handles null when a dependent batch arrives.

The added unit test also showcases different header batches that is allowed and not. Among them are a bit of strange behavior, since we allow a batch to start with null, but not empty.

This is not allowed batch 1-3 header2, header3

This is allowed batch 1-3 null,header2, header3

Also this will fix several instances of ArrayPoolList not being disposed in HeaderSyncFeed.

What types of changes does your code introduce?

  • [x] Bugfix (a non-breaking change that fixes an issue)
  • [ ] New feature (a non-breaking change that adds functionality)
  • [ ] Breaking change (a change that causes existing functionality not to work as expected)
  • [ ] Optimization
  • [ ] Refactoring
  • [ ] Documentation update
  • [ ] Build-related changes
  • [ ] Other: Description

Testing

Requires testing

  • [x] Yes
  • [ ] No

If yes, did you write tests?

  • [x] Yes
  • [ ] No

ak88 avatar May 21 '24 21:05 ak88

why missing headers is not allowed?

EDIT: Ah the response should have same number of items as request - it is protocol rules. But maybe we can work around it?

I was pointing it out because it seems a little odd to allow empty or null in the end, but only null in the start. So if the rule is: The response has to be the same length as reques size, we should not allow empty in the end either.

ak88 avatar May 22 '24 10:05 ak88

why missing headers is not allowed? EDIT: Ah the response should have same number of items as request - it is protocol rules. But maybe we can work around it?

I was pointing it out because it seems a little odd to allow empty or null in the end, but only null in the start. So if the rule is: The response has to be the same length as reques size, we should not allow empty in the end either.

So I think the logic is that response is not allowed to skip anything, but is allowed to trim at the end. In theory there is nothing about it in spec: https://github.com/ethereum/devp2p/blob/master/caps/eth.md#blockheaders-0x04

LukaszRozmej avatar May 22 '24 10:05 LukaszRozmej