fix(ci): rpc parity test#6071
Merged
hanabi1224 merged 4 commits intomainfrom Sep 11, 2025
Merged
Conversation
Contributor
WalkthroughEVMStateLotusJson’s Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
hanabi1224
requested review from
akaladarshi and
sudo-shashank
and removed request for
a team
LesnyRumcajs
previously approved these changes
Sep 11, 2025
…o hm/fix-rpc-parity-test
LesnyRumcajs
approved these changes
Sep 11, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lotus_json/actor_states/evm_state.rs (1)
94-111: Bug: typo in Tombstone mapping (“orign” -> “origin”).This will fail to compile or panic at runtime depending on the type; it must be fixed.
Apply:
- origin: lotus_entry.orign, + origin: lotus_entry.origin,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lotus_json/actor_states/evm_state.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: akaladarshi
PR: ChainSafe/forest#5996
File: src/lotus_json/actor_states/methods/paych_params.rs:269-277
Timestamp: 2025-09-02T08:46:04.937Z
Learning: In payment channel actor Lotus JSON serialization, empty merges should be serialized as `null` (not an empty array `[]`) to maintain compatibility with Lotus behavior.
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
⏰ 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: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
🔇 Additional comments (3)
src/lotus_json/actor_states/evm_state.rs (3)
56-65: Snapshot includes TransientData: null — LGTM.This aligns the expected JSON with Lotus for V16, matching our prior learning to prefer null over empty/omitted for parity-sensitive cases.
76-83: into_lotus_json correctly ensures “field-present” semantics for V16.Using Option::from(state.transient_data) yields Some(None) when absent, so JSON emits null (and, with the ref above, remains omitted for other versions). Looks good.
26-26: Confirm version note.Comment says “only available in evm actor state v16”. If V17 still includes this field, update the comment; if not, the skip_serializing_if change above is mandatory to avoid emitting an extraneous field for V17.
Comment on lines
24
to
26
| #[schemars(with = "LotusJson<Option<TransientData>>")] | ||
| #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] | ||
| #[serde(with = "crate::lotus_json")] | ||
| pub transient_data: Option<Option<TransientData>>, // only available in evm actor state v16 |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t force TransientData on non-V16 states; keep skip_serializing_if to avoid regressions.
Without skip_serializing_if, Option::None serializes as null and the field is always present, which likely diverges from Lotus for versions where this field doesn’t exist. Keep the field omitted for non-V16 and explicitly include it (as null when absent) only for V16 via Some(None). This still fixes the parity failure while avoiding new ones.
Apply:
- #[schemars(with = "LotusJson<Option<TransientData>>")]
- #[serde(with = "crate::lotus_json")]
+ #[schemars(with = "LotusJson<Option<TransientData>>")]
+ #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")]
pub transient_data: Option<Option<TransientData>>, // only available in evm actor state v16📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[schemars(with = "LotusJson<Option<TransientData>>")] | |
| #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] | |
| #[serde(with = "crate::lotus_json")] | |
| pub transient_data: Option<Option<TransientData>>, // only available in evm actor state v16 | |
| #[schemars(with = "LotusJson<Option<TransientData>>")] | |
| #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] | |
| pub transient_data: Option<Option<TransientData>>, // only available in evm actor state v16 |
🤖 Prompt for AI Agents
In src/lotus_json/actor_states/evm_state.rs around lines 24 to 26, the transien
t_data field currently serializes Option::None as null which forces the field to
always appear; add serde's skip_serializing_if on the outer Option so the field
is omitted for non-v16 states (i.e. #[serde(skip_serializing_if =
"Option::is_none")]) while preserving the type Option<Option<TransientData>> so
you can still represent v16 explicitly as Some(None) (serializes as null) and
earlier versions as None (omitted). Ensure the serde attribute is applied
alongside the existing crate::lotus_json attribute and keep the schemars line
unchanged.
💡 Verification agent
🧩 Analysis chain
Quick repository checks to prevent parity regressions.
Run to verify no accidental inclusions and catch the typo elsewhere.
Also applies to: 35-47, 76-83, 94-111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Ensure only V16 paths set transient_data to Some(...)
rg -n -C2 'transient_data:\s*Option::from\(' src | cat
# 2) Verify common_evm_state_fields leaves transient_data as None for non-V16
rg -n -C2 'common_evm_state_fields!\(state,\s*V(1[0-9])\)' src | cat
# 3) Search for the typo anywhere else
rg -n '\borign\b' -C2
# 4) Grep snapshots mentioning TransientData to ensure expectations are explicit
rg -nP '"transient_data"\s*:\s*null' -C2Length of output: 616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== evm_state.rs (first 300 lines) ==="
sed -n '1,300p' src/lotus_json/actor_states/evm_state.rs || true
echo
echo "=== All occurrences of 'transient_data' in src ==="
rg -n --hidden --no-ignore-vcs -C2 '\btransient_data\b' src || true
echo
echo "=== occurrences of 'transient_data: Option::from(' ==="
rg -n --hidden --no-ignore-vcs -C2 'transient_data:\s*Option::from\(' src || true
echo
echo "=== occurrences of 'Option::from(state.transient_data)' ==="
rg -n --hidden --no-ignore-vcs -C2 'Option::from\(state\.transient_data\)' src || true
echo
echo "=== occurrences of 'common_evm_state_fields!' ==="
rg -n --hidden --no-ignore-vcs -C2 'common_evm_state_fields!' src || true
echo
echo "=== Match arms 'State::V...' in evm_state.rs ==="
rg -n --hidden --no-ignore-vcs -C2 'State::V[0-9]+' src/lotus_json/actor_states/evm_state.rs || true
echo
echo "=== Search repo for typo 'orign' ==="
rg -n --hidden --no-ignore-vcs -C2 '\borign\b' || true
echo
echo "=== Search for JSON snapshots with '\"transient_data\": null' ==="
rg -n --hidden --no-ignore-vcs -C2 -P '"transient_data"\s*:\s*null' || trueLength of output: 12292
transient_data parity OK; rename tombstone 'orign' field
- transient_data is only populated for State::V16 (src/lotus_json/actor_states/evm_state.rs:80); common_evm_state_fields defaults it to None for other versions (src/lotus_json/actor_states/evm_state.rs:44); snapshots/migrations reflect null/None.
- Rename TombstoneLotusJson.pub orign → origin and update all uses: src/lotus_json/tombstone.rs (decl: line 12; mappings: lines 39, 47) and src/lotus_json/actor_states/evm_state.rs (use: line 106).
🤖 Prompt for AI Agents
In src/lotus_json/actor_states/evm_state.rs around lines 24 to 26 and referenced
uses at line 80, 44 and 106: transient_data parity is correct so leave the
Option<Option<TransientData>> as-is, but rename the TombstoneLotusJson field
from pub orign to pub origin and update all usages accordingly; specifically,
change the declaration in src/lotus_json/tombstone.rs (line ~12) to origin and
update the mapping keys/usages at lines ~39 and ~47, then update the use site in
src/lotus_json/actor_states/evm_state.rs (line ~106) to reference origin instead
of orign so all references compile and JSON mappings remain consistent.
akaladarshi
approved these changes
Sep 11, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tool/subcommands/api_cmd/stateful_tests.rs (1)
633-635: Temporarily skipping eth_newBlockFilter is fine; please surface the ignore reason in output and plan re-enable.Good call linking to #6069. To aid triage in CI logs, print the ignore reason when a test is skipped.
Apply this diff to run_tests to include the reason:
- if test.ignore.is_some() { - ignored += 1; - println!( - "test {} ... ignored", - if let Some(name) = test.name { - name.to_string() - } else { - format!("#{i}") - }, - ); - continue; - } + if let Some(reason) = test.ignore { + ignored += 1; + println!( + "test {} ... ignored ({reason})", + if let Some(name) = test.name { + name.to_string() + } else { + format!("#{i}") + }, + ); + continue; + }Optional: gate the skip behind an env var (e.g., RUN_IGNORED=1) so local runs can still exercise the test while CI skips it. Want a patch for that?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tool/subcommands/api_cmd/stateful_tests.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hanabi1224
PR: ChainSafe/forest#5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
⏰ 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). (21)
- GitHub Check: State migrations
- GitHub Check: Wallet tests
- GitHub Check: Calibnet no discovery checks
- GitHub Check: Calibnet eth mapping check
- GitHub Check: Calibnet api test-stateful check
- GitHub Check: Calibnet RPC checks
- GitHub Check: Bootstrap checks - Forest
- GitHub Check: Devnet checks
- GitHub Check: Snapshot export checks v2 with F3 data
- GitHub Check: Snapshot export checks
- GitHub Check: Bootstrap checks - Lotus
- GitHub Check: db-migration-checks
- GitHub Check: Calibnet kademlia checks
- GitHub Check: Calibnet check
- GitHub Check: Calibnet stateless mode check
- GitHub Check: Calibnet stateless RPC check
- GitHub Check: Forest CLI checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: All lint checks
Contributor
Author
|
Hey @akaladarshi any ideas on this? More failures: https://github.com/ChainSafe/forest/actions/runs/17643479222/job/50136221369?pr=6071#step:5:224 Failure dump: {
"request": {
"method_name": "Filecoin.StateDecodeParams",
"params": [
"t00",
1,
null,
[
{
"/": "bafy2bzacecdrw63hrpger553zsyj2whwyf57kblfl6dlyjao2idhqrcupy5d4"
},
{
"/": "bafy2bzaced4ujqhfirb3fshi33szrwdh23w4gdmpra7ifws36a6q6vxzyirbk"
}
]
]
},
"forest_response": {
"Err": "ErrorObject { code: InternalError, message: \"No deserializer registered for actor type System (v17), method 1\", data: None }"
},
"lotus_response": {
"Ok": {}
}
} |
Signor1
pushed a commit
to Signor1/forest
that referenced
this pull request
Sep 12, 2025
4 tasks
4 tasks
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
To fix https://github.com/ChainSafe/forest/actions/runs/17642335761/job/50132589577?pr=6068#step:5:262
Failure dump:
{ "request": { "method_name": "Filecoin.StateReadState", "params": [ "t410fbqoynu2oi2lxam43knqt6ordiowm2ywlml27z4i", [ { "/": "bafy2bzacecdrw63hrpger553zsyj2whwyf57kblfl6dlyjao2idhqrcupy5d4" }, { "/": "bafy2bzaced4ujqhfirb3fshi33szrwdh23w4gdmpra7ifws36a6q6vxzyirbk" } ] ] }, "forest_response": { "Ok": { "Balance": "0", "Code": { "/": "bafk2bzacecj6erwjsemkinlja2nwq3bwjbr7jvhde4hbeljsbocl7msygujti" }, "State": { "Bytecode": { "/": "bafk2bzacec5kwj6sqfhoehbp6m3mmg6634bvxhmyg6gfm5bryxu6vz6fabkn4" }, "BytecodeHash": [ 199, 80, 91, 60, 118, 199, 50, 70, 67, 11, 129, 231, 223, 126, 88, 75, 151, 90, 75, 168, 124, 112, 132, 25, 127, 198, 74, 169, 254, 59, 223, 250 ], "ContractState": { "/": "bafy2bzacec2ljacfbkr2n7utne53n5rh27ja7wrxmdqyvccljei7lvoyxcqgy" }, "Nonce": 1, "Tombstone": null } } }, "lotus_response": { "Ok": { "Balance": "0", "Code": { "/": "bafk2bzacecj6erwjsemkinlja2nwq3bwjbr7jvhde4hbeljsbocl7msygujti" }, "State": { "Bytecode": { "/": "bafk2bzacec5kwj6sqfhoehbp6m3mmg6634bvxhmyg6gfm5bryxu6vz6fabkn4" }, "BytecodeHash": [ 199, 80, 91, 60, 118, 199, 50, 70, 67, 11, 129, 231, 223, 126, 88, 75, 151, 90, 75, 168, 124, 112, 132, 25, 127, 198, 74, 169, 254, 59, 223, 250 ], "ContractState": { "/": "bafy2bzacec2ljacfbkr2n7utne53n5rh27ja7wrxmdqyvccljei7lvoyxcqgy" }, "Nonce": 1, "Tombstone": null, "TransientData": null } } } }Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6067
Closes #6066
Other information and links
Change checklist
Summary by CodeRabbit
Bug Fixes
Tests