ethereumjs-monorepo
ethereumjs-monorepo copied to clipboard
ethereumjs
Snap sync: use zero-element proof for checking validity of final, empty range result
During snap sync, if the client requests a range that is both empty and has no elements remaining to the right of it, the peer responds with just a proof. This proof can be checked as part of a zero-element proof to validate that there really are no remaining elements to be fetched past the range. This helps avoid premature halting, corrupt peers, and DoS attacks on snap sync.
Codecov Report
Attention: Patch coverage is 70.96774% with 18 lines in your changes are missing coverage. Please review.
Project coverage is 86.92%. Comparing base (
0e186fd) to head (34a7c12).
Additional details and impacted files
| Flag | Coverage Δ | |
|---|---|---|
| block | 88.34% <ø> (ø) |
|
| blockchain | 91.61% <ø> (ø) |
|
| client | 84.72% <97.43%> (+0.04%) |
:arrow_up: |
| common | 98.25% <ø> (ø) |
|
| devp2p | 82.12% <ø> (ø) |
|
| ethash | ∅ <ø> (∅) |
|
| evm | 74.33% <ø> (ø) |
|
| genesis | 99.98% <ø> (ø) |
|
| rlp | ∅ <ø> (∅) |
|
| statemanager | 77.02% <ø> (ø) |
|
| trie | 89.24% <26.08%> (-0.27%) |
:arrow_down: |
| tx | 95.45% <ø> (ø) |
|
| util | 89.13% <ø> (ø) |
|
| vm | 80.20% <ø> (ø) |
|
| wallet | 88.35% <ø> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@scorbajio This is a somewhat strange order of things with some likelihood to cause confusion with Jochem doing a review and leave some comments and then this PR being set to "Needs review" without the things mentioned actually being adressed. 🤔
Maybe as some general suggestion always good to add some short comment when one is changing the PR state, this always helps to avoid misunderstandings and additionaly avoid of PR state changes getting lost since GitHub does not notify.
Thanks 🙏
Two comments.
The implementation looks correct to me, but it seems that it lacks tests of the "unhappy" case that a peer gives us a zero element proof when there are clearly elements left in the trie (for both account + storage fetcher). Plus a case of the all-elements proof which is not covered 😄 👍
That's a good point 🙂. I've added a basic test for the storage fetcher and account fetcher to test if they will reject a zero-element proof if an element to the right exists, will be able to write more range proof tests once the flat db work is further along and we can create our own range proofs, but the all-elements proofs are already being tested in the trie package (e.g. https://github.com/ethereumjs/ethereumjs-monorepo/blob/5d522f731f6bee1b8626a3558fa8c5d5c0f6ebbd/packages/trie/test/proof/range.spec.ts#L212).