Skip to content

txpool: add txpool_contentFrom, fix txpool_status hex format, fix txpool_content#11305

Merged
svlachakis merged 9 commits into
masterfrom
txpool
Apr 22, 2026
Merged

txpool: add txpool_contentFrom, fix txpool_status hex format, fix txpool_content#11305
svlachakis merged 9 commits into
masterfrom
txpool

Conversation

@svlachakis
Copy link
Copy Markdown
Contributor

@svlachakis svlachakis commented Apr 22, 2026

Fixes Closes Resolves #11308

Changes

  • txpool_content — requires checksumed addresses

  • txpool_statuspending and queued were serialized as raw integers (e.g. 1010). The spec defines them as uint (hex-encoded quantity). Updated the property type from int to long so LongConverter produces the correct wire format (e.g. "0x3f2").

  • txpool_contentFrom — method was missing entirely. Added:

    • ITxPoolRpcModule.txpool_contentFrom(Address address)
    • TxPoolContentFrom response model — filters pool content to a single sender, returning:
      {
        "pending": { "nonce → tx" },
        "queued": { "nonce → tx" }
      }
      (without the address level)
    • Implementation in TxPoolRpcModule
  • txpool_content — no changes needed; nonce keys already serialize as decimal strings (spec requirement) via STJ’s default ulong dictionary key behavior, and blockHash / blockNumber / blockTimestamp / transactionIndex are already null with from present.


Tests

Added serialization-level tests via RpcTest.TestSerializedRequest to validate the actual JSON wire format:

  • txpool_status emits "0xa" instead of 10
  • Nonce map keys are decimal strings ("806") instead of hex ("0x326")
  • Pending transactions always include null block fields and from
  • txpool_contentFrom filters correctly and serializes empty maps as {}

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

Hive tests: https://github.com/NethermindEth/nethermind/actions/runs/24787512946

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @svlachakis's task in 5m 17s —— View job


PR Review: txpool: add txpool_contentFrom, fix txpool_status hex format

  • Gather context and read relevant files
  • Review changed files for correctness, security, and performance
  • Check adherence to repo rules
  • Post findings

Summary

The bug fix for txpool_status is correct: changing Pending/Queued from int (using IntConverter → decimal) to long (using LongConverter → hex) matches the execution-apis spec. The txpool_contentFrom implementation is also correct — Dictionary<ulong, TransactionForRpc> keys serialize as decimal strings by default (no WriteAsPropertyName override on ULongConverter), which is what the spec requires. Test coverage is solid and follows existing patterns.


Findings

Medium

# File Issue
1 TxPoolContentFrom.cs File naming inconsistency — All sibling files use TransactionPool*.cs: TransactionPoolContent.cs, TransactionPoolStatus.cs, TransactionPoolInspection.cs. The new file should be TransactionPoolContentFrom.cs. (Inline comment posted)

Low

# File Issue
2 TransactionPoolStatus.cs:11-12 Sum() returns int before wideningEnumerable.Sum(Func<T, int>) accumulates in int and overflows at ~2.1B entries before the implicit widening to long. Won't happen in practice, but cast inside the lambda: Sum(static t => (long)t.Value.Count). (Inline comment posted)
3 TransactionPoolStatus.cs long vs ulong — Pool counts are semantically non-negative; ulong would be more precise. Both serialize identically as hex via their converters, so this is cosmetic.
4 TxPoolContentFrom.cs Missing XML doc comments — Coding rules require XML docs for all public APIs. The class and its Pending/Queued properties have none.

What's correct

  • IntConverter.WriteWriteNumberValue (decimal) was the actual bug; LongConverter.WriteNumericConverterHelper.Write (hex) is the correct fix. ✓
  • ulong dictionary keys fall back to STJ's built-in decimal property-name behavior (no WriteAsPropertyName on ULongConverter), producing "806" not "0x326". ✓
  • TxPoolContentFrom.MapTransactions uses manual iteration — correct per the no-LINQ-on-simple-loops rule. ✓
  • Empty address returns new Dictionary<ulong, TransactionForRpc>(0) → serializes as {}. ✓
  • File-scoped namespaces on new/modified files. ✓
  • Test coverage validates wire format, empty cases, and filtering. ✓

@@ -0,0 +1,38 @@
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — File naming inconsistency

