Minor fixes from AI audit #2 by alexb5dh · Pull Request #11563 · NethermindEth/nethermind

alexb5dh · 2026-05-12T00:14:24Z

Fixes for minor issues across different parts of codebase spotted by AI checks, continuation of #11406.

Changes

Fixes

Fixes block GasUsed not being restored when Taiko TryAddTx fails.
Caps items counted in RLP PeekNumberOfItemsRemaining - exits loop early when limit is reached.
Applyies BlobVersionedHashes spec limit in LightTxDecoder also.
Fixes missing supportTxHash check in ReceiptStorageDecoder.GetContentLength. Also fixes invalid test, passing due to missing check.
Fix BloomStructRef.Equals(object) type check.
Fix potential out-of-order call issue in AuRaSealValidator.ValidateParams.

Error message improvements

Adds content length vs buffer size validation in PeekLongPrefixAndContentLength - prevents vague ArgumentOutOfRangeException in callers.
Improves error message on missing JSON parameter constructor.
Improves error message for invalid discovery IP address.

Other

Some clarifying comments in existing code.

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

…xAndContentLength` Prevents potential `ArgumentOutOfRangeException` in callers

…check

…tContentLength`, fix related test

…der calls

claude · 2026-05-12T00:14:42Z

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

PR Review — Minor Fixes from AI Audit #2

Summary

The PR is a clean collection of targeted bug fixes. No critical, high, or medium issues found. All fixes are correct and well-tested.

Findings

Critical / High / Medium

None.

Low

ReceiptDecoderTests.cs:64-73 — dead valueDecoder parameter (inline comment posted)
Both branches of if (valueDecoder) are identical; the parameter doubles the test matrix without exercising different code. Pre-existing issue, not introduced here.
BloomTests.cs — only positive case tested
Equals_object_returns_true_for_equal_bloom covers the fixed happy path but not null, a different type, or non-equal blooms via Equals(object?). Minor; the existing Equals(Bloom?) tests already provide some coverage.

Per-change notes

Change	Assessment
`AuRaSealValidator`: `Add` → `Insert(~index, …)`	✅ Correct. `Add` broke the sorted invariant, causing `ClearOldCache`'s `BinarySearch` to evict entries at the wrong position, allowing sibling-block equivocation to go undetected. Test traces through the exact failure scenario.
`BloomStructRef.Equals(object?)`	✅ Correct. Original implementation was effectively always-false: it checked `GetType() == typeof(BloomStructRef)` but ref structs can never be boxed, so the check always returned `false`. New `obj is Bloom bloom && Equals(bloom)` is correct.
`ReceiptStorageDecoder`: propagate `supportTxHash` to `GetContentLength`	✅ Correct. Missing `if (supportTxHash)` guard meant `GetContentLength` always included the TxHash bytes even when encoding without it, producing a mismatched length prefix.
`RlpHelpers`: bounds check for long-form RLP	✅ Correct. The new `contentLength > data.Length - position - (1 + lengthOfLength)` guard prevents out-of-bounds reads on truncated data. Prior check only covered the length-of-length field, not the declared content length itself. Tests exercise all four long-form prefix variants (string/list × 1-byte / 4-byte length).
`Rlp.DecodeByteArrays`: cap `PeekNumberOfItemsRemaining`	✅ Correct and a performance win. Limits the O(n) item-count scan to `limit+1` items so adversarial data can't force an unbounded scan before the limit guard fires.
`LightTxDecoder`: add `BlobVersionedHashesCountLimit`	✅ Correct security fix. Without the limit, a crafted blob tx in the pool with an excessive number of versioned-hash entries could cause an OOM allocation in `DecodeByteArrays`.
`RpcModuleProvider`: null check with clear error	✅ Converts a silent `NullReferenceException` at invocation time to an explicit `InvalidOperationException` at module setup time with a clear message.
`DiscoveryMsgSerializerBase`: IP length validation	✅ Validates byte length is 4 (IPv4) or 16 (IPv6) before constructing `IPAddress`. Converts an opaque `ArgumentException` from the BCL into a typed `NetworkingException.Validation`, consistent with port validation on the preceding lines.
`TaikoEngineRpcModule`: restore `blockHeader.GasUsed`	✅ Correct. Before the fix, all four rejection paths (`!executionResult`, surge filter, exception, `TryAddTx` failure) restored the world state but left `blockHeader.GasUsed` inflated by the rejected transaction's gas. The `Restore(snapshot, gasUsedBefore)` local function fixes all four sites consistently. Test covers both byte-limit and surge-filter rejection scenarios.
`PayloadAttributes`: `<remarks>` on caching	✅ Good documentation of a non-obvious footgun (calling with different `parentHeader` on the same instance silently returns the stale cached result).
`HeaderDecoder.cs`: EIP-4844 pairing comment	✅ Clarifies why `requiredItems[3]` repeats the `BlobGasUsed` check — the two fields are always encoded as a pair per EIP-4844.

