fix: `eth_getTransactionCount` to return the actual count, not parent's #6103

LesnyRumcajs · 2025-09-22T12:46:37Z

Summary of changes

Changes introduced in this pull request:

Fixed eth_getTransactionCount to return the nonce of the requested tipset and not its parent. Sadly, no bonus points for tests here.

Manually tested this transaction (note the tipset https://beryx.io/fil/calibration/txs/0x7a4de7139c4748e1df0e4aa1c02f28f69d1c3a9f9646678c17979afcaf59d529)

before

❯ curl --silent -X POST -H "Content-Type: application/json" \
             --data '{"jsonrpc":"2.0","id":2,"method":"eth_getTransactionCount","params": [ "0x72ad7c4d27fc9c2bdf8fde34ddd9bcbe34648af3", "0x2E6527" ] }' \
             localhost:2345
{"jsonrpc":"2.0","id":2,"result":"0x169"}

after

❯ curl --silent -X POST -H "Content-Type: application/json" \
             --data '{"jsonrpc":"2.0","id":2,"method":"eth_getTransactionCount","params": [ "0x72ad7c4d27fc9c2bdf8fde34ddd9bcbe34648af3", "0x2E6527" ] }' \
             localhost:2345
{"jsonrpc":"2.0","id":2,"result":"0x16a"}

Reference issue to close (if applicable)

Part of #5979

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

Bug Fixes
- Corrected eth_getTransactionCount to return the nonce from the requested tipset rather than its parent, ensuring accurate results across reorgs and tipset boundaries.
Documentation
- Updated changelog to record the eth_getTransactionCount fix and clarify behavior.

coderabbitai · 2025-09-22T12:46:45Z

Walkthrough

Updates eth_getTransactionCount to derive the state from the requested tipset via an async tipset_state call and then construct the StateTree from that state root. Adjusts StateManager::tipset_state documentation to clarify return values. Adds a changelog entry describing the fix.

Changes

Cohort / File(s)	Summary
RPC: tipset state retrieval change `src/rpc/methods/eth.rs`	Switches from using `ts.parent_state()` to asynchronously calling `tipset_state(ts)` to obtain the tipset’s state root, then constructs the `StateTree` from that CID; updates error handling path accordingly.
State manager docs `src/state_manager/mod.rs`	Clarifies docs: `tipset_state` returns (state root, receipt root) of the tipset, not the parent. No code changes.
Changelog `CHANGELOG.md`	Adds unreleased note: `eth_getTransactionCount` returns nonce of the requested tipset rather than its parent.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Client
  participant E as eth_getTransactionCount (RPC)
  participant SM as StateManager
  participant TS as Tipset
  participant ST as StateTree

  C->>E: Request tx count (addr, tipset key)
  E->>SM: tipset_state(TS) (async)
  SM-->>E: (state_root, receipt_root)
  E->>ST: construct from state_root
  ST-->>E: actor state (nonce)
  E-->>C: nonce

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

elmattic

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly and accurately describes the primary change: eth_getTransactionCount now returns the nonce for the requested tipset rather than the parent's nonce. This matches the code change in src/rpc/methods/eth.rs, the updated tipset_state documentation, and the new changelog entry outlined in the PR objectives. The phrasing is specific, actionable, and appropriate for scanning commit/PR history.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch port-eth-get-code-fix

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/state_manager/mod.rs (3)
396-406: Doc fix aligns with behavior (on-tipset state, not parent).

Nice clarification. Consider explicitly stating “for tipset T (not T’s parent)” to remove any residual ambiguity for new contributors.

1690-1699: Potential type-conversion nit: prefer deref over into() for &Cid.

state_root is &Cid (from parent_state()), but StateOutputValue.state_root is Cid. Using state_root.into() relies on a From<&Cid> for Cid impl; elsewhere in this file you use *ts.parent_state() which is clearer and avoids surprising trait requirements.

Apply this small tweak for consistency:
-                return Some(StateOutputValue {
-                    state_root: state_root.into(),
-                    receipt_root,
-                });
+                return Some(StateOutputValue {
+                    state_root: *state_root,
+                    receipt_root,
+                });
396-406: Optional: add a convenience helper to reduce repeated patterns.

Given more callers (e.g., eth RPC) now need the on-tipset state, a get_state_tree_for_tipset(&Arc<Tipset>) -> StateTree<DB> helper that internally calls tipset_state would simplify call sites and prevent future regressions to parent_state().

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f2becb and 0f7c715.

📒 Files selected for processing (3)

CHANGELOG.md (1 hunks)
src/rpc/methods/eth.rs (1 hunks)
src/state_manager/mod.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: LesnyRumcajs
PR: ChainSafe/forest#5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.

🧬 Code graph analysis (1)

src/rpc/methods/eth.rs (2)

src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)

