fix(proof): correct atomic-unit thresholds + mirror SetUint64 to prover#15
Open
DHEBP wants to merge 2 commits into
Open
fix(proof): correct atomic-unit thresholds + mirror SetUint64 to prover#15DHEBP wants to merge 2 commits into
DHEBP wants to merge 2 commits into
Conversation
…over Two related arithmetic-correctness fixes on the proof verification path, both follow-ups to PR DEROFDN#14. proof/proof_validation.go: the hard-cap and reasonable-amount thresholds were computed as if 1 DERO = 1,000,000 atomic units, but DERO actually uses 100,000 atomic units per DERO (matches globals/globals.go:295 and the GetInfo RPC supply conversion at rpc_dero_getinfo.go:81). The constants were therefore 10x too high — the validator's hard-cap check effectively triggered at ~220M DERO instead of the documented 22M. Reorder the const block so atomicUnitsPerDero is defined first, then rewrite each threshold as X * atomicUnitsPerDero so the conversion factor is explicit at every use site. Same fix applied to the largeThreshold local variable inside DetectSuspiciousProofPatterns. proof/proof_validation_test.go: four test cases referenced the buggy constants by value (e.g., 21_000_000_000_000 // "21M DERO"). With the corrected constants those amounts now correspond to 210M DERO atomic units — outside the labelled intent. Update the four affected test amounts and tighten their labels. cryptography/crypto/proof_generate.go: mirror PR DEROFDN#14's verifier-side SetInt64(int64(amount)) -> SetUint64(amount) hardening to the prover side. The previous pattern is bounded in legitimate use (transfer amounts and balances cap well below 2^63) and only causes self-DoS if it ever fires, but the asymmetry between prover and verifier is forensic noise once readers compare them. Drop the misleading "this should be reduced" comments — values at this scale (bounded by 2^64) are far smaller than the bn256 group order (~2^254), so modular reduction would be a no-op. No behavioral change for legitimate transfers. The validator's hard-cap rejection now actually fires at 22M DERO as the comments claim, rather than at 220M.
Warning 4 used a hardcoded 1 trillion atomic literal, which at the correct 100k atomic/DERO conversion flagged only multiples of 10M DERO (just 10M and 20M in the valid range). Restore the original intent of flagging exact multiples of 1M DERO via 1_000_000 * atomicUnitsPerDero. Note: DetectSuspiciousProofPatterns has zero callers in community-dev so this has no user-visible effect in this tree today, but it aligns the internal arithmetic with the corrected constants and matches the matching change in HOLOGRAM (DHEBP/HOLOGRAM @ a8902d9).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related arithmetic-correctness fixes on the proof-validation path, both follow-ups to PR #14.
proof/proof_validation.go: the hard-cap and reasonable-amount thresholds assumed1 DERO = 1,000,000 atomic units, but DERO uses100,000 atomic units per DERO(matchesglobals/globals.go:295and the GetInfo RPC supply conversion atrpc_dero_getinfo.go:81). The threshold constants were therefore 10× too high — the hard-cap check effectively triggered at ~220M DERO instead of the documented 22M. Reorder the const block soatomicUnitsPerDerois defined first, then rewrite each threshold asX * atomicUnitsPerDeroso the conversion factor is explicit at every use site. Same correction applied to thelargeThresholdlocal variable inDetectSuspiciousProofPatterns.proof/proof_validation_test.go: four test cases referenced the buggy constants by value. After the constant correction, those amounts would correspond to 210M / 230M / 500M DERO — outside the labelled intent. Update the four affected test amounts and tighten the labels.cryptography/crypto/proof_generate.go: mirror PR feat(explorer): proof validation security + modern UI refresh #14's verifier-sideSetInt64(int64(amount)) → SetUint64(amount)hardening to the prover side. The previous pattern is bounded in legitimate use (transfer amounts and balances cap well below2^63) and only causes self-DoS if it ever fires, but leaving the asymmetric pattern in place is forensic-screenshot bait once readers compare verifier and prover. Drop the misleading// this should be reducedcomments — at this scale (values bounded by2^64) the bn256 group order (~2^254) makes modular reduction a no-op.DetectSuspiciousProofPatterns: the hardcoded1_000_000_000_000literal flagged exact multiples of 10M DERO under the correct conversion. Restore the original intent — exact multiples of 1M DERO — via1_000_000 * atomicUnitsPerDero.Background
atomicUnitsPerDero = 100_000is correct (confirmed against the canonical decimals constant). Where PR #14 went off-by-10× was in the threshold constants themselves:21_000_000DERO ×100_000atomic/DERO =2_100_000_000_000atomic. The constant had21_000_000_000_000— 10× too high.maxReasonableAmountAtomic,currentSupplyApproxAtomic, and the locallargeThreshold/ Warning 4 literal.Practical exploitability: none as a chain-attack. The primary gate that rejects fabricated impossible amounts is the cryptographic commitment match at
proof/proof.go:99. The validator atproof/proof.go:54is an early-rejection shortcut, not a defense layer. The off-by-10× error meant the shortcut let through fabricated proofs with displayed amounts up to ~220M DERO (which the crypto path rejected on the commitment-match check), instead of cutting them at 22M as the comments and error messages claimed.But the constants disagreed with their own labels and the test labels by 10×, which produces misleading rejection messages ("amount X DERO exceeds DERO hard cap (21M)" computed from a threshold that's actually 210M). This PR aligns the math with the labels and tightens the prover/verifier symmetry.
Risk
Minimal. The validator's behavior changes only in the band of amounts between the old buggy threshold (~220M DERO atomic) and the corrected threshold (22M DERO atomic). No legitimate transfer can fall in that band (the on-chain hard cap is 21M DERO), so the only inputs affected are intentionally fabricated proof strings — which is what the validator exists to reject.
The prover-side
SetUint64change has no behavioral effect for legitimate amounts (which are far below2^63); it makes prover signed-overflow behavior consistent with the post-PR-#14 verifier.The Warning 4 change affects only the (currently unwired)
DetectSuspiciousProofPatternsfunction — no user-visible effect in community-dev. Included for arithmetic consistency and to align with the matching change in DHEBP/HOLOGRAM @ a8902d9.Test plan
go build ./proof/... ./cryptography/...— passesgo test ./proof/...— passes (all 11 existing wraparound + hard-cap test cases pass against the corrected constants and the updated 4 test amounts)go vet ./proof/...— clean (pre-existingunreachable codewarning inproof_innerproduct.go:309is unrelated and untouched by this PR)Notes
proof_validation.go) via DHEBP/HOLOGRAM @ da6f814 and @ a8902d9.DetectSuspiciousProofPatternsinto any display path — it remains unwired in community-dev. Either deleting it as dead code or wiring it into the explorer's display layer is a separate follow-up if desired.