Skip to content

chore: add TestNet block activation for v1.4.1 hardforks - #190

Draft
bdimitrov-netzine wants to merge 1 commit into
mainfrom
hardfork/v1.4.1-output-seq-sender-affinity
Draft

bdimitrov-netzine wants to merge 1 commit into
mainfrom
hardfork/v1.4.1-output-seq-sender-affinity

Conversation

@bdimitrov-netzine

Copy link
Copy Markdown
Contributor

The problem

Summary

Closes #

Surface areas touched

  • Consensus protocol (primary / worker / network / state-sync)
  • Execution / EVM
  • JSON-RPC (eth_*, rayls_*, faucet)
  • Middleware (orchestrator / processor / bridge)
  • Infrastructure (types / storage / config / network-cli)
  • On-chain contracts (rayls-contracts/)
  • Operations (etc/, scripts, Docker, compose)
  • CI / build (.github/workflows/, Makefile)
  • Documentation only (doc/, in-crate READMEs, root docs)
  • Tests only

Breaking / compatibility

None.

Test plan

  • [ ]
  • [ ]

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @bdimitrov-netzine's task in 1m 13s —— View job


Code Review

  • Gather context
  • Read changed file and diff
  • Post review findings

Summary

Straightforward configuration change activating two v1.4.1 hardforks on testnet by promoting them from ForkCondition::Never to ForkCondition::Block(17_300_000). The code is clean and follows the established patterns in this file.


Observations

Same block for both forks (17_300_000)

Both OutputSeqNormalization and SenderAffinityLoadBalancing are set to the same activation block. Based on the hardfork descriptions they appear functionally independent (one normalises batch ordering, the other changes committee-slot dispatch keying), but it's worth confirming this simultaneous activation is intentional and that neither migration step depends on the other having run first.

No tests for the new testnet activation blocks

The existing test suite has activation tests for earlier hardforks following this pattern:

#[test]
fn admin_transfer_active_after_testnet_activation_block() {
    let hardforks = RaylsChainHardforks::for_network(RaylsNetwork::Testnet);
    assert!(!hardforks.is_rayls_fork_active_at_block(...BLOCK - 1));
    assert!( hardforks.is_rayls_fork_active_at_block(...BLOCK + 1));
}

#[test]
fn admin_transfer_never_activates_on_devnet() { ... }

The new constants (TESTNET_OUTPUT_SEQ_NORMALIZATION_BLOCK, TESTNET_SENDER_AFFINITY_LOAD_BALANCING_BLOCK) have no corresponding tests. Adding boundary tests for both forks on testnet, plus "never activates" tests for devnet and mainnet, would keep the suite consistent and provide a regression guard before the block is hit on live testnet. Fix this →

Comment update is accurate

The doc comment for LOCAL_SENDER_AFFINITY_LOAD_BALANCING_BLOCK correctly changed from "Real networks stay Never" to "Devnet and mainnet stay Never" now that testnet has a concrete block.

No devnet constants defined

Prior hardforks (e.g., DEVNET_LOAD_BALANCING_BLOCK) defined explicit devnet constants even when the devnet schedule was later set to Never. The new forks skip devnet constants entirely. This is a minor style inconsistency, not a bug — but worth noting if the convention is ever enforced by lint.


Verdict

No correctness issues. The change is safe to merge as-is. The main suggestion is adding boundary tests to match the existing test coverage pattern.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant