More RLP limits, sizes, and positions checks

[alexb5dh]

RLP fixes related to collection sizes, limits, and post-decode positions checks.

Changes

• Reject legacy txs in typed (EIP-2718) envelope encoding
• Limit and size checks support in Rlp.DecodeByteArrays
• RLP size/length validation after decoding tx access list, blob txs, and withdrawal fields
• Post-decode position checks in inner decode loops
• Reject txs with trailing bytes in execution payload
• Tests for the fixes: tx, access list, blob, withdrawal, and execution payload decoders

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

[claude]

Claude finished @alexb5dh's task in 8m 18s —— View job

---

PR Review: More RLP limits and sizes checks

• Gather context and read relevant rules
• Read core changed files (RLP serialization, decoders)
• Read test files
• Read network serializer changes
• Analyze findings and post review

---

Summary

This PR adds important input validation across the RLP decoding stack — sequence length checks on withdrawals, typed-tx envelope guards, blob count limits, trailing-bytes rejection in the execution payload, and cross-boundary checks in P2P message parsers. The core security fixes are all correct and well-motivated. There are no critical or high-severity issues. Two medium-severity issues were found in test code, plus three low-severity observations.

---

Correctness — Main Code

All the substantive production-code changes look correct:

• AccessListDecoder — making storage keys mandatory (was optional) aligns with EIP-2930: every access-list entry is [address, [storageKeys]]. The guard if (decoderContext.Position >= accessListItemCheck) throw correctly fires regardless of AllowExtraBytes because that flag controls trailing bytes, not missing required fields.
• WithdrawalDecoder — adding Check(checkPosition) prevents the cross-boundary RLP attack described in the test docstring. Correct.
• TxDecoder.ThrowIfLegacy — legacy type byte (0) is not allowed inside an EIP-2718 typed-transaction envelope. Correct.
• BlobTxDecoder version > ProofVersion.V1 — the old version != ProofVersion.V1 would reject explicit V0 wrappers; the new > comparison correctly accepts both V0 and V1.
• BlobVersionedHashesCountLimit / blob counts — 128-blob cap is a reasonable DoS guard well above current protocol max (6/9 blobs); the DecodeByteArrays(..., innerSize: Hash256.Size) check rejects non–32-byte versioned hashes.
• ExecutionPayload / TraceRpcModule → DecodeCompleteNotNull — rejects transactions with trailing garbage, matching other client behaviour.
• Network serializers — Check(checkPosition) added after each sub-sequence in HelloMessage, NewBlockHashes, AccountRange and StorageRanges prevents cross-boundary parsing.

---

Findings

Medium

1. WithdrawalDecoderTests.CombineRlpList — buffer is 1 byte too small for sequences ≥ 56 bytes

Rlp.LengthOfLength returns only the byte-count for the length value itself, not including the 1-byte prefix byte. For content of 56–255 bytes the RLP header is 2 bytes (prefix + 1 length byte), but the current code allocates only 1 + sequenceLength, causing StartSequence to write into a 1-byte-out-of-bounds slot. The specific test never exceeds 51 bytes of content, so it passes today — but the helper is wrong in general. (Inline comment posted.)

2. TxDecoderTests — static field TxDecoder shadows the type name

private static readonly TxDecoder TxDecoder = TxDecoder.Instance; names a field identically to its type, shadowing TxDecoder as a type name within the class scope and violating the _camelCase private-field convention. (Inline comment posted.)

Low

3. Unused MathNet.Numerics.Random import in ShardBlobTxDecoderTests — all new tests use Random.Shared. (Inline comment posted.)

4. V0 and V1 proof-count limits produce identical error messages — both NetworkWrapperProofsCountLimit and NetworkWrapperCellProofsCountLimit use nameof(ShardBlobNetworkWrapper.Proofs), making limit-breach log messages indistinguishable. (Inline comment posted.)

5. RlpValueDecoder<T>.DecodeComplete duplicates the new extension method — the instance method added to RlpValueDecoder<T> is behaviorally identical to the new RlpValueDecoderExtensions.DecodeComplete extension. Minor dead-code risk if they ever diverge.

---

Test Coverage

New tests are well-structured and cover the right scenarios: cross-boundary withdrawal attack, all five blob-count per-array limits, versioned hash size enforcement, and execution payload trailing-bytes rejection.

[alexb5dh]

@claude re-review

[claude]

Claude finished @alexb5dh's task in 3m 59s —— View job

---

PR Re-Review: More RLP limits and sizes checks

• Load relevant rules files
• Check current state of previously-flagged issues
• Re-examine all changed files
• Analyze findings and post review

---

Status of Previous Findings

All five items from the first review have been addressed:

#	Issue	Status
1	Medium: CombineRlpList buffer too small for sequences ≥ 56 bytes	✅ Fixed — now uses Rlp.LengthOfSequence(sequenceLength)
2	Medium: TxDecoder field shadows type name	✅ Fixed — renamed to _txDecoder
3	Low: Unused MathNet.Numerics.Random import	✅ False positive confirmed by PR author — Random.Shared.NextBytes(int count) returning byte[] is a MathNet extension method; System.Random does not have this overload
4	Low: V0/V1 proof-count limits produce identical error messages	✅ Fixed — limits now use $"{nameof(ShardBlobNetworkWrapper.Proofs)} {ProofVersion.V0}" / ...V1
5	Low: RlpValueDecoder<T>.DecodeComplete duplicates extension method	Still present (minor, no correctness impact)

---

Fresh Review of All Changes

Production code correctness:

• Rlp.Decode<T> — The refactor from Check(length) to rlpDecoder.DecodeComplete(...) (which calls CheckEnd()) is semantically identical. Correct.
• Rlp.DecodeByteArrays(limit, innerSize) — PeekNumberOfItemsRemaining is bounded by checkPosition (which is derived from the validated sequence length), so there is no way for a malicious payload to induce an unbounded scan before the limit guard fires. The order is safe.
• AccessListDecoder storage key size check — storageItemCheck = Position + IndexLength + 1 (= 33) correctly enforces exactly one 32-byte storage key per item. Check(storageItemCheck) rejects both shorter (truncated) and longer (over-length) encodings. The mandatory-field guard if (Position >= accessListItemCheck) throw fires regardless of AllowExtraBytes, which is the correct behaviour for a required field.
• BlobTxDecoder V0/V1 proof selection — version > ProofVersion.V1 correctly rejects unknown future versions while accepting both V0 and V1. The RlpLimit selection via version is ProofVersion.V1 ? ... : ... correctly applies the cell-proof limit (128 × 128 = 16 384) for V1 and the blob-proof limit (128) for V0.
• Xdc decoders — replacing DecodeByteArrays() + LINQ post-check with DecodeByteArrays(innerSize: Signature.Size) is a clean consolidation. Note that byte[][]? signatureBytes = ... uses a nullable annotation on a non-nullable return type; the if (signatureBytes is not null) guard is dead code, but this pre-existed the PR. No regression.
• ExecutionPayload / TraceRpcModule — DecodeCompleteNotNull correctly rejects transactions with trailing bytes. The test in ExecutionPayloadTests covers the failure path with an explicit garbage suffix and verifies the "checkpoint failed" message.
• Network serializers — Check(checkPosition) after each inner sub-sequence in HelloMessage, NewBlockHashes, AccountRange, and StorageRanges prevents cross-boundary parsing attacks without any impact on encode paths.

No new Critical, High, or Medium findings.

---

Remaining Low Observation

RlpValueDecoder<T> now has an instance DecodeComplete method (added at IRlpDecoder.cs:134) that is functionally identical to the RlpValueDecoderExtensions.DecodeComplete extension. Both call Decode then CheckEnd. When the variable is typed as IRlpValueDecoder<T> the extension is used; when typed as the concrete class the instance method shadows it. They are currently equivalent so there is no divergence risk, but having two implementations of the same contract is a minor maintenance surface. This is low-severity and does not block merge.

---

Summary

All previously-flagged medium issues are resolved. The security fixes (cross-boundary withdrawal, typed-envelope legacy rejection, blob count caps, versioned hash size enforcement, execution payload trailing bytes) are correct and align with how other clients behave. The PR is ready to merge.

[claude]

Claude finished @alexb5dh's task in 2m 55s —— View job

---

PR Re-Review: More RLP limits and sizes checks

• Load relevant rules files
• Check current state of previously-flagged issues
• Re-examine all changed files for new issues
• Analyze findings and post review

---

Summary

All previously-flagged Medium issues have been resolved, and the V0/V1 proof-limit disambiguation Low issue has also been fixed. The MathNet.Numerics.Random import was correctly defended by the author — Random.Shared.NextBytes(int count) returning byte[] is not part of System.Random and the MathNet namespace is genuinely required. No new issues were found in this pass.

---

Status of Previous Findings

#	Severity	Finding	Status
1	Medium	CombineRlpList buffer underallocated for sequences ≥ 56 bytes	✅ Fixed — now uses Rlp.LengthOfSequence(sequenceLength)
2	Medium	TxDecoder field shadows type name, violates _camelCase convention	✅ Fixed — renamed to _txDecoder
3	Low	MathNet.Numerics.Random import in ShardBlobTxDecoderTests flagged as unused	✅ Not an issue — NextBytes(int) returning byte[] is a MathNet extension on System.Random; the import is required
4	Low	V0 and V1 proof-count limits share the same property name in error messages	✅ Fixed — both limits now include ProofVersion.V0 / ProofVersion.V1 in the limit label
5	Low	RlpValueDecoder<T>.DecodeComplete instance method duplicates the extension method	⚠️ Remains — the two serve different abstraction levels (concrete class vs interface) and are equivalent, so divergence risk is minimal

---

No New Issues Found

All production-code changes were re-verified:

• WithdrawalDecoder — Check(checkPosition) correctly enforces cross-boundary bounds
• AccessListDecoder — storage-keys mandatory check and per-item storageItemCheck (33 = 1 prefix + 32 data bytes) are correct for EIP-2930 bytes32 storage keys
• BlobTxDecoder — proof limit labels are now distinguishable; version guard uses > ProofVersion.V1 correctly
• TxDecoder.ThrowIfLegacy — correctly rejects type-0 tx in EIP-2718 envelope
• Network serializers — Check(checkPosition) guards on HelloMessage capabilities and NewBlockHashes items are correct
• ExecutionPayloadTests — test correctly validates that trailing garbage bytes on a typed tx cause rejection

This PR is ready to merge.