claude · 2026-05-12T00:52:15Z

Claude finished @alexb5dh's task in 5m 57s —— View job

PR Review — Minor Fixes from AI Audit #2

Summary

Clean, targeted collection of bug fixes. All fixes are correct and well-tested. No critical, high, or medium issues found.

Findings

Critical / High / Medium

None.

Low

BloomTests.cs:81 — only the positive path is tested for BloomStructRef.Equals(object?) (inline comment posted)

The new implementation is correct, but the test only asserts Equals((object)bloom) == true. The complementary cases (null, non-equal Bloom, wrong type) are not covered. The pre-existing bug (ref structs can't be boxed, so the old type check was always false) silently masked this gap. Worth adding a few [TestCase]-style assertions.

Per-change assessment

Change	Assessment
`AuRaSealValidator`: `Add` → `Insert(~index, …)`	✅ Correct. `Add` broke the sorted-list invariant; `ClearOldCache`'s `BinarySearch` then evicted entries at the wrong position, allowing a future sibling block at an already-seen step to go undetected. New test (`validate_params_out_of_order`) traces the exact failure scenario with out-of-order arrivals.
`BloomStructRef.Equals(object?)`	✅ Correct. Old code checked `GetType() == typeof(BloomStructRef)` but ref structs can never be boxed, so the comparison was always `false`. New `obj is Bloom bloom && Equals(bloom)` is correct; null is handled implicitly by the pattern. `GetHashCode()` is consistent (both use `FastHash` on the same bytes).
`ReceiptStorageDecoder`: propagate `supportTxHash` to `GetContentLength`	✅ Correct. `GetContentLength` was `static` and unconditionally added `1 + LengthOf(TxHash)` even when `supportTxHash=false`, producing a length prefix that didn't match the actual encoded bytes. Test now also simplified by removing the dead `valueDecoder` branch and switching to `DecodeComplete`.
`RlpHelpers`: content-length bounds check	✅ Correct. The new `contentLength > data.Length - position - (1 + lengthOfLength)` guard catches truncated RLP where the declared payload extends past the buffer. The subtraction is safe because the preceding `position + 1 + lengthOfLength > data.Length` check already ensures the RHS is non-negative.
`Rlp.DecodeByteArrays`: cap `PeekNumberOfItemsRemaining`	✅ Correct performance/security fix. Scanning is now bounded to `limit+1` items so adversarially large item counts don't force an O(n) scan before the limit check fires. The `+1` is intentional: it lets `GuardLimit` observe that the count exceeds the limit without scanning further.
`LightTxDecoder`: apply `BlobVersionedHashesCountLimit`	✅ Correct security fix. Without the limit, a crafted blob tx persisted in the light pool format with a huge versioned-hash list could trigger an unbounded allocation in `DecodeByteArrays` on next node startup. `BlobVersionedHashesCountLimit` visibility was correctly widened from `private` to `public` to enable the reuse.
`RpcModuleProvider`: null check on parameterless constructor	✅ Converts a silent `NullReferenceException` (at first invocation) into a clear `InvalidOperationException` at module registration time with an actionable message.
`DiscoveryMsgSerializerBase`: IP length validation	✅ Validates byte length is 4 (IPv4) or 16 (IPv6) before passing to `IPAddress` constructor. Converts an opaque `ArgumentException` from the BCL into a typed `NetworkingException(Validation)`, consistent with the port check above it. Test updated to expect `NetworkingException` instead of `ArgumentException`.
`TaikoEngineRpcModule`: restore `blockHeader.GasUsed`	✅ Correct. All four rejection paths (execution failure, surge filter, exception, `TryAddTx` overflow) previously restored `worldState` but left `blockHeader.GasUsed` inflated by the rejected tx's gas. The `Restore(snapshot, gasUsedBefore)` local function fixes all four sites. `EstimatedGasUsed` in committed batches was therefore also wrong. New test covers both rejection causes.
`PayloadAttributes`: `<remarks>` on `GetPayloadId`	✅ Good documentation of a non-obvious footgun: calling with a different `parentHeader` on the same instance silently returns the stale cached result. `<remarks>` is the right tag per coding guidelines.
`HeaderDecoder`: EIP-4844 pairing comment	✅ Clarifies why `requiredItems[3]` repeats the `BlobGasUsed` condition — EIP-4844 requires the two gas fields to always be encoded together, so their required-item bits must be consistent.

claude · 2026-05-12T00:57:45Z

+            BloomStructRef bloomStructRef = new(bloom.Bytes);
+
+            Assert.That(bloomStructRef.Equals((object)bloom), Is.True);
+        }


