Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
### Breaking Changes
### Upcoming Breaking Changes
### Additions and Improvements
- EIP-7825: Transaction gas limit cap [#8700](https://github.com/hyperledger/besu/pull/8700)
#### Fusaka Devnet
- EIP-7825 - Transaction gas limit cap [#8700](https://github.com/hyperledger/besu/pull/8700)
- EIP-7823 - Modexp upper bounds [#8632](https://github.com/hyperledger/besu/pull/8632)

### Bug fixes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.hyperledger.besu.evm.precompile.MainnetPrecompiledContracts.populateForFrontier;
import static org.hyperledger.besu.evm.precompile.MainnetPrecompiledContracts.populateForFutureEIPs;
import static org.hyperledger.besu.evm.precompile.MainnetPrecompiledContracts.populateForIstanbul;
import static org.hyperledger.besu.evm.precompile.MainnetPrecompiledContracts.populateForOsaka;
import static org.hyperledger.besu.evm.precompile.MainnetPrecompiledContracts.populateForPrague;

import org.hyperledger.besu.evm.precompile.PrecompileContractRegistry;
Expand Down Expand Up @@ -61,6 +62,13 @@ static PrecompileContractRegistry prague(
return registry;
}

static PrecompileContractRegistry osaka(
final PrecompiledContractConfiguration precompiledContractConfiguration) {
final PrecompileContractRegistry registry = new PrecompileContractRegistry();
populateForOsaka(registry, precompiledContractConfiguration.getGasCalculator());
return registry;
}

static PrecompileContractRegistry futureEips(
final PrecompiledContractConfiguration precompiledContractConfiguration) {
final PrecompileContractRegistry registry = new PrecompileContractRegistry();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,7 @@ static ProtocolSpecBuilder osakaDefinition(
Set.of(BlobType.KZG_CELL_PROOFS),
evm.getMaxInitcodeSize()))
.transactionPoolPreProcessor(new OsakaTransactionPoolPreProcessor())
.precompileContractRegistryBuilder(MainnetPrecompiledContractRegistries::osaka)
.name("Osaka");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public void runBenchmark(
"000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000030000000000000000000000000000000000000000000000000000000000000400c5a1611f8be90071a43db23cc2fe01871cc4c0e8ab5743f6378e4fef77f7f6db0095c0727e20225beb665645403453e325ad5f9aeb9ba99bf3c148f63f9c07cf4fe8847ad5242d6b7d4499f93bd47056ddab8f7dee878fc2314f344dbee2a7c41a5d3db91eff372c730c2fdd3a141a4b61999e36d549b9870cf2f4e632c4d5df5f024f81c028000073a0ed8847cfb0593d36a47142f578f05ccbe28c0c06aeb1b1da027794c48db880278f79ba78ae64eedfea3c07d10e0562668d839749dc95f40467d15cf65b9cfc52c7c4bcef1cda3596dd52631aac942f146c7cebd46065131699ce8385b0db1874336747ee020a5698a3d1a1082665721e769567f579830f9d259cec1a836845109c21cf6b25da572512bf3c42fd4b96e43895589042ab60dd41f497db96aec102087fe784165bb45f942859268fd2ff6c012d9d00c02ba83eace047cc5f7b2c392c2955c58a49f0338d6fc58749c9db2155522ac17914ec216ad87f12e0ee95574613942fa615898c4d9e8a3be68cd6afa4e7a003dedbdf8edfee31162b174f965b20ae752ad89c967b3068b6f722c16b354456ba8e280f987c08e0a52d40a2e8f3a59b94d590aeef01879eb7a90b3ee7d772c839c85519cbeaddc0c193ec4874a463b53fcaea3271d80ebfb39b33489365fc039ae549a17a9ff898eea2f4cb27b8dbee4c17b998438575b2b8d107e4a0d66ba7fca85b41a58a8d51f191a35c856dfbe8aef2b00048a694bbccff832d23c8ca7a7ff0b6c0b3011d00b97c86c0628444d267c951d9e4fb8f83e154b8f74fb51aa16535e498235c5597dac9606ed0be3173a3836baa4e7d756ffe1e2879b415d3846bccd538c05b847785699aefde3e305decb600cd8fb0e7d8de5efc26971a6ad4e6d7a2d91474f1023a0ac4b78dc937da0ce607a45974d2cac1c33a2631ff7fe6144a3b2e5cf98b531a9627dea92c1dc82204d09db0439b6a11dd64b484e1263aa45fd9539b6020b55e3baece3986a8bffc1003406348f5c61265099ed43a766ee4f93f5f9c5abbc32a0fd3ac2b35b87f9ec26037d88275bd7dd0a54474995ee34ed3727f3f97c48db544b1980193a4b76a8a3ddab3591ce527f16d91882e67f0103b5cda53f7da54d489fc4ac08b6ab358a5a04aa9daa16219d50bd672a7cb804ed769d218807544e5993f1c27427104b349906a0b654df0bf69328afd3013fbe430155339c39f236df5557bf92f1ded7ff609a8502f49064ec3d1dbfb6c15d3a4c11a4f8acd12278cbf68acd5709463d12e3338a6eddb8c112f199645e23154a8e60879d2a654e3ed9296aa28f134168619691cd2c6b9e2eba4438381676173fc63c2588a3c5910dc149cf3760f0aa9fa9c3f5faa9162b0bf1aac9dd32b706a60ef53cbdb394b6b40222b5bc80eea82ba8958386672564cae3794f977871ab62337cf010001e30049201ec12937e7ce79d0f55d9c810e20acf52212aca1d3888949e0e4830aad88d804161230eb89d4d329cc83570fe257217d2119134048dd2ed167646975fc7d77136919a049ea74cf08ddd2b896890bb24a0ba18094a22baa351bf29ad96c66bbb1a598f2ca391749620e62d61c3561a7d3653ccc8892c7b99baaf76bf836e2991cb06d6bc0514568ff0d1ec8bb4b3d6984f5eaefb17d3ea2893722375d3ddb8e389a8eef7d7d198f8e687d6a513983df906099f9a2d23f4f9dec6f8ef2f11fc0a21fac45353b94e00486f5e17d386af42502d09db33cf0cf28310e049c07e88682aeeb00cb833c5174266e62407a57583f1f88b304b7c6e0c84bbe1c0fd423072d37a5bd0aacf764229e5c7cd02473460ba3645cd8e8ae144065bf02d0dd238593d8e230354f67e0b2f23012c23274f80e3ee31e35e2606a4a3f31d94ab755e6d163cff52cbb36b6d0cc67ffc512aeed1dce4d7a0d70ce82f2baba12e8d514dc92a056f994adfb17b5b9712bd5186f27a2fda1f7039c5df2c8587fdc62f5627580c13234b55be4df3056050e2d1ef3218f0dd66cb05265fe1acfb0989d8213f2c19d1735a7cf3fa65d88dad5af52dc2bba22b7abf46c3bc77b5091baab9e8f0ddc4d5e581037de91a9f8dcbc69309be29cc815cf19a20a7585b8b3073edf51fc9baeb3e509b97fa4ecfd621e0fd57bd61cac1b895c03248ff12bdbc57509250df3517e8a3fe1d776836b34ab352b973d932ef708b14f7418f9eceb1d87667e61e3e758649cb083f01b133d37ab2f5afa96d6c84bcacf4efc3851ad308c1e7d9113624fce29fab460ab9d2a48d92cdb281103a5250ad44cb2ff6e67ac670c02fdafb3e0f1353953d6d7d5646ca1568dea55275a050ec501b7c6250444f7219f1ba7521ba3b93d089727ca5f3bbe0d6c1300b423377004954c5628fdb65770b18ced5c9b23a4a5a6d6ef25fe01b4ce278de0bcc4ed86e28a0a68818ffa40970128cf2c38740e80037984428c1bd5113f40ff47512ee6f4e4d8f9b8e8e1b3040d2928d003bd1c1329dc885302fbce9fa81c23b4dc49c7c82d29b52957847898676c89aa5d32b5b0e1c0d5a2b79a19d67562f407f19425687971a957375879d90c5f57c857136c17106c9ab1b99d80e69c8c954ed386493368884b55c939b8d64d26f643e800c56f90c01079d7c534e3b2b7ae352cefd3016da55f6a85eb803b85e2304915fd2001f77c74e28746293c46e4f5f0fd49cf988aafd0026b8e7a3bab2da5cdce1ea26c2e29ec03f4807fac432662b2d6c060be1c7be0e5489de69d0a6e03a4b9117f9244b34a0f1ecba89884f781c6320412413a00c4980287409a2a78c2cd7e65cecebbe4ec1c28cac4dd95f6998e78fc6f1392384331c9436aa10e10e2bf8ad2c4eafbcf276aa7bae64b74428911b3269c749338b0fc5075ad"));

final BigIntegerModularExponentiationPrecompiledContract contract =
new BigIntegerModularExponentiationPrecompiledContract(gasCalculatorForFork(fork));
BigIntegerModularExponentiationPrecompiledContract.byzantium(gasCalculatorForFork(fork));

if (attemptNative != null
&& (!attemptNative
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ public static EVMExecutor osaka(
final BigInteger chainId, final EvmConfiguration evmConfiguration) {
final EVMExecutor executor = new EVMExecutor(MainnetEVMs.osaka(chainId, evmConfiguration));
executor.precompileContractRegistry =
MainnetPrecompiledContracts.prague(executor.evm.getGasCalculator());
MainnetPrecompiledContracts.osaka(executor.evm.getGasCalculator());
return executor;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

@lu-pinto lu-pinto May 21, 2025

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 and MainnetPrecompiledContracts::osaka. The duty to configure these things should be on the caller (configuration) not the instance itself.

Copy link
Member

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

Copy link
Contributor Author

@siladu siladu May 25, 2025

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).

Let me know what you think 9b34bf9

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.

Copy link
Member

@lu-pinto lu-pinto May 26, 2025

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?

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 and BigIntegerModularExponentiationPrecompiledContract with a corresponding static method.

The usage of static factory follows the design already in place for some precompiles e.g AltBN128AddPrecompiledContract.byzantium

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.

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.

Not sure I understand the example - that actually seems to prove my point. So you have all of the different parts constructed in MainnetProtocolSpecs and MainnetEVMs not in ProtocolSpec neither in EVM - these simply provide the constructor, not the specific static constructor for the fork, and MainnetProtocolSpecs and MainnetEVMs instantiate with the right parameters. If we had static factories for every fork in every class it would be a lot of boiler plate.

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).

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 😆

}

/** Disable native Arithmetic libraries. */
Expand Down Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to have a unit test for this

Copy link
Contributor Author

@siladu siladu May 25, 2025

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why using underscores in variable names

Copy link
Contributor Author

@siladu siladu May 25, 2025

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The 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];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ static void populateForByzantium(
final PrecompileContractRegistry registry, final GasCalculator gasCalculator) {
populateForFrontier(registry, gasCalculator);
registry.put(
Address.MODEXP, new BigIntegerModularExponentiationPrecompiledContract(gasCalculator));
Address.MODEXP,
BigIntegerModularExponentiationPrecompiledContract.byzantium(gasCalculator));
registry.put(Address.ALTBN128_ADD, AltBN128AddPrecompiledContract.byzantium(gasCalculator));
registry.put(Address.ALTBN128_MUL, AltBN128MulPrecompiledContract.byzantium(gasCalculator));
registry.put(
Expand Down Expand Up @@ -171,6 +172,33 @@ static void populateForPrague(
registry.put(Address.BLS12_MAP_FP2_TO_G2, new BLS12MapFp2ToG2PrecompiledContract());
}

/**
* Osaka precompile contract registry.
*
* @param gasCalculator the gas calculator
* @return the precompile contract registry
*/
static PrecompileContractRegistry osaka(final GasCalculator gasCalculator) {
PrecompileContractRegistry precompileContractRegistry = new PrecompileContractRegistry();
populateForOsaka(precompileContractRegistry, gasCalculator);
return precompileContractRegistry;
}

/**
* Populate registry for Osaka.
*
* @param registry the registry
* @param gasCalculator the gas calculator
*/
static void populateForOsaka(
final PrecompileContractRegistry registry, final GasCalculator gasCalculator) {
populateForPrague(registry, gasCalculator);

// EIP-7823 - Set upper bounds for MODEXP
registry.put(
Address.MODEXP, BigIntegerModularExponentiationPrecompiledContract.osaka(gasCalculator));
}

/**
* FutureEIPs precompile contract registry.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.util.Optional;

import com.google.common.annotations.VisibleForTesting;
import jakarta.validation.constraints.NotNull;
import org.apache.tuweni.bytes.Bytes;

Expand Down Expand Up @@ -108,6 +109,16 @@ public static PrecompileContractResult halt(
return new PrecompileContractResult(
output, false, MessageFrame.State.EXCEPTIONAL_HALT, haltReason);
}

@VisibleForTesting
boolean isSuccessful() {
return state.equals(MessageFrame.State.COMPLETED_SUCCESS);
}

@VisibleForTesting
Optional<ExceptionalHaltReason> getHaltReason() {
return haltReason;
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ private static void benchModExp() {
"000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000020e8e77626586f73b955364c7b4bbf0bb7f7685ebd40e852b164633a4acbd3244cfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01681d2220bfea4bb888a5543db8c0916274ddb1ea93b144c042c01d8164c95"))
.build();
final BigIntegerModularExponentiationPrecompiledContract contract =
new BigIntegerModularExponentiationPrecompiledContract(new OsakaGasCalculator());
BigIntegerModularExponentiationPrecompiledContract.osaka(new OsakaGasCalculator());

for (final Map.Entry<String, Bytes> testCase : testcases.entrySet()) {
final long timePerCallInNs = runBenchmark(testCase.getValue(), contract);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ class MODEXPPrecompiledContractTest {
@Mock private MessageFrame messageFrame;

private final BigIntegerModularExponentiationPrecompiledContract byzantiumContract =
new BigIntegerModularExponentiationPrecompiledContract(new ByzantiumGasCalculator());
BigIntegerModularExponentiationPrecompiledContract.byzantium(new ByzantiumGasCalculator());
private final BigIntegerModularExponentiationPrecompiledContract berlinContract =
new BigIntegerModularExponentiationPrecompiledContract(new BerlinGasCalculator());
BigIntegerModularExponentiationPrecompiledContract.byzantium(new BerlinGasCalculator());

MODEXPPrecompiledContractTest() {}

Expand Down
Loading
Loading