-
Notifications
You must be signed in to change notification settings - Fork 192
Impl state fork handler in state manager call_raw #7061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sudo-shashank
wants to merge
14
commits into
main
Choose a base branch
from
shashank/fix-call-raw
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
ad5e956
run req migration
sudo-shashank b391f54
Add context
sudo-shashank fc6c9a5
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank 9405aae
impl expensive fork check
sudo-shashank c664609
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank 4066a6f
fmt error msg
sudo-shashank 23e2128
add exp fork check in eth call
sudo-shashank 654f652
fix test
sudo-shashank f3b8ef4
fix test
sudo-shashank 6621857
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank 548b665
propagate error
sudo-shashank 42abd17
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank c982c0f
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank 0593ef5
Merge branch 'main' into shashank/fix-call-raw
sudo-shashank File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about *Fix migrations on calibnet?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Fix are not marked as expensive migrations in lotus here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could implement expensive check by checking height key existence in
get_migrationsresult.get_migrationsmappings withunimplemented_migrationplaceholder@coderabbitai could you check in https://github.com/Filecoin-project/lotus if my understanding is correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: filecoin-project/lotus
Length of output: 5136
🏁 Script executed:
Repository: ChainSafe/forest
Length of output: 1620
🏁 Script executed:
Repository: ChainSafe/forest
Length of output: 3427
🏁 Script executed:
Repository: filecoin-project/lotus
Length of output: 5829
@hanabi1224Your understanding is confirmed. Here's what the Lotus codebase shows:*Why Fix heights are not marked expensive in Lotus:
In
params_mainnet.go, all fix heights are set to negative values with explicit comments:UpgradeWatermelonFixHeight = -1— "This fix upgrade only ran on calibrationnet"UpgradeWatermelonFix2Height = -2— same commentUpgradeCalibrationDragonFixHeight = -3— same commentUpgradeTockFixHeight = -1— "Only applied to calibnet..."Lotus uses a single upgrade schedule (
upgrades.go), but per-network build constants make fix heights negative on mainnet so they're never triggered. Since they don't apply on mainnet, they were never markedExpensive: true. There is no separate mapping per-network — the fix is in the constants.Forest's
get_migrationsis already per-network: It already correctly excludes all fix heights from mainnet and includes them in calibnet:WatermelonFix,WatermelonFix2,DragonFix,TockFixRegarding
@hanabi1224's suggestion: Deriving the expensive check fromget_migrationskeyset makes a lot of sense and would be more correct than the current hard-codedis_expensive_migrationpredicate. The current approach misses marking fix migrations as expensive on calibnet, where they do have real migrations. Usingget_migrationsas the source of truth would automatically handle this correctly without anyunimplemented_migrationplaceholder needed.In practice this would mean: instead of the static
is_expensive_migration(Height)function,has_expensive_fork_between(or theHeightInfo::expensiveflag population) would check whether a given height has an entry inget_migrations(&network)for the relevant network. Any height with a registered migration is inherently expensive.🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai I meant we do need to add mappings like
Height::Trust-> 'unimplemented_migration' to the mappings in Forest to properly markHeight::Trustas expensive to match Lotus. Note that some RPC methods returns error on expensive fork without running the migration, missing the mapping would lead to incorrect resultThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: ChainSafe/forest
Length of output: 15570
@hanabi1224Thank you for the clarification — understood now. The analysis confirms there are two concrete gaps:Gap 1 — Old pre-NV17 heights (Assembly, Trust, Turbo, Hyperdrive, Chocolate, OhSnap, Skyr): These are in
is_expensive_migrationbut have no entry inget_migrationsfor any network, because Forest only implements migrations from NV17 (Shark) onward. Addingunimplemented_migrationplaceholder stubs for these inget_migrationswould let the keyset-based approach cover them correctly.Gap 2 — Calibnet Fix heights (WatermelonFix, WatermelonFix2, DragonFix, TockFix): These already have real migration implementations registered in calibnet's
get_migrations, but are not inis_expensive_migration. This is an actual correctness bug in the current PR:has_expensive_fork_betweenwill returnfalsefor these epochs on calibnet, so the expensive-fork guard won't fire and the node could end up running a heavy migration during an RPC call instead of returningErrExpensiveFork.The proposed fix — deriving
is_expensive_migration(or theHeightInfo::expensiveflag) from theget_migrationskeyset — would resolve both gaps:get_migrations(&network)unimplemented_migrationstubs for old pre-NV17 expensive heights (Assembly, Trust, Turbo, Hyperdrive, Chocolate, OhSnap, Skyr) to each applicable network's migration listThis would make the migration map the single source of truth, with no possibility of the two lists drifting out of sync.
@sudo-shashank— does this refactor make sense to incorporate into this PR, or would you prefer to address it as a follow-up?✏️ Learnings added
🧠 Learnings used