Low — test only covers the positive path for BloomStructRef.Equals(object?)

The fix to Equals(object?) has three observable behaviours, but only one is exercised here:

obj Expected

equal Bloom true ✅ tested

null false ❌ not tested

non-equal Bloom false ❌ not tested

an unrelated type (e.g. string) false ❌ not tested

The old bug (GetType() == typeof(BloomStructRef) always false since ref structs can't be boxed) would have silently passed all the untested cases anyway, so it didn't matter before. Now that the implementation is correct, consider adding complementary cases to pin the contract:

// null → false Assert.That(bloomStructRef.Equals((object?)null), Is.False); // non-equal Bloom → false Bloom different = new(); different.Set(Keccak.Zero.Bytes); Assert.That(new BloomStructRef(different.Bytes).Equals((object)bloom), Is.False); // wrong type → false Assert.That(bloomStructRef.Equals("string"), Is.False);

alexb5dh added 12 commits May 11, 2026 18:22

[C-08] Taiko: restore gas on not-accepted tx

1329372

[C-18] exit RLP PeekNumberOfItemsRemaining loop when limit reached

8fe369c

Use BlobVersionedHashes spec limit in LightTxDecoder also

299756a

[C-26] validate content lengths against buffer size in `PeekLongPrefi…

f20a055

…xAndContentLength` Prevents potential `ArgumentOutOfRangeException` in callers

[C-30] Comment on PayloadAttributes.GetPayloadId usage

89efeb4

[U-12] Improve error on missing JSON parameter constructor

09338b9

[U-20] comment explaining asymmetry in BlobGasUsed/ExcessBlobGas …

22e9d1e

…check

[U-24] slightly better error on invalid IP during discovery

7d7496d

[U-26] Fix missing supportTxHash check in `ReceiptStorageDecoder.Ge…

fc9386d

…tContentLength`, fix related test

[U-33] BloomStructRef.Equals(object) fix

97c2e69

[U-32] potential fix for AuRaSealValidator.ValidateParams out-of-or…

d32e4b9

…der calls

Merge remote-tracking branch 'origin/master' into fix/ai-audit-2

a1c77cd

github-actions Bot added bug fix + reliability network rpc taiko related to the taiko alethia rollup labels May 12, 2026

Formatting

bc0e4cb

claude Bot reviewed May 12, 2026

View reviewed changes

Comment thread src/Nethermind/Nethermind.Core.Test/Encoding/ReceiptDecoderTests.cs Outdated

alexb5dh added 2 commits May 12, 2026 03:30

Test fix

f19aab5

PR feedback

32894d7

alexb5dh marked this pull request as ready for review May 12, 2026 00:51

alexb5dh requested review from LukaszRozmej, Marchhill, asdacap, benaadams, dipkakwani, flcl42, marcindsobczak and smartprogrammer93 as code owners May 12, 2026 00:51

alexb5dh requested review from MarekM25 and svlachakis as code owners May 12, 2026 00:51

claude Bot reviewed May 12, 2026

View reviewed changes

asdacap approved these changes May 12, 2026

View reviewed changes

LukaszRozmej approved these changes May 12, 2026

View reviewed changes

alexb5dh merged commit 059606b into master May 12, 2026
560 checks passed

alexb5dh deleted the fix/ai-audit-2 branch May 12, 2026 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor fixes from AI audit #2#11563

Minor fixes from AI audit #2#11563
alexb5dh merged 15 commits into
masterfrom
fix/ai-audit-2

alexb5dh commented May 12, 2026 •

edited

Loading

Uh oh!

claude Bot commented May 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

claude Bot commented May 12, 2026 •

edited

Loading

Uh oh!

claude Bot May 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`obj`	Expected
equal `Bloom`	`true` ✅ tested
`null`	`false` ❌ not tested
non-equal `Bloom`	`false` ❌ not tested
an unrelated type (e.g. `string`)	`false` ❌ not tested

Conversation

alexb5dh commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Fixes

Error message improvements

Other

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Uh oh!

claude Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — Minor Fixes from AI Audit #2

Summary

Findings

Critical / High / Medium

Low

Per-change notes

Uh oh!

Uh oh!

claude Bot commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — Minor Fixes from AI Audit #2

Summary

Findings

Critical / High / Medium

Low

Per-change assessment

Uh oh!

claude Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexb5dh commented May 12, 2026 •

edited

Loading

claude Bot commented May 12, 2026 •

edited

Loading

claude Bot commented May 12, 2026 •

edited

Loading