Skip to content

Feature/sma 54 max factor upper bound is hardcoded#73

Merged
mrnkslv merged 10 commits into
release/nodectl/v0.4.0from
feature/sma-54-max-factor-upper-bound-is-hardcoded-v2
Apr 10, 2026
Merged

Feature/sma 54 max factor upper bound is hardcoded#73
mrnkslv merged 10 commits into
release/nodectl/v0.4.0from
feature/sma-54-max-factor-upper-bound-is-hardcoded-v2

Conversation

@mrnkslv
Copy link
Copy Markdown
Contributor

@mrnkslv mrnkslv commented Apr 8, 2026

Summary

Remove the hardcoded max_factor upper bound of 3.0 and read the actual limit from the network's config param 17 (max_stake_factor). Previously, nodectl rejected any max_factor above 3.0 even on networks (e.g. testnet) that allow up to 30.0.

Changes

Centralised conversion helpers

-ton_utils: MAX_STAKE_FACTOR_SCALE, max_stake_factor_raw_to_multiplier(), extract_max_factor(ConfigParamEnum)
-commands/utils: fetch_network_max_factor() — get_config_param(17) + extract_max_factor.

Redundant methods removed

-client_json_rpc: remove network_max_stake_factor_raw() and network_max_stake_factor_multiplier() — callers now use get_config_param(17) directly with the conversion helpers above.

Validation

-ElectionsConfig::validate(Option<f32>): accept an optional upper bound.
None (offline, e.g. AppConfig::load): only checks max_factor >= 1.0 — no upper cap, so configs written for high-limit networks still load.
Some(m) (service startup / reload): enforces max_factor ∈ [1.0, m] using the live network value.
-config elections max-factor CLI: loads config first, checks elections are configured, then fetches param 17 via try_create_rpc_client (without vault) and validates against the network limit.
-config wallet stake --max-factor CLI: same RPC-based validation before submitting the stake.
-RuntimeConfigStore (service): new validate_max_factor() helper validates on initialize() and reload(); on RPC failure logs a warning and falls back to validate(None) instead of crashing.

Elections runner

-runner: single calc_max_factor(network_max_stake_factor_raw, warn_if_clamped) -> (u32, f32): computes the Elector fixed-point max_factor and the float multiplier; clamps the configured value to the network cap from config param 17 (same semantics as before, using max_stake_factor_raw_to_multiplier for the float form). tracing::warn! when the value is clamped runs only when warn_if_clamped is true (e.g. in participate), so snapshot updates do not emit the same warning every tick.

Tests

-test_elections_validate_max_factor_respects_network_cap — Some(3.0) rejects 5.0, Some(5.0) accepts it.
-test_elections_validate_none_allows_max_factor_above_default_cap — None accepts 25.0, Some(3.0) rejects, Some(30.0) accepts.
-Timing validation tests (sleep_period_pct, waiting_period_pct) updated to use validate(None).

Documentation

-README: updated config elections max-factor, elections config section, and config wallet stake table to reflect that the upper bound comes from config param 17, not a hardcoded 3.0.

Closes SMA-54

Copilot AI review requested due to automatic review settings April 8, 2026 14:24
@linear
Copy link
Copy Markdown

linear Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the hardcoded maximum stake factor limit of 3.0 and instead reads the actual network limit from config param 17 (max_stake_factor). This allows validators on networks with different limits (e.g., testnet with 30.0) to properly configure their max stake factors.

Changes:

  • Added utility functions to convert raw fixed-point stake factors to float multipliers
  • Added RPC methods to fetch the network's max stake factor from config param 17
  • Modified validation logic to accept optional upper bounds, allowing offline configs while enforcing network limits at startup
  • Updated CLI commands (config elections max-factor, config wallet stake) to validate against live network values
  • Enhanced error handling and messaging throughout to reference config param 17

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ton_utils.rs Added MAX_STAKE_FACTOR_SCALE constant and conversion function
client_json_rpc.rs Added methods to fetch network max stake factor from RPC
app_config.rs Updated ElectionsConfig::validate to accept optional network upper bound
runtime_config.rs Added validation against network max on service startup/reload
runner.rs Updated stake validation to use network max, improved error messages
config_elections_cmd.rs Added RPC-based validation for max-factor CLI command
config_wallet_cmd.rs Added RPC-based validation for stake CLI command
README.md Updated documentation to reflect network-based limits

This pull request is ready to be approved.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mrnkslv mrnkslv changed the title fix: max factor load from config Feature/sma 54 max factor upper bound is hardcoded Apr 8, 2026
@mrnkslv mrnkslv requested review from ITBear, Keshoid and Lapo4kaKek April 8, 2026 14:59
Comment thread src/node-control/elections/src/runner.rs Outdated
Comment thread src/node-control/elections/src/runner.rs Outdated
Comment thread src/node-control/elections/src/runner.rs Outdated
Comment thread src/node-control/common/src/ton_utils.rs Outdated
Comment thread src/node-control/common/src/app_config.rs Outdated
Comment thread src/node-control/common/src/app_config.rs
Comment thread src/node-control/ton-http-api-client/src/v2/client_json_rpc.rs Outdated
Comment thread src/node-control/commands/src/commands/nodectl/config_wallet_cmd.rs Outdated
Comment thread src/node-control/service/src/runtime_config.rs Outdated
Comment thread src/node-control/commands/src/commands/nodectl/config_elections_cmd.rs Outdated
Comment thread src/node-control/service/src/runtime_config.rs Outdated
@Keshoid
Copy link
Copy Markdown
Contributor

Keshoid commented Apr 8, 2026

The "Changes" section says:
config_params: add parse_config_param_16() and parse_config_param_17()

These already exist in control-client/src/config_params.rs and aren't part of this diff. The description should be corrected to avoid confusion during future archaeology.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/node-control/commands/src/commands/nodectl/config_elections_cmd.rs Outdated
Comment thread src/node-control/commands/src/commands/nodectl/utils.rs Outdated
Comment thread src/node-control/common/src/ton_utils.rs Outdated
Comment thread src/node-control/service/src/runtime_config.rs Outdated
Comment thread src/node-control/common/src/ton_utils.rs Outdated
@mrnkslv mrnkslv merged commit 28e6a60 into release/nodectl/v0.4.0 Apr 10, 2026
6 checks passed
@mrnkslv mrnkslv deleted the feature/sma-54-max-factor-upper-bound-is-hardcoded-v2 branch April 10, 2026 10:40
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.

3 participants