-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enable execution processor on PoA networks with system contract addresses #10196
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
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 |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ | |
| import org.hyperledger.besu.ethereum.mainnet.blockhash.CancunPreExecutionProcessor; | ||
| import org.hyperledger.besu.ethereum.mainnet.blockhash.FrontierPreExecutionProcessor; | ||
| import org.hyperledger.besu.ethereum.mainnet.blockhash.PraguePreExecutionProcessor; | ||
| import org.hyperledger.besu.ethereum.mainnet.blockhash.PreExecutionProcessor; | ||
| import org.hyperledger.besu.ethereum.mainnet.feemarket.BaseFeeMarket; | ||
| import org.hyperledger.besu.ethereum.mainnet.feemarket.FeeMarket; | ||
| import org.hyperledger.besu.ethereum.mainnet.parallelization.MainnetParallelBlockProcessor; | ||
|
|
@@ -875,13 +876,28 @@ static ProtocolSpecBuilder cancunDefinition( | |
| evm.getMaxInitcodeSize())) | ||
| .precompileContractRegistryBuilder(MainnetPrecompiledContractRegistries::cancun) | ||
| .blockHeaderValidatorBuilder(MainnetBlockHeaderValidator::blobAwareBlockHeaderValidator) | ||
| .preExecutionProcessor( | ||
| isPoAConsensus(genesisConfigOptions) | ||
| ? new FrontierPreExecutionProcessor() | ||
| : new CancunPreExecutionProcessor()) | ||
| .preExecutionProcessor(getPreExecutionProcessor(genesisConfigOptions)) | ||
|
daniellehrner marked this conversation as resolved.
|
||
| .hardforkId(CANCUN); | ||
| } | ||
|
|
||
| private static PreExecutionProcessor getPreExecutionProcessor( | ||
| final GenesisConfigOptions genesisConfigOptions) { | ||
| if (isPoAConsensus(genesisConfigOptions) && !hasSystemContractAddresses(genesisConfigOptions)) { | ||
| return new FrontierPreExecutionProcessor(); | ||
| } | ||
|
|
||
| return new CancunPreExecutionProcessor(); | ||
| } | ||
|
daniellehrner marked this conversation as resolved.
|
||
|
|
||
| private static PreExecutionProcessor getPraguePreExecutionProcessor( | ||
| final GenesisConfigOptions genesisConfigOptions) { | ||
| if (isPoAConsensus(genesisConfigOptions) && !hasSystemContractAddresses(genesisConfigOptions)) { | ||
| return new FrontierPreExecutionProcessor(); | ||
| } | ||
|
|
||
| return new PraguePreExecutionProcessor(); | ||
| } | ||
|
|
||
| static ProtocolSpecBuilder pragueDefinition( | ||
| final Optional<BigInteger> chainId, | ||
| final boolean enableRevertReason, | ||
|
|
@@ -959,14 +975,11 @@ static ProtocolSpecBuilder pragueDefinition( | |
| new CodeDelegationService())) | ||
| .build()) | ||
| // EIP-2935 Blockhash processor | ||
| .preExecutionProcessor( | ||
| isPoAConsensus(genesisConfigOptions) | ||
| ? new FrontierPreExecutionProcessor() | ||
| : new PraguePreExecutionProcessor()) | ||
| .preExecutionProcessor(getPraguePreExecutionProcessor(genesisConfigOptions)) | ||
| .hardforkId(PRAGUE); | ||
| if (isPoAConsensus(genesisConfigOptions)) { | ||
| LOG.debug( | ||
| "Skipping system contract request processors for PoA consensus (clique/ibft/qbft)."); | ||
| if (isPoAConsensus(genesisConfigOptions) && !hasSystemContractAddresses(genesisConfigOptions)) { | ||
| LOG.warn( | ||
| "Skipping system contract request processors for PoA consensus (clique/ibft/qbft) without system contract addresses."); | ||
| pragueSpecBuilder.requestProcessorCoordinator(RequestProcessorCoordinator.noOp()); | ||
| } else { | ||
| try { | ||
|
|
@@ -990,6 +1003,13 @@ private static boolean isPoAConsensus(final GenesisConfigOptions genesisConfigOp | |
| || genesisConfigOptions.isQbft(); | ||
| } | ||
|
|
||
| private static boolean hasSystemContractAddresses( | ||
|
Contributor
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. It may seem a little surprising to the user that all system contracts must be specified for any of them to activate, as conceptually they are indepenent, but I think it's fine if it's clearly communicated whether they are active or not. I agree it does make sense to require specifying all of them, as that's the current use case on mainnet and apparently on the PoA networks that do use them. Also some of the existing code (like
Contributor
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.
I doubt, because that would break backwards compatibility. For the clients that used to support clique, in order to be compatible with the PoA networks, they had to implement the strictest rules. I.e. If some chain would disable some system contract, since Nethermind, for example, supports it, it would disable Geth nodes syncing to this chain. So it seems like all clients have to stick with the strictest validity rule amongst the client, that rule being "All system contracts have to be set"
Contributor
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. I meant addition of further system contracts in future forks might change this logic ("all system contracts have to be set"), exactly to preserve backwards compatibility.
Contributor
Author
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. I agree, I would leave it as is for now |
||
| final GenesisConfigOptions genesisConfigOptions) { | ||
| return genesisConfigOptions.getDepositContractAddress().isPresent() | ||
| && genesisConfigOptions.getWithdrawalRequestContractAddress().isPresent() | ||
| && genesisConfigOptions.getConsolidationRequestContractAddress().isPresent(); | ||
|
daniellehrner marked this conversation as resolved.
|
||
| } | ||
|
|
||
| static ProtocolSpecBuilder osakaDefinition( | ||
| final Optional<BigInteger> chainId, | ||
| final boolean enableRevertReason, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.