RPC: add eth_getRawTransactionByBlockHashAndIndex & eth_getRawTransactionByBlockNumberAndIndex & eth_sendRawTransactionSync

[svlachakis]

Fixes Closes Resolves #11518 & #11519 & #11520

Changes

• eth_getRawTransactionByBlockHashAndIndex implementation
• eth_getRawTransactionByBlockNumberAndIndex implementation
• eth_sendRawTransactionSync implementation

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

[claude]

Claude finished @svlachakis's task in 4m 40s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

The two eth_getRawTransactionBy* methods are clean and correct. The eth_sendRawTransactionSync implementation has a critical production bug that will cause it to always time out, plus one high-severity design issue in the constructor. Details below.

---

Critical

eth_sendRawTransactionSync always times out in production

EthModuleFactory passes blockTree.AsReadOnly() — a ReadOnlyBlockTree — as the IBlockFinder argument:

// EthModuleFactory.cs
private readonly IReadOnlyBlockTree _blockTree = blockTree.AsReadOnly();
// ...
new EthRpcModule(config, bridge, _blockTree, ...)

ReadOnlyBlockTree.NewHeadBlock is a no-op event:

public event EventHandler<BlockEventArgs> NewHeadBlock
{
    add { }    // ← silently swallowed
    remove { }
}

So the subscription at _blockTree.NewHeadBlock += OnNewHead (line 389) never receives any events. The semaphore signal is never released, WaitAsync blocks until the CTS fires, and every call returns a timeout error regardless of whether the tx was included. The tests pass only because TestRpcBlockchain uses a real BlockTree that fires events.

Fix: accept the underlying writable IBlockTree for event subscription — either as an additional constructor parameter or by replacing the IBlockFinder parameter with IBlockTree (valid since IBlockTree : IBlockFinder). The factory already holds the writable IBlockTree before calling .AsReadOnly().

Fix this →

---

High

Constructor secretly requires IBlockFinder to be IBlockTree

private readonly IBlockTree _blockTree = blockFinder as IBlockTree
    ?? throw new ArgumentException($"Expected {nameof(IBlockTree)}, ...", nameof(blockFinder));

The public contract says IBlockFinder; the body throws for anything else. This violates LSP for derived classes and test doubles. It breaks OptimismEthRpcModule (and others) if they ever pass a decorated IBlockFinder. The right approach is to change the parameter type to IBlockTree (which also fixes the Critical above), since IBlockTree already extends IBlockFinder and all real callers already pass a BlockTree.

---

Medium

positionIndex < 0 is always false for UInt256

UInt256 is unsigned — the left half of the guard on line 593 is dead code. The pre-existing pattern on lines 612 and 646 has the same issue. Also, when block.Transactions.Length == 0, the expression block.Transactions.Length - 1 evaluates to -1 as int; converting that to UInt256 wraps to UInt256.MaxValue, so the upper-bound check silently passes and the next line throws IndexOutOfRangeException instead of returning null. Use (uint)block.Transactions.Length for the comparison:

if (positionIndex >= (uint)block.Transactions.Length)
    return ResultWrapper<string?>.Success(null);

ResolveSyncTimeoutMs unsafe casts

If RpcTxSyncMaxTimeoutMs is misconfigured to a negative value, (ulong)max wraps to a huge number and the Math.Min doesn't clamp anything; the subsequent (int) cast on a large ulong overflows to a negative delay, causing CancellationTokenSource to throw or fire immediately. See inline comment for a hardened version.

---

Low

Missing ExampleValue on the three new [JsonRpcMethod] declarations

The surrounding methods all carry [JsonRpcParameter(ExampleValue = "...")] on each parameter. The three new methods (eth_sendRawTransactionSync, eth_getRawTransactionByBlockHashAndIndex, eth_getRawTransactionByBlockNumberAndIndex) have bare parameters, which means the auto-generated docs and RPC playground will lack examples. See inline comment on IEthRpcModule.cs.

Missing trace logging

eth_getTransactionByBlockHashAndIndex and eth_getTransactionByBlockNumberAndIndex both emit a _logger.IsTrace log line on success. The two new raw-tx variants don't, making it harder to correlate RPC traffic in trace logs.

---

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 4m 18s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All Critical, High, and Medium findings from the previous review are now fixed. One new Medium issue was found, plus one low-severity note.

---

Fixed from previous review ✅

