feat(consensus): add STATE_IN_TRIE fork-gate scaffolding (SIP-6, PR 1/N)#755
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR introduces a new consensus-fork activation mechanism for the "state-in-trie" migration (SIP-6 Bug A). It adds compile-time default constants for mainnet and testnet in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/sentrix-core/src/fork_heights.rs (1)
607-635: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd STATE_IN_TRIE to the risky forks test.
Since both
STATE_IN_TRIE_HEIGHT_DEFAULTandSTATE_IN_TRIE_HEIGHT_TESTNET_DEFAULTare set tou64::MAX, the new fork should be verified in this test alongside other risky/disabled forks.✅ Proposed test coverage addition
for var in [ "JAIL_CONSENSUS_HEIGHT", "NFT_TOKENOP_HEIGHT", "EXTENDED_TOUCH_LIST_HEIGHT", "STRICT_JUSTIFICATION_HEIGHT", + "STATE_IN_TRIE_HEIGHT", ] { std::env::remove_var(var); } // AddSelfStake moved out of this test in v2.2.22 — it's now // an enabled testnet fork (see mature_testnet_forks_have_code_ // _level_defaults). Mainnet still defaults disabled. assert_eq!(get_jail_consensus_height(), u64::MAX); assert_eq!(get_nft_tokenop_height(), u64::MAX); assert_eq!(get_extended_touch_list_height(), u64::MAX); assert_eq!(get_strict_justification_height(), u64::MAX); + assert_eq!(get_state_in_trie_height(), u64::MAX); assert!(!is_jail_consensus_height(0)); assert!(!is_nft_tokenop_height(0)); assert!(!is_extended_touch_list_height(0)); assert!(!is_strict_justification_height(0)); + assert!(!is_state_in_trie_height(0)); std::env::remove_var("SENTRIX_CHAIN_ID");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sentrix-core/src/fork_heights.rs` around lines 607 - 635, Add the new STATE_IN_TRIE fork to the risky test by asserting its default/testnet height is u64::MAX and that its predicate returns false; specifically, inside risky_forks_stay_disabled_by_default_on_testnet update the assertions to include assert_eq!(get_state_in_trie_height(), u64::MAX) and assert!(!is_state_in_trie_height(0)) so the test covers STATE_IN_TRIE alongside get_jail_consensus_height, get_nft_tokenop_height, get_extended_touch_list_height and get_strict_justification_height.crates/sentrix-core/src/blockchain.rs (1)
359-361: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
Blockchain::is_state_in_trie_heightdelegator method.Every other fork predicate has a static delegator method in
impl Blockchain(e.g.,is_voyager_height,is_reward_v2_height, etc.). The STATE_IN_TRIE fork should follow the same pattern so callers can useBlockchain::is_state_in_trie_height(h)consistently.🔧 Proposed delegator addition
pub fn is_strict_justification_height(height: u64) -> bool { crate::fork_heights::is_strict_justification_height(height) } + pub fn is_state_in_trie_height(height: u64) -> bool { + crate::fork_heights::is_state_in_trie_height(height) + } + pub fn is_bft_gate_relax_height(height: u64) -> bool { crate::fork_heights::is_bft_gate_relax_height(height) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sentrix-core/src/blockchain.rs` around lines 359 - 361, Add a static delegator method in impl Blockchain named is_state_in_trie_height with the same signature as the other fork predicates (pub fn is_state_in_trie_height(height: u64) -> bool) that simply calls crate::fork_heights::is_state_in_trie_height(height); place it alongside the existing delegators such as is_bft_gate_relax_height so callers can use Blockchain::is_state_in_trie_height(h) consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/sentrix-core/src/blockchain.rs`:
- Around line 359-361: Add a static delegator method in impl Blockchain named
is_state_in_trie_height with the same signature as the other fork predicates
(pub fn is_state_in_trie_height(height: u64) -> bool) that simply calls
crate::fork_heights::is_state_in_trie_height(height); place it alongside the
existing delegators such as is_bft_gate_relax_height so callers can use
Blockchain::is_state_in_trie_height(h) consistently.
In `@crates/sentrix-core/src/fork_heights.rs`:
- Around line 607-635: Add the new STATE_IN_TRIE fork to the risky test by
asserting its default/testnet height is u64::MAX and that its predicate returns
false; specifically, inside risky_forks_stay_disabled_by_default_on_testnet
update the assertions to include assert_eq!(get_state_in_trie_height(),
u64::MAX) and assert!(!is_state_in_trie_height(0)) so the test covers
STATE_IN_TRIE alongside get_jail_consensus_height, get_nft_tokenop_height,
get_extended_touch_list_height and get_strict_justification_height.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ead243f-ba33-4893-b5d4-030ed2598ac5
📒 Files selected for processing (2)
crates/sentrix-core/src/blockchain.rscrates/sentrix-core/src/fork_heights.rs
satyakwok
force-pushed
the
feat/sip-6-state-in-trie-fork-gate
branch
from
5910b72 to
7205657
Compare
1 participant
Summary
First PR in the multi-stage SIP-6 implementation. Compile-only, no behavior change — adds the fork-gate constants + accessor + predicate the later migration PRs will key on. Default is `u64::MAX` on both mainnet and testnet, so this is bit-identical to existing chain history.
Companion to SIPs#3 (SIP-6 draft). Bug A class is the off-trie consensus state drift first surfaced as a 3-way state fork on testnet 2026-05-30 (val3 ClaimRewards at h=5,817,131 → cascade; tracked in issue #750).
Subsequent PRs will:
Each piece sits behind `is_state_in_trie_height(height)` so pre-fork chain history stays untouched.
What this PR changes
Test plan
Summary by CodeRabbit