Add optional length limit to ProgressiveVariableList

[eserilev]

Add an optional length limit to ProgressiveVariableList

This lets Gloas fields easily enforce their spec count limits

[codecov]

Codecov Report

❌ Patch coverage is 87.34177% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.25%. Comparing base (3110fa0) to head (f865786).

Files with missing lines	Patch %	Lines
src/progressive_variable_list.rs	86.66%	8 Missing ⚠️
src/serde_utils/prog_list_of_hex_fixed_vec.rs	66.66%	1 Missing ⚠️
src/serde_utils/prog_list_of_hex_prog_var_list.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   53.37%   59.25%   +5.88%     
==========================================
  Files          18       18              
  Lines         622      675      +53     
==========================================
+ Hits          332      400      +68     
+ Misses        290      275      -15     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[michaelsproul]

huh, I thought progressive lists didn't have any limits?

where are the limits defined?

[eserilev]

they dont have limits. This adds a guard to decoding so that we dont end up decoding a massive object and OOM ourselves

for example MAX_ATTESTATIONS_ELECTRA in blocks, we should fail early decoding a block whose list of attestations exceeds the max

we dont have to necessarily include the guard here, but I thought it might be the cleanest option

this issue was flagged by one of the security researchers here:
https://discord.com/channels/595666850260713488/892088344438255616/1547691356615479377

[michaelsproul]

my understanding was that we'd try to enforce bounds one level up at the network later, e.g. don't decode a block larger than X kB

I think we have those limits in LH but they are set absurdly high at the moment due to the payload size. We could probably greatly reduce the cap for blocks

[michaelsproul]

I'm coming around to this change, I think we should try to put the length limit checks as "low" and early as possible.

I tried putting them into the decode for BeaconBlock, but even that feels a bit too high because we sometimes construct BeaconBlocks without SSZ decoding (e.g. during proposal, or receiving a JSON BeaconBlock over HTTP).

[michaelsproul]

Impl looks solid, I think this is the way to go.

I'll try cooking up a Lighthouse branch to use these changes.

[michaelsproul]

rebased on main

[michaelsproul]

@eserilev Why did you revert the early failures in Deserialize/ContextDeserialize? (this commit: 31f9025)

They seemed OK to me, did you just want to save on complexity?

[michaelsproul]

I checked all your commits and they look good, so I'm happy to merge this.

[eserilev]

yeah it just seemed overly complex, my b for the last minute changes!

[macladson]

I'm a bit late here but just noting that this could lead to an edge case since we cannot define anything to have a limit of 0. It would instead have no limit. This is most relevant for Deposit which is spec'd with a limit of 0: https://github.com/ethereum/consensus-specs/blob/638af25cd7adea0314c71a6052262c1a5ad6f368/specs/gloas/beacon-chain.md?plain=1#L305-L312

This likely isn't an issue for Lighthouse currently since we reject non-empty deposits either way:

https://github.com/sigp/lighthouse/blob/73e4dc120dfb49fbc5123d4fad42d935a94f368f/beacon_node/beacon_chain/src/block_verification.rs#L910

Theoretically if we could de-couple U0 from None we could set MaxDeposits to U0 and reject non-empty Deposits at decode time rather than during verification.

Also, as a side note, for VariableList, U0 actually does mean "limit of 0" so this is inconsistent API-wise.