Severity	Finding	Status
Critical	NewHeadBlock on ReadOnlyBlockTree is a no-op → always times out	Fixed — separate IBlockTree blockTree parameter added; factory passes writable blockTree
High	Constructor IBlockFinder cast-and-throw violated LSP	Fixed — clean separate parameter, no more cast
Medium	positionIndex < 0 dead code for UInt256	Fixed — new code uses positionIndex >= (uint)block.Transactions.Length
Medium	ResolveSyncTimeoutMs unsafe cast wrapping	Fixed — Math.Max(1, _rpcConfig.RpcTxSyncMaxTimeoutMs) guards the cast
Low	Missing ExampleValue on new methods	Addressed — follows existing convention (all examples packed into the first parameter's attribute as a JSON array)
Low	Missing trace logging on new raw-tx methods	Fixed — trace log lines added

---

New findings

Medium — ObjectDisposedException can propagate to the block-processing thread

OnNewHead is a using-scoped closure over signal. C# event dispatch captures a snapshot of the invocation list before iterating it. If BlockTree has already captured OnNewHead in its snapshot when finally runs _blockTree.NewHeadBlock -= OnNewHead, the unsubscription does not stop that in-flight call. By the time the handler reaches signal.Release(), the using may have disposed signal, throwing ObjectDisposedException. There is no try/catch around the invocation at BlockTree.cs:1325, so this exception propagates into the block processing chain.

The lock (signal) double-check prevents SemaphoreFullException (the at-max-capacity case) but not ObjectDisposedException.

Fix: replace the lock+double-check with:

void OnNewHead(object? sender, BlockEventArgs _)
{
    try { signal.Release(); }
    catch (SemaphoreFullException) { }
    catch (ObjectDisposedException) { }
}

See inline comment on EthRpcModule.cs:384. Fix this →

Low — pre-existing positionIndex < 0 dead code in GetTransactionByBlockAndIndex

EthRpcModule.cs:626 still has if (positionIndex < 0 || positionIndex > block.Transactions.Length - 1) using UInt256. Since UInt256 is unsigned, < 0 is always false, and Length - 1 wraps to UInt256.MaxValue for an empty block. This code predates the PR but the author correctly fixed it in the new GetRawTransactionByBlockAndIndex. Worth fixing the pre-existing method in a follow-up or here.

---

Verdict: not yet mergeable — one new Medium finding (threading/robustness).

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 4m 29s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All Critical, High, and Medium findings from both prior reviews are fixed. No new Critical, High, or Medium issues found. One pre-existing Low is noted.

---

Fixed from previous reviews ✅

Severity	Finding	Status
Critical	NewHeadBlock on ReadOnlyBlockTree is a no-op → always times out	Fixed — separate IBlockTree blockTree parameter; factory passes writable blockTree
High	Constructor IBlockFinder cast-and-throw violated LSP	Fixed — clean separate parameter, no cast
Medium	positionIndex < 0 dead code for UInt256	Fixed — new code uses positionIndex >= (uint)block.Transactions.Length
Medium	ResolveSyncTimeoutMs unsafe cast wrapping	Fixed — Math.Max(1, ...) guards the cast
Medium	ObjectDisposedException could propagate to block-processing thread	Fixed — OnNewHead now catches both SemaphoreFullException and ObjectDisposedException
Low	Missing ExampleValue on new methods	Fixed
Low	Missing trace logging on new raw-tx methods	Fixed

---

Pre-existing (not introduced by this PR)

Low — positionIndex < 0 dead code in GetTransactionByBlockAndIndex and uncle lookup

EthRpcModule.cs:625 (GetTransactionByBlockAndIndex) and :659 (uncle lookup) still carry the old positionIndex < 0 || positionIndex > block.Transactions.Length - 1 guard. The new GetRawTransactionByBlockAndIndex correctly uses positionIndex >= (uint)block.Transactions.Length. Worth fixing the pre-existing methods in a separate follow-up.

---

Verdict: mergeable — all Critical, High, and Medium findings from prior reviews are resolved.

[claude]

Claude finished @svlachakis's task in 4m 52s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All findings from the three prior review rounds are confirmed fixed. No new Critical, High, or Medium issues. The implementation is correct and ready to merge.

---

All prior findings — fixed ✅

Severity	Finding
Critical	NewHeadBlock is a no-op on ReadOnlyBlockTree → always timed out in production
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256 (new and pre-existing methods)
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast when config is negative
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block-processing event
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods

---

Verification notes

IBlockTree wiring — EthModuleFactory correctly passes the read-only wrapper as IBlockFinder and the writable blockTree as IBlockTree (line 47 vs 48). All downstream constructors — OptimismEthRpcModule, benchmark, test infrastructure — are updated consistently.

OnNewHead handler — catches both SemaphoreFullException (at-max-capacity coalescing) and ObjectDisposedException (in-flight-after-dispose race). The comment accurately explains the invariant.

ResolveSyncTimeoutMs — Math.Max(1, ...) ensures (ulong)max never wraps; result ≤ int.MaxValue since max is already int, so the (int) cast is safe.

GetRawTransactionByBlockAndIndex index bounds — positionIndex >= (uint)block.Transactions.Length correctly handles empty blocks (no Length - 1 wrapping) and makes the subsequent (int)positionIndex cast safe since positionIndex < array.Length ≤ int.MaxValue.

NettyRlpStream lifetime — using scope ends after ToHexString copies bytes into a managed string; no use-after-dispose.

No other EthRpcModule subclasses — grep confirms only OptimismEthRpcModule inherits from EthRpcModule; Taiko uses composition, not inheritance.

---

Verdict: mergeable — no unresolved Critical, High, or Medium findings.

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 4m 59s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All findings from the previous four review rounds remain fixed. One new Medium issue found.

---

All prior findings — confirmed fixed ✅

Severity	Finding
Critical	NewHeadBlock no-op on ReadOnlyBlockTree → always timed out in production
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast on negative config
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block event
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods

---

New finding

Medium — eth_sendRawTransactionSync is not virtual, so Optimism validator nodes always time out

OptimismEthRpcModule overrides eth_sendRawTransaction to forward to a configured sequencer when SequencerUrl is set. eth_sendRawTransactionSync is not virtual, so no override is possible. The method calls the private SendTx() directly, bypassing the sequencer path.

Impact: on an Optimism validator node (sequencer URL configured), the tx is submitted to the local inactive pool instead of the sequencer. Since validators never produce blocks, the receipt is never found and every call times out.

Fix: mark the method virtual in EthRpcModule, then add an OptimismEthRpcModule override that delegates to base when _sequencerRpcClient is null, or forwards to the sequencer and polls for receipt otherwise.

See inline comment on EthRpcModule.cs:361. Fix this →

---

Verdict: not yet mergeable — one unresolved Medium finding.

[LukaszRozmej]

Why do we need separate timeouts?

[svlachakis]

They seem to serve different purposes. Default is the no-arg UX (~1–2 block times so tx-not-included callers get fast feedback), Max is a server-side cap against clients passing huge timeouts and pinning workers, same split Geth uses for the same reason I believe (RPCTxSyncDefaultTimeout/RPCTxSyncMaxTimeout).

[LukaszRozmej]

N semaphores and N event subscriptions per call. Maybe they should be consolidated to 1?

[svlachakis]

I think a single SemaphoreSlim won't work (Release wakes only one waiter), so the real alternative is a shared TCS that gets swapped on each block. Looks like it saves a few microseconds per block, but adds a race: if a block lands between the receipt check and the await, the shared TCS has already swapped to the next block, and the call sits idle for ~12s before checking again. N per-call semaphores avoid that because each release queues, even with bad timing the next await returns right away.

In the case where one semaphore receives two releases, if these two Release() calls happen before any WaitAsync():

• 1st: count 0 → 1
• 2nd: count is already at max → throws SemaphoreFullException (we catch it)
• That second release is lost / coalesced, not queued

Microseconds vs occasionally missing a block I think it's better to keep N.

[LukaszRozmej]

TCS is fine, IMO race is avoidable

[svlachakis]

@claude review again, I switched to use eth_sendRawTransaction to catch the override cases also without too much hustle

[claude]

Claude finished @svlachakis's task in 2m 31s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases (focus on the eth_sendRawTransaction refactor)
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All prior Medium+ findings remain fixed. The refactor from private SendTx() to virtual eth_sendRawTransaction() is the right approach — it correctly inherits the Optimism sequencer-forward path without needing a separate override. One pre-existing Low noted.

---

Prior findings — confirmed fixed ✅

Severity	Finding
Critical	NewHeadBlock no-op on ReadOnlyBlockTree → always timed out
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast on negative config
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block event
Medium	eth_sendRawTransactionSync non-virtual, Optimism validator nodes always timed out
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods

---

New finding

Low — Optimism eth_sendRawTransactionSync returns base ReceiptForRpc, omitting L1 fee fields

TryGetReceipt (private in EthRpcModule) always constructs a plain ReceiptForRpc, while OptimismEthRpcModule.eth_getTransactionReceipt returns an OptimismReceiptForRpc with l1Fee, l1GasUsed, l1GasPrice, depositReceiptVersion, etc.

On an Optimism node, a caller that uses eth_sendRawTransactionSync to submit-and-wait gets a receipt without L1 fee info, while a subsequent eth_getTransactionReceipt call returns the full Optimism receipt. Since this is a new non-standard convenience method, callers needing full Optimism receipt fields can fall back to eth_getTransactionReceipt. Worth noting in a follow-up.

---

Pre-existing (not introduced by this PR)

OptimismEthRpcModule.GetTransactionByBlockAndIndex at line 210 still uses the old guard positionIndex < 0 || positionIndex > block!.Transactions.Length - 1 (both halves are either dead code or wrap on empty blocks, since positionIndex is UInt256). The base EthRpcModule method was fixed in this PR to use positionIndex >= block.Transactions.Length; the Optimism override was missed. Worth fixing in a follow-up.

---

Verdict: mergeable — no unresolved Critical, High, or Medium findings.

[svlachakis]

@claude review again, switched to pass through eth_getTransactionReceipt, again for overrides

[claude]

Claude finished @svlachakis's task in 2m 57s —— View job

---

Claude finished @svlachakis's task —

---

PR Review

• Load repo coding rules
• Read changed files (focus on eth_getTransactionReceipt pass-through)
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

All findings from all prior review rounds remain fixed. The latest change (1616975) is the correct final touch: switching from the now-deleted private TryGetReceipt to calling the virtual eth_getTransactionReceipt in the poll loop. No new Critical, High, or Medium issues.

---

All prior findings — confirmed fixed ✅

Severity	Finding
Critical	NewHeadBlock is a no-op on ReadOnlyBlockTree → always timed out in production
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256 (new and pre-existing methods)
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast on negative config
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block event
Medium	eth_sendRawTransactionSync non-virtual, Optimism validator nodes always timed out
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods
Low	Optimism eth_sendRawTransactionSync returned plain ReceiptForRpc, omitting L1 fee fields

---

Verification of the latest change

The switch from the deleted TryGetReceipt helper to eth_getTransactionReceipt(hash) is correct in every dimension:

• Correctness: eth_getTransactionReceipt is virtual (line 948) and overridden by OptimismEthRpcModule (line 144) to return OptimismReceiptForRpc with L1 fee fields — callers now get the full, chain-appropriate receipt.
• Performance: The extra Optimism work (block lookup, L1BlockGasInfo) only executes when a receipt is actually found, i.e., exactly once per call at the moment of inclusion. The null-path returns immediately.
• Trace log: eth_getTransactionReceipt's trace log is inside the found-receipt branch, so it fires once per eth_sendRawTransactionSync call — not on every poll iteration.
• Simplicity: Removing TryGetReceipt eliminates a duplicate of eth_getTransactionReceipt's logic; DRY is satisfied.
• Subscribe-before-submit ordering: Still intact — the NewHeadBlock subscription precedes eth_sendRawTransaction, preventing any fast-inclusion race.

---

Verdict: mergeable — no unresolved Critical, High, or Medium findings.

[svlachakis]

@claude review again my refactoring based on @LukaszRozmej point here #11521 (comment)

[claude]

Claude finished @svlachakis's task in 2m 43s —— View job

---

PR Review

• Load repo coding rules
• Read the new HeadBlockSignal.cs and updated EthRpcModule.cs
• Analyze the TCS-based refactoring for correctness and race conditions
• Check thread safety and disposal semantics
• Post findings

---

Summary

All prior Medium+ findings remain fixed. The HeadBlockSignal TCS refactoring is correct and addresses @LukaszRozmej's point. No new Critical, High, or Medium issues.

---

Correctness of the HeadBlockSignal design

The snapshot-before-check ordering in eth_sendRawTransactionSync is correct:

nextHead = _headBlockSignal.NextHeadTask   ← snapshot the TCS task
receiptResult = eth_getTransactionReceipt  ← check state
if (receiptResult.Data is not null) return ← fast exit
await nextHead.WaitAsync(cts.Token)        ← wait if still pending

• If a block arrives before the snapshot: NextHeadTask returns an already-completed Task; WaitAsync returns immediately; next loop iteration finds the receipt.
• If a block arrives between snapshot and receipt check: receipt check finds it; returns immediately.
• If a block arrives between receipt check and WaitAsync: the snapshotted task is already completed; WaitAsync returns immediately and the loop re-checks. The previous N-semaphores approach had this same property; the TCS approach preserves it cleanly.

OnNewHead thread safety: Interlocked.Exchange atomically replaces _tcs and completes the old one. All waiters on the old Task are woken simultaneously via TrySetResult. Volatile.Read(ref _tcs) in NextHeadTask ensures readers always see the latest write. The whole thing is lock-free and correct.

TaskCreationOptions.RunContinuationsAsynchronously: prevents awaiter continuations from running inline on the block-processing thread that calls TrySetResult. Correct.

One HeadBlockSignal per factory, shared across all IEthRpcModule instances: EthModuleFactory creates it once in the field initializer; OptimismEthModuleFactory creates it once in the constructor. Each module's _headBlockSignal field points to the shared singleton. This is the right design — one event subscription regardless of how many concurrent eth_sendRawTransactionSync calls are in flight.

---

Findings

Low — Stale "semaphore" comment in test

EthRpcModuleTests.cs:1448:

// first loop iteration finds it and returns without ever waiting on the semaphore.

Should now say "signal" or "head block signal" since SemaphoreSlim is gone.

Low — HeadBlockSignal does not implement IDisposable

The blockTree.NewHeadBlock += OnNewHead subscription is never removed. For the singleton factory pattern (process lifetime), this is fine in production. In benchmarks (EthModuleBenchmarks.cs) a new HeadBlockSignal is created in each [GlobalSetup] call, leaving a growing delegation list on the block tree — harmless since NullTxPool/NullBlockTree never fire NewHeadBlock, but it's an untidy pattern. Worth implementing IDisposable or at minimum a Dispose() method that removes the subscription, so callers that want clean teardown can opt in.

---

All prior findings — confirmed fixed ✅

Severity	Finding
Critical	NewHeadBlock no-op on ReadOnlyBlockTree → always timed out in production
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast on negative config
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block event
Medium	eth_sendRawTransactionSync non-virtual, Optimism validator nodes always timed out
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods
Low	Optimism eth_sendRawTransactionSync returned plain ReceiptForRpc, omitting L1 fee fields

---

Verdict: mergeable — no unresolved Critical, High, or Medium findings. The TCS refactoring is a clean improvement: one subscription, no per-call allocation of SemaphoreSlim, no lock needed, and the race-avoidance argument is cleaner to reason about.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 3m 21s —— View job

---

PR Review

• Load repo coding rules
• Read changed files
• Analyze correctness and edge cases
• Analyze security implications
• Analyze performance
• Post findings

---

Summary

Both Low findings from the previous review are now resolved. No new Critical, High, or Medium issues found. The PR is in good shape.

---

Fixed from previous review ✅

Severity	Finding	Status
Low	Stale "semaphore" comment in EthRpcModuleTests.cs:1448	Fixed — comment removed
Low	HeadBlockSignal did not implement IDisposable	Fixed — IDisposable implemented; Dispose() unsubscribes the event; benchmark calls it in TearDown()

---

All prior findings — confirmed fixed ✅

Severity	Finding
Critical	NewHeadBlock no-op on ReadOnlyBlockTree → always timed out in production
High	Constructor IBlockFinder cast-and-throw violated LSP
Medium	positionIndex < 0 dead code for UInt256
Medium	ResolveSyncTimeoutMs unsafe (ulong) cast on negative config
Medium	ObjectDisposedException race on SemaphoreSlim from in-flight block event
Medium	eth_sendRawTransactionSync non-virtual, Optimism validator nodes always timed out
Low	Missing ExampleValue on new methods
Low	Missing trace log on new raw-tx methods
Low	Optimism eth_sendRawTransactionSync returned plain ReceiptForRpc, omitting L1 fee fields

---

Pre-existing (not introduced by this PR)

OptimismEthRpcModule.cs:212 — positionIndex < 0 || positionIndex > block!.Transactions.Length - 1: the base EthRpcModule.GetTransactionByBlockAndIndex was corrected in this PR to use positionIndex >= block.Transactions.Length; the Optimism override still uses the old pattern. positionIndex < 0 is dead code for UInt256, and Length - 1 wraps to UInt256.MaxValue for an empty block. Follow-up fix is fine.

---

Verdict: mergeable — no unresolved Critical, High, or Medium findings.