Skip to content
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

Mining options refactor #6027

Merged
merged 25 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c57c2ac
Refactor mining options
fab-10 Oct 10, 2023
b408b64
Fix null pointer exception
fab-10 Oct 10, 2023
da79e09
fix another null pointer exception
fab-10 Oct 10, 2023
7684d7e
uncomment code
fab-10 Oct 10, 2023
78741da
Move miner options tests
fab-10 Oct 11, 2023
18bcce3
Unit test fixes
fab-10 Oct 12, 2023
59fbe8e
Removed the commented code
fab-10 Oct 12, 2023
8f14dd3
WIP
fab-10 Oct 13, 2023
2280e02
Merge branch 'main' into mining-options-refactor
fab-10 Oct 13, 2023
65bdabd
WIP
fab-10 Oct 13, 2023
e89a244
Remove not relevant for this feature
fab-10 Oct 16, 2023
20e498b
Merge branch 'main' into mining-options-refactor
fab-10 Oct 16, 2023
86b9a6e
Merge branch 'main' into mining-options-refactor
fab-10 Oct 17, 2023
2267bc7
Fix javadoc
fab-10 Oct 17, 2023
c6b56d2
Remove code not belonging to this PR
fab-10 Oct 17, 2023
477acf9
coinbase is an updatable parameter
fab-10 Oct 17, 2023
4626230
Move MiningOptions to upper package
fab-10 Oct 17, 2023
25b1522
Fix coinbase for *bft
fab-10 Oct 17, 2023
ae775ae
Merge branch 'main' into mining-options-refactor
fab-10 Oct 18, 2023
16315a4
Merge branch 'main' into mining-options-refactor
macfarla Oct 20, 2023
2700930
Merge branch 'mining-options-refactor' of github.com:fab-10/besu into…
fab-10 Oct 23, 2023
e55392b
Apply suggestions from code review
fab-10 Oct 23, 2023
d710e5c
Merge branch 'main' into mining-options-refactor
fab-10 Oct 24, 2023
639af82
Update besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
fab-10 Oct 24, 2023
232caa6
Merge branch 'mining-options-refactor' of github.com:fab-10/besu into…
fab-10 Oct 24, 2023
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
Prev Previous commit
Next Next commit
coinbase is an updatable parameter
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
  • Loading branch information