ctx (106-159)

src/shim/state_tree.rs (1)

new_from_root (168-188)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: Build Ubuntu
GitHub Check: cargo-publish-dry-run
GitHub Check: Build MacOS
GitHub Check: All lint checks
GitHub Check: Build forest binaries on Linux AMD64
GitHub Check: tests-release
GitHub Check: tests

🔇 Additional comments (3)

CHANGELOG.md (1)

44-45: Changelog entry is clear and accurate.

Good, succinct description; matches the PR objective (on-tipset nonce vs parent).

src/state_manager/mod.rs (1)

396-406: Verify eth_getTransactionCount uses tipset_state(...) instead of ts.parent_state()

Check src/rpc/methods/eth.rs (EthGetTransactionCount impl — ~line 2227) and confirm the RPC builds the StateTree from tipset_state(tipset), not ts.parent_state(). To verify locally, run:
rg -nF 'eth_getTransactionCount' src/rpc/methods/eth.rs
rg -nF 'parent_state(' src/rpc/methods/eth.rs || true
rg -nF 'tipset_state(' src/rpc/methods/eth.rs || true

src/rpc/methods/eth.rs (1)

2255-2258: LGTM! Correct fix for eth_getTransactionCount implementation.

The change correctly implements returning the nonce of the requested tipset instead of its parent. According to Filecoin documentation, "The state looked up is the parent state of the tipset", but for eth_getTransactionCount, we want the state after the tipset's messages have been executed, which is achieved by calling tipset_state on the tipset itself and then constructing the StateTree from that result.

The async call to ctx.state_manager.tipset_state(&ts) followed by StateTree::new_from_root(ctx.store_owned(), &state_cid) correctly gets the state after the tipset execution, ensuring the transaction count reflects transactions that have been executed up to and including the requested tipset.

elmattic · 2025-09-22T13:06:39Z

Sadly, no bonus points for tests here.

Is this a breaking change? Have you considered adding an rpcsnap file to verify that the RPC method now behaves correctly?

LesnyRumcajs · 2025-09-22T14:09:38Z

Sadly, no bonus points for tests here.

Is this a breaking change? Have you considered adding an rpcsnap file to verify that the RPC method now behaves correctly?

I could add it, but the problem is that it'd be difficult to re-create if needed. So I decided not to add it at all.

In general, such tests would be better suited for running on devnet, but we don't have them (yet).

LesnyRumcajs marked this pull request as ready for review September 22, 2025 12:52

LesnyRumcajs requested a review from a team as a code owner September 22, 2025 12:52

LesnyRumcajs requested review from elmattic and sudo-shashank and removed request for a team September 22, 2025 12:52

fix: eth_getTransactionCount to return the actual count, not parent

0f7c715

LesnyRumcajs force-pushed the port-eth-get-code-fix branch from 818ce0f to 0f7c715 Compare September 22, 2025 12:53

LesnyRumcajs changed the title ~~fix: eth_getTransactionCount to return the actual count, not parent~~ fix: eth_getTransactionCount to return the actual count, not parent's Sep 22, 2025

sudo-shashank approved these changes Sep 22, 2025

View reviewed changes

coderabbitai bot reviewed Sep 22, 2025

View reviewed changes

elmattic approved these changes Sep 22, 2025

View reviewed changes

LesnyRumcajs added this pull request to the merge queue Sep 22, 2025

Merged via the queue into main with commit 8a105a1 Sep 22, 2025
50 checks passed

LesnyRumcajs deleted the port-eth-get-code-fix branch September 22, 2025 14:34

coderabbitai bot mentioned this pull request Jan 8, 2026

Add support for the Filecoin.EthGetTransactionCount V2 #6387

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: `eth_getTransactionCount` to return the actual count, not parent's #6103

fix: `eth_getTransactionCount` to return the actual count, not parent's #6103

Uh oh!

LesnyRumcajs commented Sep 22, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Sep 22, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

elmattic commented Sep 22, 2025

Uh oh!

LesnyRumcajs commented Sep 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: eth_getTransactionCount to return the actual count, not parent's #6103

fix: eth_getTransactionCount to return the actual count, not parent's #6103

Uh oh!

Conversation

LesnyRumcajs commented Sep 22, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

elmattic commented Sep 22, 2025

Uh oh!

LesnyRumcajs commented Sep 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: `eth_getTransactionCount` to return the actual count, not parent's #6103

fix: `eth_getTransactionCount` to return the actual count, not parent's #6103

LesnyRumcajs commented Sep 22, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Sep 22, 2025 •

edited

Loading