Progressive list type-level limits - #10106
Conversation
|
I need to fix the tests as well |
michaelsproul
added
ready-for-review
|
Ready for review. I think we can just leave the transactions unbounded for now. You can fit like 2.6M of them in the 10MB gossip limit, which is only a little higher than the 1M limit we could enforce |
| let operation = match self.operation.as_ref().ok_or(Error::SkippedBls)? { | ||
| Ok(operation) => operation, | ||
| Err(error) => return compare_result::<BeaconState<E>, _>(&Err(error), &self.post), | ||
| }; |
There was a problem hiding this comment.
We are accepting any error from decode when post is None A missing file or some decoder bug would pass this test silently. For bls test cases we pass the test when it matches a specific error variant. I think we should do the same here
There was a problem hiding this comment.
had a quick look at this and it's a bit non-trivial to differentiate the errors, so I've opened an issue:
eserilev
left a comment
eserilev
left a comment
There was a problem hiding this comment.
Code looks good. Ran a kurtosis network against prysm and lodestar with these changes. I got claude to try and stress test all the different bounded lists
- Transactions and BALs: spammed transactiosn w/ large call data and contract deployments w/ generates BALs
- data columns: spamoor w/ max blob count
- deposit requests: burst of deposits to the deposit contract in a single block
- withdraw and consolidations: submit withdraws and consolidations above the payload limit
- voluntary exists & bls to execution changes: submitted batches at the limit via beacon api
- attester and proposer slashings: turn off doppleganger protection and trigger slashings
- Sync: stop and restart the node to trigger range sync
The network was fine. LGTM!
| pub deposits: VariableList<Deposit, E::MaxDeposits>, | ||
| #[superstruct(only(Gloas, Heze), partial_getter(rename = "deposits_progressive"))] | ||
| pub deposits: ProgressiveVariableList<Deposit>, | ||
| pub deposits: ProgressiveVariableList<Deposit, E::MaxDeposits>, |
There was a problem hiding this comment.
Just noting this comment I left on the ssz_types PR: sigp/ssz_types#86 (comment)
pawanjay176
left a comment
pawanjay176
left a comment
There was a problem hiding this comment.
LGTM. I think the remaining issues can be addressed separately, but will wait for Michael to confirm.
|
Queued — the merge queue status continues in this comment ↓. |
|
Yeah I think good to go, we can re-assess how we handle the zero limits in a bit. I'll just push a fix for the test harness thing |
|
Bumped ssz_types in 1a87c12, and merged unstable Merging |
Merge Queue Status
This pull request spent 9 minutes 54 seconds in the queue, with no time running CI. ReasonPull request #10106 has been dequeued by @michaelsproul with a Waiting for
All conditions
Requeued — the merge queue status continues in this comment ↓. |
|
@mergify dequeue |
|
@mergify queue |
Merge Queue Status
This pull request spent 29 minutes 38 seconds in the queue, including 28 minutes 6 seconds running CI. Required conditions to merge
|
4 participants
Issue Addressed
Trying to solve ProgressiveList DoS vectors.
Proposed Changes
Go back to type-level limits like before, using:
Additional Info
Several imperfections we still need to solve:
ssz_typesinitially only enforced the limits in some places. I've now changed it to enforce them everywhere to avoid oversized lists slipping through. For exampleProgressiveVariableList::try_from_iterdidn't enforce the limit but was reachable from indexed attestation JSON decoding (viaquoted_u64_var_list). See: Add optional length limit to ProgressiveVariableList ssz_types#86 (comment).