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 |
|
✅ Final review complete — no blockers (commit 2e06247) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #3462 +/- ##
===========================================
Coverage ? 87.12%
===========================================
Files ? 2625
Lines ? 321377
Branches ? 0
===========================================
Hits ? 279987
Misses ? 41390
Partials ? 0
🚀 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']
| while current_point <= last_step { | ||
| let base_amount = | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64())?; | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64(), platform_version)?; |
There was a problem hiding this comment.
CI is red before any test runs: this branch is not rustfmt-clean.
cargo fmt --check --all reports 44 unformatted hunks, all in files this PR touches (39 in distribution_function/evaluate.rs, 2 here in evaluate_interval.rs, 3 in rs-drive-abci/.../perpetual/block_based.rs). Nothing else in the workspace is dirty, so this is PR-introduced — the evaluate() signature change pushed these calls past 100 cols and cargo fmt --all was not re-run.
.github/workflows/tests-rs-workspace.yml runs cargo fmt --check --all as the Check formatting step, which is why the Rust workspace tests (macOS) job failed in 29s — the Rust test suite never executed on this PR at all. Everything else in this review was validated against a local run instead.
This line is one of the two production (non-test) offenders:
self.evaluate(distribution_start_step.to_u64(), current_point.to_u64(), platform_version)?;Fix is just cargo fmt --all.
🤖 Posted autonomously by Claude on behalf of pasta.
| "draft202012", | ||
| ], optional = true } | ||
| lazy_static = { version = "1.4" } | ||
| libm = "0.2" |
There was a problem hiding this comment.
Pin libm exactly — a caret range undermines the determinism guarantee this PR is built on.
libm = "0.2"The entire premise of distribution_function_evaluate_version: 1 is that libm produces bit-identical results everywhere. libm is not a correctly-rounded implementation, and its 0.2.x series has shipped accuracy/algorithm changes in patch releases. A caret range means the guarantee is only "same math" for builds that respect this repo's Cargo.lock — a cargo update, a regenerated lock, or any downstream crate resolving rs-dpp from crates.io can land on a different 0.2.x.
That matters because the result feeds a truncating cast (diff_exp as i128, intermediate.floor()), so a 1-ulp difference is a whole-token difference. Two validators at the same protocol version would then disagree on a claim amount — exactly the divergence this PR removes, but now invisible because both report the same version.
The manifest already pins consensus-critical deps this way two lines down (bincode = { version = "=2.0.1", ... }). Suggest libm = "=0.2.16" (the currently locked version) plus a comment explaining why.
Concrete evidence the last bit really is in play here — on aarch64-darwin, for the exact input in your new polynomial test:
std powf(125.0, 1.0/3.0) = 4.999999999999999 (bits 4013ffffffffffff) -> as i128 = 4
libm pow (125.0, 1.0/3.0) = 5.0 (bits 4014000000000000) -> as i128 = 5
🤖 Posted autonomously by Claude on behalf of pasta.
| 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.
The fix ships inert, and TOKEN_VERSIONS_V3 is an unreferenced pub const the compiler will never flag.
I grepped every pub const *_V<n> under packages/rs-platform-version/src: TOKEN_VERSIONS_V3 is the only version constant in the crate with no consumer. Every other *_VERSIONS_Vn is wired into some version/vN.rs or mocks/. PLATFORM_V12 (the latest) binds TOKEN_VERSIONS_V2, which carries distribution_function_evaluate_version: 0.
So after merge, every reachable PlatformVersion — including PlatformVersion::latest() — still takes the std arm. An aarch64-darwin node and an x86_64-glibc node evaluating the same Logarithmic claim can still disagree. The breaking (!) signature churn lands with zero behavior change.
The header comment documents this as intentional, so the ask is not "wire it up now" but: because it is pub in a pub mod, there is no dead-code warning and nothing anchors it. Two risks worth deciding on explicitly:
- If another
DPPTokenVersionsfield is added or bumped inv1.rs/v2.rsbefore the follow-up lands,v3.rsgets updated from memory — andPLATFORM_V13then activates a token version set whose other fields were frozen months earlier and never re-reviewed. - No test asserts that any real
PlatformVersionresolves to the libm path, so a follow-up that copy-pastesdistribution_function_evaluate_version: 0would keep the suite fully green while the fix silently never activates.
Cleanest option: drop v3.rs + pub mod v3; from this PR (it only needs the struct field and the 0 defaults), and add them in the PLATFORM_V13 PR that actually references the constant. If it stays, please add a test asserting PlatformVersion::latest().dpp.token_versions.distribution_function_evaluate_version == 1 in the activation PR.
🤖 Posted autonomously by Claude on behalf of pasta.
| start_from_moment_for_distribution, | ||
| max_cycle_moment, | ||
| None, | ||
| platform_version, |
There was a problem hiding this comment.
Activation will retroactively re-price already-emitted cycles, making payouts depend on when an identity claims.
rewards_in_interval walks from start_from_moment_for_distribution (the last-paid moment, arbitrarily far in the past) to max_cycle_moment and calls evaluate(..., platform_version) for every step — using the version active at claim time, not the version active at each cycle's own moment. The single &PlatformVersion reaching evaluate() makes per-moment gating impossible by construction.
Scenario at the distribution_function_evaluate_version 0→1 flip: identities X and Y hold identical Polynomial { m: 1, n: 3 } perpetual distributions that accrued entirely before the fork. X claims at height N-1 and is paid on the std path; Y claims at N+1 and every one of those same pre-fork cycles is recomputed with libm. Using the measured values above, that is 5 vs 4 per cycle — over a long unclaimed interval (bounded by max_token_redemption_cycles, and the step loop by MAX_DISTRIBUTION_CYCLES_PARAM = 32_767) the gap compounds.
This is not a chain split — all nodes at a given height agree — but realized emission for a fixed block range becomes a function of claim timing, and the token's pre-fork history is rewritten for anyone who had not yet claimed. If that is the accepted tradeoff, please say so in the PR description / activation PR so it is a recorded decision rather than an emergent one.
🤖 Posted autonomously by Claude on behalf of pasta.
| 1 => pow(diff as f64, exponent), | ||
| version => { | ||
| return Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "DistributionFunction::evaluate (Polynomial)".to_string(), |
There was a problem hiding this comment.
Four copies of this dispatch block mean known_versions: vec![0, 1] is hardcoded in four places — and the version check is not uniform across variants.
The same 14-line match platform_version.dpp.token_versions.distribution_function_evaluate_version { 0 => std, 1 => libm, version => Err(UnknownVersionMismatch { .. }) } appears verbatim at lines 228 (Polynomial), 346 (Exponential), 434 (Logarithmic) and 586 (InvertedLogarithmic), differing only in the two function calls and the method: string.
Two concrete consequences:
- Partial-edit hazard. A future
distribution_function_evaluate_version: 2touching only the polynomial path gets written at line 238; the other three sites still sayvec![0, 1], so after activation every exponential/logarithmic token claim returnsUnknownVersionMismatchinstead of an amount — a class of tokens becomes unclaimable, and the omission is invisible in review because the four blocks look identical. The inverse miss (forgetting to add a2 =>arm to one variant) yields a silently mixed-version evaluator that still compiles. - Non-uniform rejection.
FixedAmount,Stepwise,Linear,StepDecreasingAmountandRandomnever read the field at all, and eachversion =>arm sits after that branch'sDivideByZero/Overflowguards. So an unrecognized version is a hard error for aPolynomialcontract, a silent success for aFixedAmountone, and for aPolynomialwithn == 0it surfaces asDivideByZerorather than the version error. Version-incompatibility becomes contract-shape-dependent instead of a clean "this binary does not understand this protocol version".
Both dissolve with one resolution at the top of evaluate() — e.g. a small struct FloatOps { pow: fn(f64, f64) -> f64, exp: fn(f64) -> f64, log: fn(f64) -> f64 } selected by a single match with a single error site — after which the arms just call ops.pow(..). That also matches the crate's usual shape of one version match at method entry.
🤖 Posted autonomously by Claude on behalf of pasta.
| max_value: *max_value, | ||
| } | ||
| .evaluate(0, start_moment)?; | ||
| .evaluate(0, start_moment, platform_version)?; |
There was a problem hiding this comment.
Registration-time validation and payout-time evaluation can straddle the activation boundary.
This call computes start_token_amount, which then drives the coherence checks a few lines below (start_token_amount == *max → InvalidTokenDistributionFunctionIncoherenceError). Pre-PR there was exactly one implementation, so those checks provably described the numbers emission would later produce. Now validation runs under the version active at registration and emission under the version active at claim.
Concretely: a contract registered under PLATFORM_V12 whose start_token_amount via powf is max_value - 1 passes the incoherence check; after activation libm::pow returns max_value, so the distribution is pinned at its cap from cycle 0 — precisely the degenerate state this validation exists to reject. It is now permanently on-chain in a state the validator would have refused, and there is no re-validation path that catches it.
The mirror case is worse: the accept/reject boundary for new contracts silently moves at the fork, with no version gate on the validation rule itself. Same at lines 447, 637, 815 and 979. Worth confirming this is understood and bounded (the window is narrow — it needs the value to land within one ulp of a clamp boundary), or gating the validation rule alongside the math.
🤖 Posted autonomously by Claude on behalf of pasta.
| 0 => argument.ln(), | ||
| 1 => log(argument), | ||
| version => { | ||
| return Err(ProtocolError::UnknownVersionMismatch { |
There was a problem hiding this comment.
Pre-existing, but directly under the new correctly-named "DistributionFunction::evaluate (Logarithmic)" string you just added: the whole Logarithmic branch reports its overflow errors as InvertedLogarithmic — at line 453 ("InvertedLogarithmic: evaluation overflow") and again at lines 473, 476 and 486. Since you are already touching these exact lines and adding an accurate method: label three lines above, worth correcting the four strings in the same pass so a logarithmic-distribution overflow does not get triaged against the wrong variant.
🤖 Posted autonomously by Claude on behalf of pasta.
| while current_point <= last_step { | ||
| let base_amount = | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64())?; | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64(), platform_version)?; |
There was a problem hiding this comment.
CI is hard-failing on formatting, so the Rust test suite (including the four new libm tests) has never run on CI for this PR. cargo fmt --check --all reports 44 unformatted hunks, all in the 3 files this PR touched (39 in evaluate.rs, 2 production lines here at 1653/1843, 3 in block_based.rs) — introduced by appending the new platform_version/PlatformVersion::latest() arguments past 100 cols. The "Rust workspace tests" job aborts at "Check formatting" before clippy or any test. Running cargo fmt --all and pushing fixes it.
🤖 Posted autonomously by Claude on behalf of pasta.
| "draft202012", | ||
| ], optional = true } | ||
| lazy_static = { version = "1.4" } | ||
| libm = "0.2" |
There was a problem hiding this comment.
libm = "0.2" should be pinned exactly (= "0.2.16"). The determinism guarantee of the v1 path is "every node runs the same libm code", but a caret range lets any rebuild with a refreshed lock (or a downstream consumer not using this repo's Cargo.lock) resolve a different 0.2.x, and libm has shipped accuracy changes to pow/exp/log in patch releases. A one-ulp difference flips the truncating as i128/as u64 casts and re-creates exactly the consensus divergence this PR exists to fix — now dependent on Cargo resolution instead of the OS. This manifest already pins bincode = "=2.0.1" two lines down for the same class of reason.
Notably, the boundary case the new polynomial test locks in is razor-thin: the exact value of 125^(f64(1/3)) is 4.99999999999999955…, so libm 0.2.16 returning exactly 5.0 is within half an ulp of returning 4.999999999999999 (what Apple/glibc pow return). Any future libm patch that moves pow by one ulp changes consensus results at version 1.
| libm = "0.2" | |
| libm = "=0.2.16" |
🤖 Posted autonomously by Claude on behalf of pasta.
| start_from_moment_for_distribution, | ||
| max_cycle_moment, | ||
| None, | ||
| platform_version, |
There was a problem hiding this comment.
Activation retroactively re-prices historical cycles. rewards_in_interval walks every cycle from the last-paid moment (bounded by max redemption cycles) and evaluates each with the platform version active at claim time, not at the cycle's own moment. At the v0→v1 flip, two identities with identical distributions and identical unclaimed pre-fork cycles get different totals depending solely on whether they claim one block before or after activation (e.g. a Polynomial{m:1,n:3} cycle paying 5 under std powf and 4 under libm, or vice versa). Realized emission for a fixed historical range stops being a function of the range.
This may be an acceptable trade-off (per-moment gating would require threading the moment→version mapping through here), but it changes already-accrued rewards and deserves an explicit decision in the PR description / activation plan rather than being implicit.
🤖 Posted autonomously by Claude on behalf of pasta.
| /// NOTE: Not yet wired to any `PlatformVersion::PLATFORM_V*`. This constant sets | ||
| /// `distribution_function_evaluate_version: 1` (deterministic libm reward math), but | ||
| /// activation is deferred to a follow-up `PLATFORM_V13` PR. Until then it has no consumer. | ||
| pub const TOKEN_VERSIONS_V3: DPPTokenVersions = DPPTokenVersions { |
There was a problem hiding this comment.
The fix ships inert, and nothing will catch the follow-up not landing. TOKEN_VERSIONS_V3 has no consumer — PLATFORM_V12 (latest) uses TOKEN_VERSIONS_V2 with distribution_function_evaluate_version: 0 — so after this breaking-API change merges, every reachable code path still runs the platform-dependent std math and the cross-platform divergence in the PR title remains fully live. That is documented here as intentional, but two consequences are worth addressing now:
- No test asserts that any real
PlatformVersionreaches version 1 (the four new tests hand-mutate a cloned version). If the PLATFORM_V13 PR copy-pastesdistribution_function_evaluate_version: 0, the suite stays green and the fix silently never activates. - This is the only unreferenced
*_VERSIONS_Vnconstant in the crate; its other five fields are frozen now and won't be re-reviewed when V13 finally references it. Consider droppingv3.rsfrom this PR and introducing it in the activation PR instead — this PR only needs the new field plus the0defaults in v1/v2.
🤖 Posted autonomously by Claude on behalf of pasta.
| max_value: None, | ||
| }; | ||
|
|
||
| // cbrt(125) is exactly 5. The std f64 powf() on some platforms rounds |
There was a problem hiding this comment.
This comment is factually inverted, and the assertion is one ulp from flipping. With m: 1, n: 3, exponent is the f64 nearest 1/3 (strictly below it), so the exact value of 125^exponent is 4.99999999999999955… — below 5. A correctly-rounded pow (Apple, glibc) returns 4.999999999999999, which truncates to 4; the libm crate returning exactly 5.0 is the implementation-specific outlier this test locks in, not the "correct" cube root the comment describes. The test is still valid as a bit-determinism regression guard (libm is the same everywhere), but the comment should say that, otherwise the natural response to a future 4 != 5 failure (e.g. a libm patch bump) will be to "fix" the assertion and silently change consensus math.
Separately: none of the four new tests asserts what v0 and v1 produce for the same fixtures (e.g. the exact-value inverted-log fixtures in block_based.rs asserting 85171/78240/16094 under v0). Parameterizing a few existing fixtures over both versions would document the magnitude of the v12→v13 reward discontinuity before the fork instead of at it.
🤖 Posted autonomously by Claude on behalf of pasta.
| } | ||
|
|
||
| let diff_exp = (diff as f64).powf(exponent); | ||
| let diff_exp = match platform_version |
There was a problem hiding this comment.
Four copies of this dispatch, and the version check is non-uniform across variants. This 15-line match … { 0 => std, 1 => libm, v => Err(UnknownVersionMismatch) } block is repeated verbatim at the Exponential, Logarithmic, and InvertedLogarithmic arms, each with its own hardcoded known_versions: vec![0, 1]. Two concrete costs:
- A future
distribution_function_evaluate_version: 2that changes only one operation requires editing four blocks; missing one still compiles and yields a mixed-version evaluator — either a silent consensus split orUnknownVersionMismatchon three of the four curve types while the fourth works. FixedAmount/Stepwise/Linear/StepDecreasingAmount/Randomnever read the version, so an unknown version is a hard error for a Polynomial contract but silent success for a FixedAmount one.
Both collapse if the version is resolved once at the top of evaluate() into three fn pointers (pow/exp/ln) — one match, one error site, uniform rejection — or via small private versioned_pow/exp/ln(pv) helpers. Sibling token methods (token_config_update, token_set_price) already follow single-point dispatch on the same dpp.token_versions struct.
🤖 Posted autonomously by Claude on behalf of pasta.
| while current_point <= last_step { | ||
| let base_amount = | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64())?; | ||
| self.evaluate(distribution_start_step.to_u64(), current_point.to_u64(), platform_version)?; |
There was a problem hiding this comment.
CI is red at Check formatting, so the Rust test suite never ran on this PR. cargo fmt --check --all reports 44 unformatted hunks, all in the 3 Rust files this PR touches (39 in evaluate.rs, 2 production lines here at 1653/1843, 3 in block_based.rs) — the added platform_version / PlatformVersion::latest() arguments pushed lines past 100 cols. Because the macOS workspace job fails at the fmt step, clippy and every Rust test (including the four new libm-path tests) were skipped in CI; they currently pass only in local runs. cargo fmt --all and repush.
🤖 Posted autonomously by Claude on behalf of pasta.
| "draft202012", | ||
| ], optional = true } | ||
| lazy_static = { version = "1.4" } | ||
| libm = "0.2" |
There was a problem hiding this comment.
libm = "0.2" is a caret range on the crate whose bit-exact output is the entire premise of this fix. libm 0.2.x patch releases have changed pow/exp/log implementations; a regenerated lockfile (or any downstream consumer resolving a different 0.2.x) can shift results by 1 ulp, and the as i128/as u64 truncations in evaluate() turn a 1-ulp difference into a whole-token difference — the same consensus-divergence class this PR fixes, relocated from "OS libm" to "Cargo resolution". This manifest already pins bincode = "=2.0.1" two lines down for the same reason.
| libm = "0.2" | |
| libm = "=0.2.16" |
🤖 Posted autonomously by Claude on behalf of pasta.
| start_from_moment_for_distribution, | ||
| max_cycle_moment, | ||
| None, | ||
| platform_version, |
There was a problem hiding this comment.
Activation will retroactively re-price already-accrued cycles. rewards_in_interval walks every unclaimed cycle from the last-paid moment (bounded only by max redemption cycles) and evaluates each one with the platform version active at claim time, not at the cycle's own moment. When distribution_function_evaluate_version flips 0→1, two identities with identical pre-fork accrual get different totals depending on whether they claim before or after activation (e.g. the Polynomial{m:1,n:3} boundary case pays 5/cycle under libm where std paid 4). If that's acceptable, it's worth stating in the activation PR; if not, the version used per cycle would need to derive from the cycle's moment rather than the claim's.
🤖 Posted autonomously by Claude on behalf of pasta.
| /// NOTE: Not yet wired to any `PlatformVersion::PLATFORM_V*`. This constant sets | ||
| /// `distribution_function_evaluate_version: 1` (deterministic libm reward math), but | ||
| /// activation is deferred to a follow-up `PLATFORM_V13` PR. Until then it has no consumer. | ||
| pub const TOKEN_VERSIONS_V3: DPPTokenVersions = DPPTokenVersions { |
There was a problem hiding this comment.
The fix ships inert, and nothing guards its activation. TOKEN_VERSIONS_V3 has no consumer — LATEST_PLATFORM_VERSION is PLATFORM_V12, which uses TOKEN_VERSIONS_V2 (distribution_function_evaluate_version: 0) — so every reachable PlatformVersion still takes the std powf/exp/ln path and the cross-platform divergence in the PR title remains live after merge. That's documented as intentional, but two follow-ups are unguarded: (1) no test asserts that any real platform version reaches the libm path, so a PLATFORM_V13 that copy-pastes : 0 passes green and the fix silently never activates; (2) this is the only unreferenced *_VERSIONS_Vn const in the crate, and its other five fields can drift stale before V13 lands. Consider landing this file with the activation PR instead, or adding a test pinning PLATFORM_V13.dpp.token_versions.distribution_function_evaluate_version == 1 when it exists.
🤖 Posted autonomously by Claude on behalf of pasta.
| } | ||
|
|
||
| let diff_exp = (diff as f64).powf(exponent); | ||
| let diff_exp = match platform_version |
There was a problem hiding this comment.
This 15-line version-dispatch block is copy-pasted 4× (here, :346, :435, :587), and the check is non-uniform across variants. Consequences: a future version 2 requires editing four blocks and four known_versions: vec![0, 1] literals — miss one and a single distribution shape keeps old math on the new protocol version (compiles fine, splits consensus); and FixedAmount/Stepwise/Linear/StepDecreasingAmount/Random never read the version at all, so an unknown version errors for a Polynomial claim but silently succeeds for a FixedAmount one. Resolving once at the top of evaluate() — e.g. a small FloatOps { pow, exp, ln } selected by a single match (or versioned_pow/versioned_exp/versioned_ln helpers) — gives one error site, uniform rejection, and one place to add version 2. This would also match the crate's usual pattern of dispatching once at method entry (cf. token_config_update_transition/v0_methods.rs).
🤖 Posted autonomously by Claude on behalf of pasta.
| max_value: None, | ||
| }; | ||
|
|
||
| // cbrt(125) is exactly 5. The std f64 powf() on some platforms rounds |
There was a problem hiding this comment.
This comment has the rounding story backwards, and the assertion sits one ulp from flipping. 1.0/3.0 rounds below 1/3, so the exact value of 125^fl(1/3) is 4.99999999999999955… — a correctly-rounded pow (glibc, Apple libm) returns 4.999999999999999 and truncates to 4. It's the libm crate's pow that returns the 1-ulp-high 5.0 making this assert pass; "std powf on some platforms rounds below 5.0" describes the mathematically-correct behavior, not a platform quirk. The determinism claim (libm = same bits everywhere) still holds, but (a) if a future libm release becomes correctly rounded this test fails as a mysterious 4 != 5 and the tempting "fix" erases the only regression guard on the consensus path, and (b) no test locks the v0 vs v1 divergence on existing fixtures (e.g. the exact-value inverted-log fixtures in block_based.rs), so the magnitude of the v12→v13 reward discontinuity is undocumented until fork day. Suggest rewording the comment and adding a v0-vs-v1 comparison over the existing fixture set.
🤖 Posted autonomously by Claude on behalf of pasta.
| max_value: *max_value, | ||
| } | ||
| .evaluate(0, start_moment)?; | ||
| .evaluate(0, start_moment, platform_version)?; |
There was a problem hiding this comment.
Registration-time validation and payout evaluation can disagree across the activation boundary. These coherence checks (e.g. rejecting a contract whose start_token_amount already sits at max_value) run once, at registration, with the version active then; payout runs forever with the version active at claim time. A contract validated pre-activation whose std-powf start value is one ulp below max_value can, post-activation, evaluate to exactly max_value under libm — the precise degenerate shape this check exists to reject, now permanently on-chain. Probably acceptable (the divergence window is 1 ulp), but worth a conscious decision in the activation PR rather than an accident.
🤖 Posted autonomously by Claude on behalf of pasta.
| max_value: *max_value, | ||
| } | ||
| .evaluate(0, start_moment)?; | ||
| .evaluate(0, start_moment, platform_version)?; |
There was a problem hiding this comment.
Registration-time validation and payout evaluation can disagree across the activation boundary. These coherence checks (e.g. rejecting a contract whose start_token_amount already sits at max_value) now evaluate with whatever version is active at registration. A contract registered pre-activation whose v0 (powf) start value is one ulp below max_value passes validation; post-activation, v1 (pow) can put the same contract exactly at max_value from cycle 0 — the degenerate shape this check exists to reject — and it is permanently on-chain with no re-validation. Probably acceptable (any version gate on evaluation implies this), but worth stating as a known consequence, since validation runs once and evaluation runs forever.
🤖 Posted autonomously by Claude on behalf of pasta.
| 1 => log(argument), | ||
| version => { | ||
| return Err(ProtocolError::UnknownVersionMismatch { | ||
| method: "DistributionFunction::evaluate (Logarithmic)".to_string(), |
There was a problem hiding this comment.
Nit, pre-existing but right below this correctly-labeled string: the Logarithmic arm's error messages at :454, :473, :478, :486, :492, :499 all say "InvertedLogarithmic: …" (copy-paste from the real InvertedLogarithmic arm at :587+). Since this PR is already touching this arm, worth fixing the labels so version-mismatch and overflow errors from the two branches are distinguishable in logs.
🤖 Posted autonomously by Claude on behalf of pasta.
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