feat(vm): implement TIP-7883 ModExp gas cost increase#6654
feat(vm): implement TIP-7883 ModExp gas cost increase#6654yanghang8612 wants to merge 3 commits intotronprotocol:developfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <[email protected]>
yanghang8612
force-pushed
the
implement-tip-7883
branch
from
be82ead to
f13e430
Compare
| BigInteger energy = BigInteger.valueOf(multComplexity) | ||
| .multiply(BigInteger.valueOf(iterCount)); | ||
|
|
||
| BigInteger minEnergy = BigInteger.valueOf(500); | ||
| if (isLessThan(energy, minEnergy)) { | ||
| return 500L; | ||
| } | ||
|
|
||
| return isLessThan(energy, BigInteger.valueOf(Long.MAX_VALUE)) ? energy.longValueExact() | ||
| : Long.MAX_VALUE; |
There was a problem hiding this comment.
Good defensive coding — using BigInteger for the intermediate multiplication avoids any potential overflow with large baseLen/modLen values (up to 1024 bytes), and the Long.MAX_VALUE cap ensures the result is always safe to return as a long. Well handled.
| */ | ||
| private long getMultComplexityTIP7883(int baseLen, int modLen) { | ||
| long maxLength = max(baseLen, modLen, VMConfig.disableJavaLangMath()); | ||
| long words = (maxLength + 7) / 8; // ceil(maxLength / 8) |
There was a problem hiding this comment.
The inline comment // ceil(maxLength / 8) is a nice touch — it makes the intent of (maxLength + 7) / 8 immediately clear without needing to mentally decode the arithmetic. The structure also maps 1:1 to the spec's pseudocode, which makes auditing straightforward.
| if (maxLength <= 32) { | ||
| return 16; | ||
| } | ||
| return 2 * words * words; |
There was a problem hiding this comment.
[P1] Suggestion: use overflow-safe arithmetic for long-term review burden reduction
2 * words * words uses plain long multiplication. Proving it won't overflow requires tracing the full input chain: parseLen() → intValueSafe() returns int → max words = 268,435,456 → 2 * words² = 1.44×10¹⁷ < Long.MAX_VALUE. This reasoning is correct today, but not self-evident — if parseLen() return type or UPPER_BOUND ever changes, the safety assumption silently breaks.
Using overflow-safe arithmetic makes the code self-evidently safe with zero external reasoning required, reducing the review burden for future readers:
return Math.multiplyExact(2, Math.multiplyExact(words, words));This is a long-term maintainability suggestion, not a current correctness issue.
There was a problem hiding this comment.
Thanks for flagging this. Applied in e512e21 — used StrictMathWrapper.multiplyExact instead of Math.multiplyExact to stay consistent with the cross-platform-determinism convention this repo follows (the Maths javadoc explicitly redirects new code to StrictMathWrapper). Same fail-fast guarantee, and reviewers no longer need to trace parseLen() → int → UPPER_BOUND to convince themselves the multiplication is safe.
| * Minimal complexity of 16; doubled complexity for base/modulus > 32 bytes. | ||
| */ | ||
| private long getMultComplexityTIP7883(int baseLen, int modLen) { | ||
| long maxLength = max(baseLen, modLen, VMConfig.disableJavaLangMath()); |
There was a problem hiding this comment.
[SHOULD]getMultComplexityTIP7883 and getIterationCountTIP7883 are new methods with no legacy constraints. They should use StrictMathWrapper directly instead of the deprecated Maths.max(a, b, boolean) pattern.
Suggested changes:
// getMultComplexityTIP7883
- long maxLength = max(baseLen, modLen, VMConfig.disableJavaLangMath());
+ long maxLength = StrictMathWrapper.max(baseLen, modLen);
// avoid silent overflow on 2 * words * words
- return 2 * words * words;
+ return StrictMathWrapper.multiplyExact(2L, StrictMathWrapper.multiplyExact(words, words));
// getIterationCountTIP7883
- return max(iterCount, 1, VMConfig.disableJavaLangMath());
+ return StrictMathWrapper.max(iterCount, 1L);Reasons:
Mathsis@Deprecated— new code should not add more callers.2 * words * wordsrelies on manual reasoning about value ranges for overflow safety. UsingmultiplyExactmakes this explicit and fail-fast.
There was a problem hiding this comment.
Thanks, all three suggestions applied in e512e21 — getMultComplexityTIP7883 / getIterationCountTIP7883 now call StrictMathWrapper.max directly, and the 2 * words * words product is wrapped in StrictMathWrapper.multiplyExact so the overflow argument no longer depends on parseLen() returning int. Agreed that new code shouldn't keep adding callers to the deprecated Maths helper.
halibobo1205
assigned
halibobo1205,
CodeNinjaEvan and
xxo1shine and unassigned
halibobo1205,
CodeNinjaEvan and
xxo1shine
Proposals are now activated exclusively through the on-chain committee vote; the config.conf / CommonParameter / ConfigKey / Args plumbing for committee.allowTvmOsaka is removed, and DynamicPropertiesStore#getAllowTvmOsaka falls back to 0L when the store has no entry (matching the shape used by ALLOW_TVM_SELFDESTRUCT_RESTRICTION). No behavioural change for nodes that did not set the config knob, since its unset default was already 0. Going forward new proposal flags should not grow a corresponding config-file override - this change establishes that precedent by retiring the one Osaka had.
Maths is @deprecated, and the TIP-7883 helpers are new code with no legacy constraint, so switch them to StrictMathWrapper directly. Replacing the 2 * words * words product with multiplyExact also makes overflow safety self-evident instead of relying on parseLen() returning int.
5 participants
Summary
Implement TIP-7883 ModExp precompile repricing, gated behind the existing
allowTvmOsakaflag.Changes from the current pricing formula:
GQUAD_DIVISOR, tripling general cost2 * ceil(maxLen/8)²for >32 bytesRelated: tronprotocol/tips#837