The sibling files in this directory follow a TransactionPool*.cs naming convention:

  • TransactionPoolContent.cs → class TxPoolContent
  • TransactionPoolStatus.cs → class TxPoolStatus
  • TransactionPoolInspection.cs → class TxPoolInspection

This new file breaks that pattern. It should be renamed to TransactionPoolContentFrom.cs for consistency.

Fix this →

Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TransactionPoolStatus.cs Outdated
Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TransactionPoolStatus.cs Outdated
@svlachakis svlachakis changed the title txpool: add txpool_contentFrom, fix txpool_status hex format txpool: add txpool_contentFrom, fix txpool_status hex format, fix txpool_content Apr 22, 2026
@svlachakis
Copy link
Copy Markdown
Contributor Author

@claude review again please

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

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


PR Review: txpool: add txpool_contentFrom, fix txpool_status hex format

  • Gather context and read relevant files
  • Review changed files for correctness, security, and performance
  • Check adherence to repo rules
  • Post findings

Summary

All three findings from the previous review have been addressed. The implementation is correct and the PR is in good shape.


Previous findings — status

# Severity Finding Status
1 Medium File naming: TxPoolContentFrom.cs should be TransactionPoolContentFrom.cs Resolved — file is TransactionPoolContentFrom.cs
2 Low Sum() accumulates in int, overflows before widening to long Resolved(ulong)info.Pending.Sum(static t => (long)t.Value.Count)
3 Low long vs ulong for pool counts Resolved — now ulong
4 Low Missing XML doc comments on TxPoolContentFrom Resolved — class summary and property <summary> docs added

New findings

Low

# File Issue
1 ITxPoolRpcModule.cs:22 ExampleResponse is missing blockTimestampTransactionForRpc.BlockTimestamp is declared [JsonIgnore(Condition = JsonIgnoreCondition.Never)], so it serializes as "blockTimestamp":null for mempool transactions. The example string omits this field. Not runtime-impacting, but the doc example is inaccurate.

What's correct

  • ulong Pending/QueuedULongConverterNumericConverterHelper.Write → hex string in JSON-RPC Hex mode. Both ulong and long route through the same helper; the fix is correct. ✓
  • (ulong)Sum(static t => (long)t.Value.Count) — accumulates in 64-bit (long overload of Sum), then safely widened to ulong since counts are non-negative. ✓
  • Dictionary<string, ...> keyed by address.ToString(withZeroX: true, withEip55Checksum: true) — the old Dictionary<AddressAsKey, ...> routed through AddressConverter.WriteAsPropertyName which outputs lowercase hex; the new approach pre-computes the EIP-55 key. ✓
  • Dictionary<ulong, TransactionForRpc> keys — STJ uses the built-in decimal integer property-name serialization (no WriteAsPropertyName override on ULongConverter), so nonce 806 → "806" not "0x326". ✓
  • MapTransactions returns new Dictionary<ulong, TransactionForRpc>(0) when address is missing → serializes as {}. ✓
  • No LINQ in the new mapping helpers — manual foreach. ✓
  • File-scoped namespaces on all new/converted files. ✓
  • TransactionPoolContent.cs retains brace-style namespace — correct per the rule ("for existing files, follow their style"). ✓
  • Test coverage: wire-format serialization tests for txpool_status hex counts, decimal nonce keys, null block fields, txpool_contentFrom filtering, and empty-address cases. ✓

@svlachakis
Copy link
Copy Markdown
Contributor Author

@claude review again

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

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


PR Review: txpool: add txpool_contentFrom, fix txpool_status hex format

  • Gather context and read relevant files
  • Review changed files for correctness, security, and performance
  • Check adherence to repo rules
  • Post findings

Previous findings — status

# Severity Finding Status
1 Medium File naming: TxPoolContentFrom.csTransactionPoolContentFrom.cs ✅ Resolved
2 Low Sum() accumulates in int, overflows before widening ✅ Resolved — (ulong)Sum(static t => (long)t.Value.Count)
3 Low long vs ulong for pool counts ✅ Resolved — now ulong
4 Low Missing XML doc comments on TxPoolContentFrom ✅ Resolved
5 Low txpool_contentFrom ExampleResponse omitting blockTimestamp ✅ Resolved

New findings

Low