fab-10 committed Oct 17, 2023
commit 477acf93deeb5c3ab950fa1e27f37357645c2f8d
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.hyperledger.besu.ethereum.api.tls.FileBasedPasswordProvider;
import org.hyperledger.besu.ethereum.core.AddressHelpers;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.UpdatableInitValues;
import org.hyperledger.besu.ethereum.core.MiningParameters;
import org.hyperledger.besu.ethereum.core.PrivacyParameters;
import org.hyperledger.besu.ethereum.p2p.config.NetworkingConfiguration;
Expand All @@ -56,7 +57,10 @@ public class BesuNodeConfigurationBuilder {
private String name;
private Optional<Path> dataPath = Optional.empty();
private MiningParameters miningParameters =
ImmutableMiningParameters.builder().coinbase(AddressHelpers.ofValue(1)).build();
ImmutableMiningParameters.builder()
.updatableInitValues(
UpdatableInitValues.builder().coinbase(AddressHelpers.ofValue(1)).build())
.build();

private JsonRpcConfiguration jsonRpcConfiguration = JsonRpcConfiguration.createDefault();
private JsonRpcConfiguration engineRpcConfiguration = JsonRpcConfiguration.createEngineDefault();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,8 +310,8 @@ public BesuNode createNodeWithMultiTenantedPrivacy(
UpdatableInitValues.builder()
.isMiningEnabled(true)
.minTransactionGasPrice(Wei.ZERO)
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();

return create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ public void shouldMineOnSingleNodeWithFreeGas_Berlin() throws Exception {
UpdatableInitValues.builder()
.isMiningEnabled(true)
.minTransactionGasPrice(Wei.ZERO)
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();
minerNode.setMiningParameters(zeroGasMiningParams);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Wei;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.UpdatableInitValues;
import org.hyperledger.besu.ethereum.core.MiningParameters;

import java.util.List;
Expand Down Expand Up @@ -262,7 +263,7 @@ static MiningOptions fromConfig(final MiningParameters miningParameters) {
@Override
public MiningParameters toDomainObject() {
final var updatableInitValuesBuilder =
ImmutableMiningParameters.UpdatableInitValues.builder()
UpdatableInitValues.builder()
.isMiningEnabled(isMiningEnabled)
.extraData(extraData)
.minTransactionGasPrice(minTransactionGasPrice)
Expand All @@ -271,6 +272,9 @@ public MiningParameters toDomainObject() {
if (targetGasLimit != null) {
updatableInitValuesBuilder.targetGasLimit(targetGasLimit);
}
if (coinbase != null) {
updatableInitValuesBuilder.coinbase(coinbase);
}

final var miningParametersBuilder =
ImmutableMiningParameters.builder()
Expand All @@ -290,10 +294,6 @@ public MiningParameters toDomainObject() {
unstableOptions.posBlockCreationRepetitionMinDuration)
.build());

if (coinbase != null) {
miningParametersBuilder.coinbase(coinbase);
}

return miningParametersBuilder.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,8 @@ private MiningParameters getMiningParameters() {
.nonceGenerator(new IncrementingNonceGenerator(0))
.extraData(extraData)
.minTransactionGasPrice(minTransactionGasPrice)
.coinbase(coinbase)
.build())
.coinbase(coinbase)
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ protected MiningCoordinator createMiningCoordinator(

final Address localAddress = Util.publicKeyToAddress(nodeKey.getPublicKey());
final BftProtocolSchedule bftProtocolSchedule = (BftProtocolSchedule) protocolSchedule;
miningParameters.setCoinbase(localAddress);
Copy link
Contributor

@siladu siladu Oct 18, 2023

Choose a reason for hiding this comment

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

Why do we need to do this here now?

Also, surprised we do this here but not for QbftBesuControllerBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed since it is redundant the coinbase is already set in `BftBlockCreator' for all *bft

final BftBlockCreatorFactory<?> blockCreatorFactory =
new BftBlockCreatorFactory<>(
transactionPool,
Expand Down
11 changes: 9 additions & 2 deletions besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import org.hyperledger.besu.ethereum.api.jsonrpc.websocket.WebSocketConfiguration;
import org.hyperledger.besu.ethereum.api.tls.TlsConfiguration;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.UpdatableInitValues;
import org.hyperledger.besu.ethereum.core.MiningParameters;
import org.hyperledger.besu.ethereum.core.PrivacyParameters;
import org.hyperledger.besu.ethereum.eth.sync.SyncMode;
Expand Down Expand Up @@ -902,7 +903,10 @@ public void envVariableOverridesValueFromConfigFile() {
verify(mockControllerBuilder)
.miningParameters(
ImmutableMiningParameters.builder()
.coinbase(Address.fromHexString(expectedCoinbase))
.updatableInitValues(
UpdatableInitValues.builder()
.coinbase(Address.fromHexString(expectedCoinbase))
.build())
.build());
}

Expand All @@ -916,7 +920,10 @@ public void cliOptionOverridesEnvVariableAndConfig() {
verify(mockControllerBuilder)
.miningParameters(
ImmutableMiningParameters.builder()
.coinbase(Address.fromHexString(expectedCoinbase))
.updatableInitValues(
UpdatableInitValues.builder()
.coinbase(Address.fromHexString(expectedCoinbase))
.build())
.build());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,8 @@ protected MiningParameters createCustomizedDomainObject() {
.isMiningEnabled(true)
.extraData(Bytes.fromHexString("0xabc321"))
.minBlockOccupancyRatio(0.5)
.coinbase(Address.ZERO)
.build())
.coinbase(Address.ZERO)
.isStratumMiningEnabled(true)
.unstable(Unstable.builder().posBlockCreationMaxTime(1000).build())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.hyperledger.besu.consensus.common.EpochManager;
import org.hyperledger.besu.consensus.common.validator.ValidatorVote;
import org.hyperledger.besu.cryptoservices.NodeKey;
import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Hash;
import org.hyperledger.besu.ethereum.ProtocolContext;
import org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator;
Expand All @@ -49,7 +48,6 @@ public class CliqueBlockCreator extends AbstractBlockCreator {
* Instantiates a new Clique block creator.
*
* @param miningParameters the mining parameters
* @param coinbase the coinbase
* @param extraDataCalculator the extra data calculator
* @param transactionPool the pending transactions
* @param protocolContext the protocol context
Expand All @@ -60,7 +58,6 @@ public class CliqueBlockCreator extends AbstractBlockCreator {
*/
public CliqueBlockCreator(
final MiningParameters miningParameters,
final Address coinbase,
final ExtraDataCalculator extraDataCalculator,
final TransactionPool transactionPool,
final ProtocolContext protocolContext,
Expand All @@ -70,7 +67,6 @@ public CliqueBlockCreator(
final EpochManager epochManager) {
super(
miningParameters,
coinbase,
__ -> Util.publicKeyToAddress(nodeKey.getPublicKey()),
extraDataCalculator,
transactionPool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public CliqueMinerExecutor(
this.nodeKey = nodeKey;
this.localAddress = Util.publicKeyToAddress(nodeKey.getPublicKey());
this.epochManager = epochManager;
miningParams.setCoinbase(localAddress);
}

@Override
Expand All @@ -82,7 +83,6 @@ public CliqueBlockMiner createMiner(
(header) ->
new CliqueBlockCreator(
miningParameters,
localAddress, // TOOD(tmm): This can be removed (used for voting not coinbase).
this::calculateExtraData,
transactionPool,
protocolContext,
Expand All @@ -103,7 +103,7 @@ public CliqueBlockMiner createMiner(

@Override
public Optional<Address> getCoinbase() {
return Optional.of(localAddress);
return miningParameters.getCoinbase();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@ public void proposerAddressCanBeExtractFromAConstructedBlock() {
final CliqueBlockCreator blockCreator =
new CliqueBlockCreator(
miningParameters,
coinbase,
parent -> extraData,
createTransactionPool(),
protocolContext,
Expand Down Expand Up @@ -170,7 +169,6 @@ public void insertsValidVoteIntoConstructedBlock() {
final CliqueBlockCreator blockCreator =
new CliqueBlockCreator(
miningParameters,
coinbase,
parent -> extraData,
createTransactionPool(),
protocolContext,
Expand Down Expand Up @@ -204,7 +202,6 @@ public void insertsNoVoteWhenAtEpoch() {
final CliqueBlockCreator blockCreator =
new CliqueBlockCreator(
miningParameters,
coinbase,
parent -> extraData,
createTransactionPool(),
protocolContext,
Expand Down Expand Up @@ -252,8 +249,8 @@ private static MiningParameters createMiningParameters(
.extraData(extraData)
.targetGasLimit(10_000_000L)
.minTransactionGasPrice(Wei.ZERO)
.coinbase(coinbase)
.build())
.coinbase(coinbase)
.build();
return miningParameters;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ private static MiningParameters createMiningParameters(final Bytes vanityData) {
UpdatableInitValues.builder()
.extraData(vanityData)
.minTransactionGasPrice(Wei.ZERO)
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ public BftBlockCreator(
final BftExtraDataCodec bftExtraDataCodec) {
super(
miningParameters,
localAddress,
miningBeneficiaryCalculator(localAddress, forksSchedule),
extraDataCalculator,
transactionPool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,8 @@ private static ControllerAndState createControllerAndFinalState(
.isMiningEnabled(true)
.minTransactionGasPrice(Wei.ZERO)
.extraData(Bytes.wrap("Ibft Int tests".getBytes(UTF_8)))
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();

final StubGenesisConfigOptions genesisConfigOptions = new StubGenesisConfigOptions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ public BlockHeaderValidator.Builder createBlockHeaderRuleset(
0,
initialValidatorList)))
.minTransactionGasPrice(Wei.ZERO)
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();

final BftBlockCreator blockCreator =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ class MergeBlockCreator extends AbstractBlockCreator {
* @param transactionPool the pending transactions
* @param protocolContext the protocol context
* @param protocolSchedule the protocol schedule
* @param miningBeneficiary the mining beneficiary
* @param parentHeader the parent header
*/
public MergeBlockCreator(
Expand All @@ -53,13 +52,11 @@ public MergeBlockCreator(
final TransactionPool transactionPool,
final ProtocolContext protocolContext,
final ProtocolSchedule protocolSchedule,
final Address miningBeneficiary,
final BlockHeader parentHeader,
final Optional<Address> depositContractAddress) {
super(
miningParameters,
miningBeneficiary,
__ -> miningBeneficiary,
__ -> miningParameters.getCoinbase().orElseThrow(),
extraDataCalculator,
transactionPool,
protocolContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ public MergeCoordinator(
this.mergeContext = protocolContext.getConsensusContext(MergeContext.class);
this.backwardSyncContext = backwardSyncContext;

if (miningParams.getCoinbase().isEmpty()) {
miningParams.setCoinbase(Address.ZERO);
}
if (miningParams.getTargetGasLimit().isEmpty()) {
miningParams.setTargetGasLimit(30000000L);
}
Expand All @@ -133,7 +136,6 @@ public MergeCoordinator(
transactionPool,
protocolContext,
protocolSchedule,
address.or(miningParameters::getCoinbase).orElse(Address.ZERO),
parentHeader,
depositContractAddress);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
import org.hyperledger.besu.ethereum.core.Difficulty;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.Unstable;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.UpdatableInitValues;
import org.hyperledger.besu.ethereum.core.MiningParameters;
import org.hyperledger.besu.ethereum.core.TransactionTestFixture;
Expand Down Expand Up @@ -134,9 +135,9 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {

MiningParameters miningParameters =
ImmutableMiningParameters.builder()
.coinbase(coinbase)
.updatableInitValues(UpdatableInitValues.builder().coinbase(coinbase).build())
.unstable(
ImmutableMiningParameters.Unstable.builder()
Unstable.builder()
.posBlockCreationRepetitionMinDuration(REPETITION_MIN_DURATION)
.build())
.build();
Expand Down Expand Up @@ -285,7 +286,6 @@ public void exceptionDuringBuildingBlockShouldNotBeInvalid()
transactionPool,
protocolContext,
protocolSchedule,
address.or(miningParameters::getCoinbase).orElse(Address.ZERO),
parentHeader,
Optional.empty()));

Expand Down Expand Up @@ -551,8 +551,7 @@ public void shouldStopRetryBlockCreationIfTimeExpired() throws InterruptedExcept
miningParameters =
ImmutableMiningParameters.builder()
.from(miningParameters)
.unstable(
ImmutableMiningParameters.Unstable.builder().posBlockCreationMaxTime(100).build())
.unstable(Unstable.builder().posBlockCreationMaxTime(100).build())
.build();
doAnswer(
invocation -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
import org.hyperledger.besu.ethereum.core.Difficulty;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters.UpdatableInitValues;
import org.hyperledger.besu.ethereum.eth.sync.backwardsync.BackwardSyncContext;
import org.hyperledger.besu.ethereum.eth.transactions.TransactionPool;
import org.hyperledger.besu.ethereum.mainnet.BlockHeaderValidator;
Expand Down Expand Up @@ -93,7 +94,9 @@ public void setUp() {
mockProtocolSchedule,
CompletableFuture::runAsync,
mockTransactionPool,
ImmutableMiningParameters.builder().coinbase(coinbase).build(),
ImmutableMiningParameters.builder()
.updatableInitValues(UpdatableInitValues.builder().coinbase(coinbase).build())
.build(),
mock(BackwardSyncContext.class),
Optional.empty());
mergeContext.setIsPostMerge(genesisState.getBlock().getHeader().getDifficulty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,8 +372,8 @@ private static ControllerAndState createControllerAndFinalState(
.isMiningEnabled(true)
.minTransactionGasPrice(Wei.ZERO)
.extraData(Bytes.wrap("Qbft Int tests".getBytes(UTF_8)))
.coinbase(AddressHelpers.ofValue(1))
.build())
.coinbase(AddressHelpers.ofValue(1))
.build();

final StubGenesisConfigOptions genesisConfigOptions = new StubGenesisConfigOptions();
Expand Down
Loading