fix(dpp)!: version-gate distribution function floating-point evaluation#3462
fix(dpp)!: version-gate distribution function floating-point evaluation#3462PastaPastaPasta wants to merge 2 commits into
Conversation
DistributionFunction::evaluate() uses f64 transcendental functions (pow/exp/log) to compute consensus-critical token rewards. The std implementations are platform-dependent, risking consensus divergence between nodes with different architectures or libm versions. Gate these operations behind distribution_function_evaluate_version in DPPTokenVersions. Version 0 preserves the original std behavior (.powf/.exp/.ln) for existing protocol versions. Version 1+ uses deterministic libm functions for cross-platform consistency. Changes: - Add distribution_function_evaluate_version field to DPPTokenVersions - Create TOKEN_VERSIONS_V3 with deterministic evaluation enabled - Add libm 0.2 dependency to rs-dpp - Thread platform_version through evaluate() -> evaluate_interval() -> rewards_in_interval() call chain - Version-gate 4 transcendental call sites: Polynomial (pow), Exponential (exp), Logarithmic (log), InvertedLogarithmic (log) - Add determinism regression tests for all 4 affected variants
📝 WalkthroughWalkthroughThe PR enables platform-version-controlled selection of math implementations for token distribution evaluation. It adds libm 0.2 as a dependency, extends the platform version schema with a distribution_function_evaluate_version field, and modifies the evaluation call chain to route between standard f64 transcendental operations and deterministic libm equivalents based on protocol version. ChangesToken Distribution Math Versioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
|
Will review for 4.1 |
|
✅ Review complete (commit 2e06247) |
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR correctly version-gates four f64 transcendental call sites in DistributionFunction::evaluate behind a new distribution_function_evaluate_version field, with the new libm path activated only by TOKEN_VERSIONS_V3 (which is intentionally not yet wired to any PlatformVersion). The change is consensus-safe (v0 path preserved bit-for-bit) and threaded cleanly through evaluate_interval/rewards_in_interval. Main feedback is that the new version dispatch uses a wildcard fallback instead of the codebase's explicit-arms + UnknownVersionMismatch convention; plus minor test-comment and orphan-constant nits.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs:228-235: Wildcard `_ =>` dispatch diverges from the repo's fail-closed version pattern
All four new version gates (lines 228-235, 339-346, 421-428, 566-573) match `0 => std` and `_ => libm`, so any future `distribution_function_evaluate_version` value (2, 3, …) silently executes the v1 libm semantics instead of failing closed. Elsewhere in `rs-dpp` (e.g. `document/extended_document/v0/serialize.rs:130-133`, `document/serialization_traits/cbor_conversion/mod.rs:40-43`, `document/v0/serialize.rs:1457`) the codebase consistently enumerates known versions and ends with `version => Err(ProtocolError::UnknownVersionMismatch { method: ..., known_versions, received: version })`. Since this is consensus-critical reward math, a future protocol version bump that intends a third algorithm (e.g. fixed-point) but forgets to update this site would silently pay rewards under v1 semantics on this code path rather than producing a clean version-mismatch error. Tightening to explicit `1 => libm` + `version => Err(UnknownVersionMismatch …)` makes any future version change a typechecked code edit. Apply to all four call sites in this file.
| let diff_exp = match platform_version | ||
| .dpp | ||
| .token_versions | ||
| .distribution_function_evaluate_version | ||
| { | ||
| 0 => (diff as f64).powf(exponent), | ||
| _ => pow(diff as f64, exponent), | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Wildcard _ => dispatch diverges from the repo's fail-closed version pattern
All four new version gates (lines 228-235, 339-346, 421-428, 566-573) match 0 => std and _ => libm, so any future distribution_function_evaluate_version value (2, 3, …) silently executes the v1 libm semantics instead of failing closed. Elsewhere in rs-dpp (e.g. document/extended_document/v0/serialize.rs:130-133, document/serialization_traits/cbor_conversion/mod.rs:40-43, document/v0/serialize.rs:1457) the codebase consistently enumerates known versions and ends with version => Err(ProtocolError::UnknownVersionMismatch { method: ..., known_versions, received: version }). Since this is consensus-critical reward math, a future protocol version bump that intends a third algorithm (e.g. fixed-point) but forgets to update this site would silently pay rewards under v1 semantics on this code path rather than producing a clean version-mismatch error. Tightening to explicit 1 => libm + version => Err(UnknownVersionMismatch …) makes any future version change a typechecked code edit. Apply to all four call sites in this file.
source: ['claude', 'codex']
| use crate::version::dpp_versions::dpp_token_versions::DPPTokenVersions; | ||
|
|
||
| pub const TOKEN_VERSIONS_V3: DPPTokenVersions = DPPTokenVersions { | ||
| identity_token_info_default_structure_version: 0, | ||
| identity_token_status_default_structure_version: 0, | ||
| token_contract_info_default_structure_version: 0, | ||
| token_config_update_action_id_version: 1, | ||
| token_set_price_action_id_version: 1, | ||
| distribution_function_evaluate_version: 1, | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: TOKEN_VERSIONS_V3 has no PlatformVersion consumer
TOKEN_VERSIONS_V3 is the only constant that sets distribution_function_evaluate_version = 1, but no PlatformVersion::PLATFORM_V* references it (V1–V12 still bind TOKEN_VERSIONS_V1 or _V2). The PR description states activation is deferred to a follow-up PLATFORM_V13 PR, which is fine, but the protective effect is entirely deferred. Recommend ensuring a tracking issue exists for the activation PR (so the orphaned constant isn't forgotten) and considering a brief module-level doc comment in v3.rs pointing readers to the follow-up so future maintainers don't think it's wired up.
source: ['claude', 'codex']
| fn test_inverted_logarithmic_deterministic_libm_path() { | ||
| // f(x) = 10 * ln(100 / (1 * x)) / 1 + 5 | ||
| // At x=1 (with o=1, so arg = 100/1 = 100): ln(100) ≈ 4.605, * 10 = 46.05 + 5 = 51 |
There was a problem hiding this comment.
💬 Nitpick: Comment in inverted-log determinism test says x=1 but the call passes x=0
The comment reads At x=1 (with o=1, so arg = 100/1 = 100) but the test invokes distribution.evaluate(0, 0, &deterministic_version) (x=0). The numeric expectation 51 still holds because with start_moment=Some(0) and o=1, diff = 0 - 0 + 1 = 1 and argument = n/(m*diff) = 100/(1*1) = 100. Either update the comment to At x=0 or pass x=1 and recompute expected (which would give the same value here since start_moment=Some(0) is fixed and only diff depends on x). The assertion is correct; only the comment is misleading.
| fn test_inverted_logarithmic_deterministic_libm_path() { | |
| // f(x) = 10 * ln(100 / (1 * x)) / 1 + 5 | |
| // At x=1 (with o=1, so arg = 100/1 = 100): ln(100) ≈ 4.605, * 10 = 46.05 + 5 = 51 | |
| // f(x) = 10 * ln(100 / (1 * (x - s + o))) / 1 + 5 | |
| // At x=0 (with s=0, o=1, so arg = 100/(1*1) = 100): ln(100) ≈ 4.605, * 10 = 46.05 + 5 = 51 |
source: ['claude']
Replace wildcard libm dispatch with explicit version arms (0/1) ending in UnknownVersionMismatch across all four evaluate.rs call sites. Fix a misleading determinism-test comment and document that TOKEN_VERSIONS_V3 has no PlatformVersion consumer yet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
670-678: ⚡ Quick winPin the baseline assertions to an explicit evaluation version.
Most of this suite uses
PlatformVersion::latest(), so once a future PR pointslatest()atTOKEN_VERSIONS_V3, these tests will silently stop exercising v0 semantics and some expectations may flip without any change in this file. A small helper that cloneslatest()and forcesdistribution_function_evaluate_version = 0would keep the legacy-path coverage stable, while the version-1 determinism cases stay explicit.♻️ Example helper
mod tests { use super::*; use platform_version::version::PlatformVersion; use std::collections::BTreeMap; + + fn legacy_distribution_math_version() -> PlatformVersion { + let mut version = PlatformVersion::latest().clone(); + version + .dpp + .token_versions + .distribution_function_evaluate_version = 0; + version + }🤖 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 `@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs` around lines 670 - 678, The tests call DistributionFunction::evaluate(...) with PlatformVersion::latest(), which will change behavior when latest() advances; update the tests (e.g., test_fixed_amount and other evaluate tests) to pin the legacy evaluation path by cloning PlatformVersion::latest() into a mutable variable and setting distribution_function_evaluate_version = 0 before passing it to distribution.evaluate so the assertions remain stable; locate uses of PlatformVersion::latest() in the evaluate tests and replace them with the cloned-and-modified PlatformVersion instance referenced when calling DistributionFunction::evaluate.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (1)
1903-1920: ⚡ Quick winPin these explanation tests to a fixed math version too.
These assertions currently inherit whatever
PlatformVersion::latest()means at the time the test runs. Whenlatest()eventually switches to the deterministic token version, this suite will stop validating the legacy interval totals by default. Mirroring the explicit v0/v1 fixtures here would keep the call-chain coverage stable across future platform bumps.🤖 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 `@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs` around lines 1903 - 1920, The test uses PlatformVersion::latest() which makes it float as the platform evolves; change the call in test_fixed_amount_explanation_first_claim (and sibling explanation tests) to use an explicit legacy math platform version instead of latest() so the assertions remain fixed — replace PlatformVersion::latest() with a pinned PlatformVersion representing the legacy math (e.g., the v0/v1 fixture you use elsewhere such as PlatformVersion::v0() or PlatformVersion::new(0), depending on your API) when calling DistributionFunction::evaluate_interval_with_explanation to ensure deterministic behavior.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (1)
1825-1825: ⚡ Quick winPin the platform version in this exact-value test.
evaluate()is version-gated now, soPlatformVersion::latest()will make this assertion drift when a later protocol version flipsdistribution_function_evaluate_version. Prefer a fixed platform version here, or explicitly override just the distribution-function evaluation version used by the test.🤖 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 `@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs` at line 1825, The test calls dist.evaluate(0, 4, PlatformVersion::latest()), which will drift as distribution_function_evaluate_version changes; replace PlatformVersion::latest() with a pinned PlatformVersion instance (or construct a PlatformVersion and explicitly set distribution_function_evaluate_version to the expected version) so that evaluate() is invoked with a fixed protocol version; update the call site (the evaluate invocation) to pass that pinned/overridden PlatformVersion instead of PlatformVersion::latest().packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs (1)
2296-2335: ⚡ Quick winAvoid
PlatformVersion::latest()in exact numeric regression checks.These assertions now depend on version-gated math behavior, so they will become brittle as soon as
latest()starts consuming the deterministic evaluator. Pin the platform version used by the test, or construct a test-only version with the intendeddistribution_function_evaluate_version, so the expected constants stay stable.Also applies to: 2413-2416
🤖 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 `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs` around lines 2296 - 2335, The test uses PlatformVersion::latest() when calling InvertedLogarithmic::evaluate (and in other nearby assertions), which makes numeric expectations brittle; change those calls to a pinned PlatformVersion that encodes the deterministic distribution evaluation you expect (or build a test-only PlatformVersion with the intended distribution_function_evaluate_version) instead of PlatformVersion::latest(); update all evaluate invocations in this test (and the similar calls at the other assertions) to pass that pinned/versioned PlatformVersion so the InvertedLogarithmic::evaluate results remain stable for the asserted constants.
🤖 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.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs`:
- Around line 1903-1920: The test uses PlatformVersion::latest() which makes it
float as the platform evolves; change the call in
test_fixed_amount_explanation_first_claim (and sibling explanation tests) to use
an explicit legacy math platform version instead of latest() so the assertions
remain fixed — replace PlatformVersion::latest() with a pinned PlatformVersion
representing the legacy math (e.g., the v0/v1 fixture you use elsewhere such as
PlatformVersion::v0() or PlatformVersion::new(0), depending on your API) when
calling DistributionFunction::evaluate_interval_with_explanation to ensure
deterministic behavior.
In
`@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs`:
- Around line 670-678: The tests call DistributionFunction::evaluate(...) with
PlatformVersion::latest(), which will change behavior when latest() advances;
update the tests (e.g., test_fixed_amount and other evaluate tests) to pin the
legacy evaluation path by cloning PlatformVersion::latest() into a mutable
variable and setting distribution_function_evaluate_version = 0 before passing
it to distribution.evaluate so the assertions remain stable; locate uses of
PlatformVersion::latest() in the evaluate tests and replace them with the
cloned-and-modified PlatformVersion instance referenced when calling
DistributionFunction::evaluate.
In
`@packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs`:
- Line 1825: The test calls dist.evaluate(0, 4, PlatformVersion::latest()),
which will drift as distribution_function_evaluate_version changes; replace
PlatformVersion::latest() with a pinned PlatformVersion instance (or construct a
PlatformVersion and explicitly set distribution_function_evaluate_version to the
expected version) so that evaluate() is invoked with a fixed protocol version;
update the call site (the evaluate invocation) to pass that pinned/overridden
PlatformVersion instead of PlatformVersion::latest().
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rs`:
- Around line 2296-2335: The test uses PlatformVersion::latest() when calling
InvertedLogarithmic::evaluate (and in other nearby assertions), which makes
numeric expectations brittle; change those calls to a pinned PlatformVersion
that encodes the deterministic distribution evaluation you expect (or build a
test-only PlatformVersion with the intended
distribution_function_evaluate_version) instead of PlatformVersion::latest();
update all evaluate invocations in this test (and the similar calls at the other
assertions) to pass that pinned/versioned PlatformVersion so the
InvertedLogarithmic::evaluate results remain stable for the asserted constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b32316de-a0c3-4b69-b6a4-4cbc192dcab7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
packages/rs-dpp/Cargo.tomlpackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rspackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rspackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rspackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/evaluate_interval.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/distribution/perpetual/block_based.rspackages/rs-drive/src/state_transition_action/batch/batched_transition/token_transition/token_claim_transition_action/v0/transformer.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_token_versions/v3.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest commit 2e06247 resolves all three prior findings: the four version-dispatch sites in evaluate.rs now fail closed with ProtocolError::UnknownVersionMismatch (explicit 0/1 arms), v3.rs has a doc comment documenting the deferred PLATFORM_V13 activation, and the inverted-log test comment matches the actual call. One in-scope suggestion remains: the new error path has no regression test. The TOKEN_VERSIONS_V3-unwired nitpicks are intentionally staged per the PR description and not actionable here.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs:228-242: No regression test for the new UnknownVersionMismatch error path
The four new version-dispatch sites (Polynomial 228-242, Exponential 346-360, Logarithmic 435-449, InvertedLogarithmic 587-602) correctly fail closed with `ProtocolError::UnknownVersionMismatch` for any value other than 0 or 1, but the test suite only exercises the happy paths (`test_*_deterministic_libm_path` at version 1, plus the existing version-0 tests). A grep for `UnknownVersionMismatch` in this file matches only the four production sites — zero tests. Since this is consensus-critical reward math and the new behavior is specifically about rejecting unknown protocol versions, a future edit could reintroduce a wildcard arm at any of the four sites without breaking any test. Add at least one unit test that constructs a `PlatformVersion` with `distribution_function_evaluate_version = 2` (or similar) and asserts `Err(ProtocolError::UnknownVersionMismatch { .. })` for each transcendental variant, or a table-driven test across all four branches.
| let diff_exp = match platform_version | ||
| .dpp | ||
| .token_versions | ||
| .distribution_function_evaluate_version | ||
| { | ||
| 0 => (diff as f64).powf(exponent), | ||
| 1 => pow(diff as f64, exponent), | ||
| version => { | ||
| return Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "DistributionFunction::evaluate (Polynomial)".to_string(), | ||
| known_versions: vec![0, 1], | ||
| received: version, | ||
| }) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: No regression test for the new UnknownVersionMismatch error path
The four new version-dispatch sites (Polynomial 228-242, Exponential 346-360, Logarithmic 435-449, InvertedLogarithmic 587-602) correctly fail closed with ProtocolError::UnknownVersionMismatch for any value other than 0 or 1, but the test suite only exercises the happy paths (test_*_deterministic_libm_path at version 1, plus the existing version-0 tests). A grep for UnknownVersionMismatch in this file matches only the four production sites — zero tests. Since this is consensus-critical reward math and the new behavior is specifically about rejecting unknown protocol versions, a future edit could reintroduce a wildcard arm at any of the four sites without breaking any test. Add at least one unit test that constructs a PlatformVersion with distribution_function_evaluate_version = 2 (or similar) and asserts Err(ProtocolError::UnknownVersionMismatch { .. }) for each transcendental variant, or a table-driven test across all four branches.
source: ['codex']
3 participants
Issue being fixed or feature implemented
DistributionFunction::evaluate()uses f64 transcendental functions (powf/exp/ln) to compute consensus-critical token rewards. The std implementations are platform-dependent (varying across CPU architectures and libm versions), risking consensus divergence between nodes computing different integer rewards for the same claim.Concrete proof:
125^(1/3)yields 4 on some platforms (stdpowf) vs 5 on others. The deterministiclibmpath always returns 5.What was done?
Gate transcendental float operations behind
distribution_function_evaluate_versioninDPPTokenVersions:.powf(),.exp(),.ln())libmfunctions (pow,exp,log)Changes:
distribution_function_evaluate_versionfield toDPPTokenVersionsTOKEN_VERSIONS_V3with deterministic evaluation enabled (for use in a future protocol version)libm = "0.2"dependency to rs-dppplatform_versionthroughevaluate()→evaluate_interval()→rewards_in_interval()call chainpow), Exponential (exp), Logarithmic (log), InvertedLogarithmic (log)Note:
TOKEN_VERSIONS_V3is created but not yet assigned to a platform version. A future PR creatingPLATFORM_V13should reference it to activate deterministic evaluation on the network.How Has This Been Tested?
evaluate()unit tests pass (47 existing + 3 new determinism tests)validation.rstests passevaluate_intervaltests pass (withtoken-reward-explanationsfeature)distribution_function_evaluate_version = 1cargo check -p dpp -p drivecompiles cleanBreaking Changes
Function signature changes (compile-time only, no behavioral change for existing protocol versions):
DistributionFunction::evaluate()now requiresplatform_version: &PlatformVersionDistributionFunction::evaluate_interval()now requiresplatform_version: &PlatformVersionRewardDistributionType::rewards_in_interval()now requiresplatform_version: &PlatformVersionChecklist:
Summary by CodeRabbit
New Features
Improvements