# File Issue
1 ITxPoolRpcModule.cs:16 txpool_content ExampleResponse omits blockTimestamp — The txpool_contentFrom example was fixed in this PR; the txpool_content example still has "blockNumber":null,"transactionIndex":null with blockTimestamp absent. The new test explicitly asserts blockTimestamp:null is always emitted. (Inline comment posted)
2 TxPoolRpcModule.cs:28 Missing null guard on addressaddress is untrusted RPC caller input; the robustness rules require ArgumentNullException.ThrowIfNull(address) before dereferencing it. A null address passes silently into AddressAsKey key = address and throws NullReferenceException. (Inline comment posted)

What's correct

  • ulong Pending/Queued + ULongConverter → hex wire format. ✓
  • (ulong)Sum(static t => (long)t.Value.Count) — 64-bit accumulation, safe widening. ✓
  • Dictionary<string, ...> keyed by address.ToString(withZeroX: true, withEip55Checksum: true) — correct EIP-55 keys. ✓
  • Dictionary<ulong, TransactionForRpc> nonce keys — STJ decimal property-name serialization ("806" not "0x326"). ✓
  • MapTransactions returns new Dictionary<ulong, TransactionForRpc>(0) on missing address → serializes as {}. ✓
  • No LINQ in the new mapping helpers — manual foreach throughout. ✓
  • File-scoped namespaces on all new/converted files. ✓
  • TransactionPoolContent.cs retains brace-style namespace (existing file convention). ✓
  • Test coverage: hex counts, decimal nonce keys, null block fields, filtering, empty-address cases. ✓

Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/ITxPoolRpcModule.cs Outdated
Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TxPoolRpcModule.cs
@svlachakis
Copy link
Copy Markdown
Contributor Author

@claude review again

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

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


PR Review: txpool: add txpool_contentFrom, fix txpool_status hex format

  • Gather context and read relevant files
  • Review changed files for correctness, security, and performance
  • Check adherence to repo rules
  • Post findings

Previous findings — status

# Severity Finding Status
1 Medium File naming: TxPoolContentFrom.csTransactionPoolContentFrom.cs ✅ Resolved
2 Low Sum() accumulates in int, overflows before widening to long ✅ Resolved — (ulong)Sum(static t => (long)t.Value.Count)
3 Low long vs ulong for pool counts ✅ Resolved — now ulong
4 Low Missing XML doc comments on new class ✅ Resolved — class summary and property <summary> docs added
5 Low txpool_contentFrom ExampleResponse omitting blockTimestamp ✅ Resolved
6 Low txpool_content ExampleResponse omitting blockTimestamp ✅ Resolved — "blockTimestamp":null is present in the example string
7 Low Missing ArgumentNullException.ThrowIfNull(address) before dereferencing RPC input ✅ Resolved — TxPoolRpcModule.cs:31

New findings

None.


What's correct

  • ulong Pending/Queued + ULongConverterNumericConverterHelper.Write → hex wire format. ✓
  • (ulong)Sum(static t => (long)t.Value.Count) — 64-bit accumulation, safe widening to ulong. ✓
  • Dictionary<string, ...> keyed by address.ToString(withZeroX: true, withEip55Checksum: true) — correct EIP-55 keys on outer dict. ✓
  • Dictionary<ulong, TransactionForRpc> nonce keys — STJ decimal property-name serialization ("806" not "0x326"). ✓
  • MapTransactions returns new Dictionary<ulong, TransactionForRpc>(0) on missing address → serializes as {}. ✓
  • ArgumentNullException.ThrowIfNull(address) guards the RPC input before any dereference. ✓
  • No LINQ in new mapping helpers — manual foreach throughout. ✓
  • File-scoped namespaces on all new/modified files (existing TransactionPoolContent.cs retains brace-style — correct). ✓
  • Test coverage: hex counts, decimal nonce keys, null block fields, EIP-55 address keys, filtering, empty-address wire format. ✓

The PR is clean. All previously raised findings are resolved and no new issues were found.

@svlachakis svlachakis marked this pull request as ready for review April 22, 2026 15:43
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

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


PR Review: txpool: add txpool_contentFrom, fix txpool_status hex format, fix txpool_content

  • Gather context and read relevant files
  • Review changed files for correctness, security, and performance
  • Check adherence to repo rules
  • Post findings

Previous findings — status

# Severity Finding Status
1 Medium File naming: TxPoolContentFrom.csTransactionPoolContentFrom.cs ✅ Resolved
2 Low Sum() accumulates in int, overflows before widening to long ✅ Resolved — (ulong)Sum(static t => (long)t.Value.Count)
3 Low long vs ulong for pool counts ✅ Resolved — now ulong
4 Low Missing XML doc comments on new class ✅ Resolved — class summary and property <summary> docs added
5 Low txpool_contentFrom ExampleResponse omitting blockTimestamp ✅ Resolved
6 Low txpool_content ExampleResponse omitting blockTimestamp ✅ Resolved
7 Low Missing ArgumentNullException.ThrowIfNull(address) before dereferencing RPC input ✅ Resolved — TxPoolRpcModule.cs:31

New findings

None.


What's correct

  • ulong Pending/Queued + ULongConverterNumericConverterHelper.Write → hex wire format. ✓
  • (ulong)Sum(static t => (long)t.Value.Count) — 64-bit accumulation, safe widening to ulong. ✓
  • Dictionary<string, ...> keyed by address.ToString(withZeroX: true, withEip55Checksum: true) — correct EIP-55 keys on outer dict. ✓
  • Dictionary<ulong, TransactionForRpc> nonce keys — STJ decimal property-name serialization ("806" not "0x326"). ✓
  • MapTransactions returns new Dictionary<ulong, TransactionForRpc>(0) on missing address → serializes as {}. ✓
  • ArgumentNullException.ThrowIfNull(address) guards the RPC input before any dereference. ✓
  • No LINQ in new mapping helpers — manual foreach throughout. ✓
  • File-scoped namespaces on all new/modified files (existing TransactionPoolContent.cs retains brace-style — correct per rule). ✓
  • Test coverage: hex counts, decimal nonce keys, null block fields, EIP-55 address keys, filtering, empty-address wire format. ✓

The PR is clean. All previously raised findings are resolved and no new issues were found.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Implements missing txpool_contentFrom and aligns txpool RPC JSON serialization with the spec (hex-quantity counts and checksummed address keys), with added serialization-level tests.

Changes:

  • Added txpool_contentFrom(Address address) RPC method + TxPoolContentFrom response model.
  • Updated txpool_status response to serialize pending/queued as hex-encoded quantities.
  • Updated txpool_content to emit EIP-55 checksummed address keys and refactored mapping logic.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TxPoolRpcModule.cs Adds txpool_contentFrom endpoint and modernizes module declaration.
src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TransactionPoolStatus.cs Changes status counts to unsigned type for hex-quantity serialization.
src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TransactionPoolContentFrom.cs Introduces response model that filters txpool content to a single sender.
src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TransactionPoolContent.cs Emits checksummed address keys and refactors mapping to explicit loops.
src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/ITxPoolRpcModule.cs Adds new RPC method and updates txpool_status example response.
src/Nethermind/Nethermind.JsonRpc.Test/Modules/TxPoolRpcModuleTests.cs Adds wire-format serialization tests covering status/content/contentFrom behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Nethermind/Nethermind.JsonRpc/Modules/TxPool/TxPoolRpcModule.cs
Copy link
Copy Markdown
Contributor

@batrr batrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to address the allocation/perf comments from #11048; not critical though

@svlachakis
Copy link
Copy Markdown
Contributor Author

@batrr it becomes very messy because we would need two GetInfo() & GetInfo(Address)..that's why I didn't.

  • GetInfo() - full pool scan, used by txpool_content, txpool_status, and txpool_inspect (all need the whole pool)
  • GetInfo(Address) - single-address scan, used only by txpool_contentFrom

There's no way to collapse them without regressing the other three methods. The only way to eliminate GetInfo() entirely would be to rewrite txpool_status to count directly (without building the full TxPoolInfo) and similarly for the others but that's a broader refactor that I can do as a separate PR.

@svlachakis svlachakis merged commit b54bbb8 into master Apr 22, 2026
455 checks passed
@svlachakis svlachakis deleted the txpool branch April 22, 2026 17:25
asdacap pushed a commit that referenced this pull request Apr 25, 2026
… `txpool_content` (#11305)

* txpool fixes & tests

* fix hive get_content

* claude review

* checksummed address only in get-content

* build fix

* claude review

* claude review

* format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add txpool_contentFrom, fix txpool_status hex format, fix txpool_content

5 participants