-
Notifications
You must be signed in to change notification settings - Fork 958
EIP-7823 - Modexp upper bounds #8632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0249c2c
698c904
cd367bd
afdaed1
5b6f3b8
43f5ce4
a291317
7fd6c5d
0ac157f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,13 +52,42 @@ public class BigIntegerModularExponentiationPrecompiledContract | |
private static final int EXPONENT_LENGTH_OFFSET = 32; | ||
private static final int MODULUS_LENGTH_OFFSET = 64; | ||
|
||
private static final long BYZANTIUM_UPPER_BOUND = Long.MAX_VALUE; | ||
private static final long OSAKA_UPPER_BOUND = 1024L; | ||
private final long upperBound; | ||
|
||
/** | ||
* Instantiates a new BigInteger modular exponentiation precompiled contract. | ||
* | ||
* @param gasCalculator the gas calculator | ||
*/ | ||
public BigIntegerModularExponentiationPrecompiledContract(final GasCalculator gasCalculator) { | ||
private BigIntegerModularExponentiationPrecompiledContract( | ||
final GasCalculator gasCalculator, final long upperBound) { | ||
super("BigIntModExp", gasCalculator); | ||
this.upperBound = upperBound; | ||
} | ||
|
||
/** | ||
* Create the Byzantium BigIntegerModularExponentiationPrecompiledContract. | ||
* | ||
* @param gasCalculator the gas calculator | ||
* @return the BigIntegerModularExponentiationPrecompiledContract | ||
*/ | ||
public static BigIntegerModularExponentiationPrecompiledContract byzantium( | ||
final GasCalculator gasCalculator) { | ||
return new BigIntegerModularExponentiationPrecompiledContract( | ||
gasCalculator, BYZANTIUM_UPPER_BOUND); | ||
} | ||
|
||
/** | ||
* Create the Osaka BigIntegerModularExponentiationPrecompiledContract. Introduced in EIP-7823 | ||
* | ||
* @param gasCalculator the gas calculator | ||
* @return the BigIntegerModularExponentiationPrecompiledContract | ||
*/ | ||
public static BigIntegerModularExponentiationPrecompiledContract osaka( | ||
final GasCalculator gasCalculator) { | ||
return new BigIntegerModularExponentiationPrecompiledContract(gasCalculator, OSAKA_UPPER_BOUND); | ||
} | ||
|
||
/** Disable native Arithmetic libraries. */ | ||
|
@@ -99,24 +128,35 @@ public long gasRequirement(final Bytes input) { | |
@Override | ||
public PrecompileContractResult computePrecompile( | ||
final Bytes input, @NotNull final MessageFrame messageFrame) { | ||
// https://eips.ethereum.org/EIPS/eip-7823 | ||
// We introduce an upper bound to the inputs of the precompile, | ||
// each of the length inputs (length_of_BASE, length_of_EXPONENT and length_of_MODULUS) | ||
// MUST be less than or equal to 1024 bytes | ||
final long length_of_BASE = baseLength(input); | ||
final long length_of_EXPONENT = exponentLength(input); | ||
final long length_of_MODULUS = modulusLength(input); | ||
if (length_of_BASE > upperBound | ||
|| length_of_EXPONENT > upperBound | ||
|| length_of_MODULUS > upperBound) { | ||
return PrecompileContractResult.halt( | ||
null, Optional.of(ExceptionalHaltReason.PRECOMPILE_ERROR)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to have a unit test for this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, this was first draft for the devnet, planning to productionise next. Good reminder 👍 |
||
} | ||
if (useNative) { | ||
return computeNative(input); | ||
return computeNative(input, length_of_MODULUS); | ||
} else { | ||
return computeDefault(input); | ||
return computeDefault(input, length_of_BASE, length_of_EXPONENT, length_of_MODULUS); | ||
} | ||
} | ||
|
||
/** | ||
* Compute default precompile contract. | ||
* | ||
* @param input the input | ||
* @return the precompile contract result | ||
*/ | ||
@NotNull | ||
public PrecompileContractResult computeDefault(final Bytes input) { | ||
final int baseLength = clampedToInt(baseLength(input)); | ||
final int exponentLength = clampedToInt(exponentLength(input)); | ||
final int modulusLength = clampedToInt(modulusLength(input)); | ||
private PrecompileContractResult computeDefault( | ||
final Bytes input, | ||
final long length_of_BASE, | ||
final long length_of_EXPONENT, | ||
final long length_of_MODULUS) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why using underscores in variable names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to match the spec - controversial in java conventions perhaps, but I find it's really useful if the spec and code values match for future reference. |
||
final int baseLength = clampedToInt(length_of_BASE); | ||
final int exponentLength = clampedToInt(length_of_EXPONENT); | ||
final int modulusLength = clampedToInt(length_of_MODULUS); | ||
// If baseLength and modulusLength are zero | ||
// we could have a massively overflowing exp because it wouldn't have been filtered out at the | ||
// gas cost phase | ||
|
@@ -241,14 +281,9 @@ private static long square(final long n) { | |
return clampedMultiply(n, n); | ||
} | ||
|
||
/** | ||
* Compute native precompile contract. | ||
* | ||
* @param input the input | ||
* @return the precompile contract result | ||
*/ | ||
public PrecompileContractResult computeNative(final @NotNull Bytes input) { | ||
final int modulusLength = clampedToInt(modulusLength(input)); | ||
private PrecompileContractResult computeNative( | ||
final @NotNull Bytes input, final long length_of_MODULUS) { | ||
final int modulusLength = clampedToInt(length_of_MODULUS); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
final IntByReference o_len = new IntByReference(modulusLength); | ||
|
||
final byte[] result = new byte[modulusLength]; | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an usual design, not consistent with our code - IMO this static logic does not belong here but can be accessed through
MainnetPrecompiledContracts::byzantium
andMainnetPrecompiledContracts::osaka
. The duty to configure these things should be on the caller (configuration) not the instance itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think 9b34bf9
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean static factory pattern is an unusual design or something specific to this?
The usage of static factory follows the design already in place for some precompiles e.g
AltBN128AddPrecompiledContract.byzantium
I think it's pretty conventional for the static factory to live with the class it's constructing and be responsible for configuring the "osaka" version of it.
If you look through MainnetProtocolSpecs, a lot of fork configuration is done by wiring in a fork-specific version of a class, which extends the previous one, e.g. GasCalculator.
The difference here is we wire in a static factory instead of a whole new version of the class, since it's only one value that is changed.
However, if there is a performance issue with the extra check for prague though, wiring in e.g. OsakaBigIntegerModularExponentiationPrecompiledContract is more code but might be easier to JIT compile (we can check profiling to confirm).
No objections in general, but I think my approach is more conventional, at least in Besu.
No configuration currently exists in MainnetPrecompiledContracts, it's just wiring. That's mostly the case for MainnetProtocolSpecs too, with some exceptions. Most config happens in the fork-specific classes I believe.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that it's unusual but it looks a bit weird like this IMO. So you have 2 places you have to modify if you want to add a new fork even if such fork only lowers the limits,
MainnetPrecompiledContracts
andBigIntegerModularExponentiationPrecompiledContract
with a corresponding static method.Right... I missed this case. I only looked at
populateForPrague
and they are all using constructors. I think the one of the problems is not being consistent, as there are 2 different approaches in parallel.Not sure I understand the example - that actually seems to prove my point. So you have all of the different parts constructed in
MainnetProtocolSpecs
andMainnetEVMs
not inProtocolSpec
neither inEVM
- these simply provide the constructor, not the specific static constructor for the fork, andMainnetProtocolSpecs
andMainnetEVMs
instantiate with the right parameters. If we had static factories for every fork in every class it would be a lot of boiler plate.Yes, I think you are referring to the size of the method. It impacts JIT compilation for sure. Though these are configutation methods - they are not hot - so performance is not really a concern here.
I also think that the statics here degrade the testability of
BigIntegerModularExponentiationPrecompiledContract
because now there's no way to inject dependencies into the class for testing since all constructors are private. The first temptation would be to package-private the constructor for testing which makes it even worse.On a separate segway, not sure why
MainnetPrecompiledContracts
is an interface since it's clearly a factory class 😆