Skip to content

Force tx replacement price bump to zero when zero base fee market is configured #6079

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.

Sign up for GitHub

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

Merged
fab-10 merged 9 commits into from
Nov 8, 2023
Merged

Force tx replacement price bump to zero when zero base fee market is configured #6079

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a12ad97
Make a tx replacement valid also if the increment is = the priceBump
fab-10 Oct 24, 2023
c7ca182
Force tx replacement price bump to zero when zero base fee market is …
fab-10 Oct 24, 2023
09e46dc
Merge branch 'main' into price-bump-zero
fab-10 Oct 25, 2023
8b73053
Unit tests
fab-10 Oct 25, 2023
4acffd7
[skip ci] Javadoc and CHANGELOG
fab-10 Oct 25, 2023
caf560e
Merge branch 'main' into price-bump-zero
fab-10 Nov 2, 2023
ca3c337
Merge branch 'main' into price-bump-zero
fab-10 Nov 7, 2023
c481949
Set tx-pool-price-bump to 0, when not specified and min-gas-price is …
fab-10 Nov 7, 2023
4404e8d
Merge branch 'main' into price-bump-zero
fab-10 Nov 8, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
### Breaking Changes

### Deprecations
- `--tx-pool-disable-locals` has been deprecated for removal in favor of `--tx-pool-no-local-priority`, no semantic change, only a renaming [#5959](https://github.com/hyperledger/besu/pull/5959)

@matthew1001 matthew1001 Oct 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this from another PR? It doesn't seem related to the price-bump behaviour.

@fab-10 fab-10 Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed, this is not related to this PR


### Additions and Improvements
- Force tx replacement price bump to zero when zero base fee market is configured. This allows for easier tx replacement in networks where there is not gas price. [#6079](https://github.com/hyperledger/besu/pull/6079)

### Bug Fixes

Expand Down
28 changes: 14 additions & 14 deletions besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@
import org.hyperledger.besu.cli.error.BesuExecutionExceptionHandler;
import org.hyperledger.besu.cli.error.BesuParameterExceptionHandler;
import org.hyperledger.besu.cli.options.MiningOptions;
import org.hyperledger.besu.cli.options.TransactionPoolOptions;
import org.hyperledger.besu.cli.options.stable.DataStorageOptions;
import org.hyperledger.besu.cli.options.stable.EthstatsOptions;
import org.hyperledger.besu.cli.options.stable.LoggingLevelOption;
import org.hyperledger.besu.cli.options.stable.NodePrivateKeyFileOption;
import org.hyperledger.besu.cli.options.stable.P2PTLSConfigOptions;
import org.hyperledger.besu.cli.options.stable.TransactionPoolOptions;
import org.hyperledger.besu.cli.options.unstable.ChainPruningOptions;
import org.hyperledger.besu.cli.options.unstable.DnsOptions;
import org.hyperledger.besu.cli.options.unstable.EthProtocolOptions;
Expand Down Expand Up @@ -283,9 +283,6 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
final SynchronizerOptions unstableSynchronizerOptions = SynchronizerOptions.create();
final EthProtocolOptions unstableEthProtocolOptions = EthProtocolOptions.create();
final MetricsCLIOptions unstableMetricsCLIOptions = MetricsCLIOptions.create();
final org.hyperledger.besu.cli.options.unstable.TransactionPoolOptions
unstableTransactionPoolOptions =
org.hyperledger.besu.cli.options.unstable.TransactionPoolOptions.create();
private final DnsOptions unstableDnsOptions = DnsOptions.create();
private final NatOptions unstableNatOptions = NatOptions.create();
private final NativeLibraryOptions unstableNativeLibraryOptions = NativeLibraryOptions.create();
Expand All @@ -303,8 +300,7 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
private final LoggingLevelOption loggingLevelOption = LoggingLevelOption.create();

@CommandLine.ArgGroup(validate = false, heading = "@|bold Tx Pool Common Options|@%n")
final org.hyperledger.besu.cli.options.stable.TransactionPoolOptions
stableTransactionPoolOptions = TransactionPoolOptions.create();
final TransactionPoolOptions transactionPoolOptions = TransactionPoolOptions.create();

@CommandLine.ArgGroup(validate = false, heading = "@|bold Block Builder Options|@%n")
final MiningOptions miningOptions = MiningOptions.create();
Expand Down Expand Up @@ -1525,7 +1521,6 @@ private void handleUnstableOptions() {
.put("NAT Configuration", unstableNatOptions)
.put("Privacy Plugin Configuration", unstablePrivacyPluginOptions)
.put("Synchronizer", unstableSynchronizerOptions)
.put("TransactionPool", unstableTransactionPoolOptions)
.put("Native Library", unstableNativeLibraryOptions)
.put("EVM Options", unstableEvmOptions)
.put("IPC Options", unstableIpcOptions)
Expand Down Expand Up @@ -1794,7 +1789,7 @@ private void validateOptions() {
}

private void validateTransactionPoolOptions() {
stableTransactionPoolOptions.validate(commandLine);
transactionPoolOptions.validate(commandLine, getActualGenesisConfigOptions());
}

private void validateRequiredOptions() {
Expand Down Expand Up @@ -2811,12 +2806,17 @@ private SynchronizerConfiguration buildSyncConfig() {
}

private TransactionPoolConfiguration buildTransactionPoolConfiguration() {
final var stableTxPoolOption = stableTransactionPoolOptions.toDomainObject();
return ImmutableTransactionPoolConfiguration.builder()
.from(stableTxPoolOption)
.unstable(unstableTransactionPoolOptions.toDomainObject())
.saveFile((dataPath.resolve(stableTxPoolOption.getSaveFile().getPath()).toFile()))
.build();
final var txPoolConf = transactionPoolOptions.toDomainObject();
final var txPoolConfBuilder =
ImmutableTransactionPoolConfiguration.builder()
.from(txPoolConf)
.saveFile((dataPath.resolve(txPoolConf.getSaveFile().getPath()).toFile()));

if (getActualGenesisConfigOptions().isZeroBaseFee()) {

@matthew1001 matthew1001 Oct 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this mean that you need to be on the london fork in order to get the new behaviour? Could it be made to work in the same way for --min-gas-price=0 for users pre-london?

@fab-10 fab-10 Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but setting --min-gas-price=0 does not necessarily imply that it is a zero-gas-price network (actually also in zeroBaseFee network, the maxGasPrice could be > 0).

So what if we introduce a specific option, that acts like a profile, that state clearly that we want a gas free network, for example --gas-free? that has the effect of setting sound values for other options like the min-gas-price and the tx-pool-price-bump, instead of trying to deduce the intent of the network from single options.

what do you think of this approach?

@matthew1001 matthew1001 Nov 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In principle I agree @fab-10. However, I do have a couple of concerns about introducing it under this PR:

  1. Most people are probably using --min-gas-price=0 and/or zeroBaseFee: true to create what they think of as a gas free network. I agree it's not strictly gas free - but it probably means that defaulting --tx-pool-price-bump to 0 if you're using one/both of those settings is what most people would expect. They can after all set --tx-pool-price-bump= (num > 0) if they don't want it
  2. I think if we had a --gas-free option people would have additional expectations if they set it - not just a default price bump of 0. For example, would allocating initial funds to accounts be valid if --gas-free was set? Would setting a transaction with a gasPrice>0 be allowed?

So I agree that --gas-free would be a really useful option. But I think it's probably worth separate discussion & PR to introduce it.

@fab-10 fab-10 Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok I see your point about min-gas-price and will implement your suggestion.

And let's continue the discussion about a gas-free profile, and all the feature that it should enable separately.

txPoolConfBuilder.priceBump(Percentage.ZERO);
}

return txPoolConfBuilder.build();
}

private MiningParameters getMiningParameters() {
Expand Down
58 changes: 55 additions & 3 deletions ...ptions/stable/TransactionPoolOptions.java → ...u/cli/options/TransactionPoolOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,19 @@
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.cli.options.stable;
package org.hyperledger.besu.cli.options;

import static org.hyperledger.besu.cli.DefaultCommandValues.MANDATORY_DOUBLE_FORMAT_HELP;
import static org.hyperledger.besu.cli.DefaultCommandValues.MANDATORY_INTEGER_FORMAT_HELP;
import static org.hyperledger.besu.cli.DefaultCommandValues.MANDATORY_LONG_FORMAT_HELP;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration.Implementation.LAYERED;
import static org.hyperledger.besu.ethereum.eth.transactions.TransactionPoolConfiguration.Implementation.LEGACY;

import org.hyperledger.besu.cli.converter.DurationMillisConverter;
import org.hyperledger.besu.cli.converter.FractionConverter;
import org.hyperledger.besu.cli.converter.PercentageConverter;
import org.hyperledger.besu.cli.options.CLIOptions;
import org.hyperledger.besu.cli.util.CommandLineUtils;
import org.hyperledger.besu.config.GenesisConfigOptions;
import org.hyperledger.besu.datatypes.Address;
import org.hyperledger.besu.datatypes.Wei;
import org.hyperledger.besu.ethereum.eth.transactions.ImmutableTransactionPoolConfiguration;
Expand All @@ -32,6 +33,7 @@
import org.hyperledger.besu.util.number.Percentage;

import java.io.File;
import java.time.Duration;
import java.util.List;
import java.util.Set;

Expand Down Expand Up @@ -195,6 +197,39 @@ static class Legacy {
Integer txPoolMaxSize = TransactionPoolConfiguration.DEFAULT_MAX_PENDING_TRANSACTIONS;
}

@CommandLine.ArgGroup(validate = false)
private final TransactionPoolOptions.Unstable unstableOptions =
new TransactionPoolOptions.Unstable();

static class Unstable {
private static final String TX_MESSAGE_KEEP_ALIVE_SEC_FLAG =
"--Xincoming-tx-messages-keep-alive-seconds";

private static final String ETH65_TX_ANNOUNCED_BUFFERING_PERIOD_FLAG =
"--Xeth65-tx-announced-buffering-period-milliseconds";
Comment on lines +205 to +209

@matthew1001 matthew1001 Oct 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again this seems to be unrelated to this PR?

@fab-10 fab-10 Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I put this here since, I am in the process of refactoring how we manage the option, and so when I took some, I also take the opportunity to iterate on it, in this case it was the merging of (un)stable options in a single place.


@CommandLine.Option(
names = {TX_MESSAGE_KEEP_ALIVE_SEC_FLAG},
paramLabel = "<INTEGER>",
hidden = true,
description =
"Keep alive of incoming transaction messages in seconds (default: ${DEFAULT-VALUE})",
arity = "1")
private Integer txMessageKeepAliveSeconds =
TransactionPoolConfiguration.Unstable.DEFAULT_TX_MSG_KEEP_ALIVE;

@CommandLine.Option(
names = {ETH65_TX_ANNOUNCED_BUFFERING_PERIOD_FLAG},
paramLabel = "<LONG>",
converter = DurationMillisConverter.class,
hidden = true,
description =
"The period for which the announced transactions remain in the buffer before being requested from the peers in milliseconds (default: ${DEFAULT-VALUE})",
arity = "1")
private Duration eth65TrxAnnouncedBufferingPeriod =
TransactionPoolConfiguration.Unstable.ETH65_TRX_ANNOUNCED_BUFFERING_PERIOD;
}

private TransactionPoolOptions() {}

/**
Expand Down Expand Up @@ -230,6 +265,10 @@ public static TransactionPoolOptions fromConfig(final TransactionPoolConfigurati
config.getTxPoolLimitByAccountPercentage();
options.legacyOptions.txPoolMaxSize = config.getTxPoolMaxSize();
options.legacyOptions.pendingTxRetentionPeriod = config.getPendingTxRetentionPeriod();
options.unstableOptions.txMessageKeepAliveSeconds =
config.getUnstable().getTxMessageKeepAliveSeconds();
options.unstableOptions.eth65TrxAnnouncedBufferingPeriod =
config.getUnstable().getEth65TrxAnnouncedBufferingPeriod();

return options;
}
Expand All @@ -239,8 +278,10 @@ public static TransactionPoolOptions fromConfig(final TransactionPoolConfigurati
* options are valid for the selected implementation.
*
* @param commandLine the full commandLine to check all the options specified by the user
* @param genesisConfigOptions the genesis config options
*/
public void validate(final CommandLine commandLine) {
public void validate(
final CommandLine commandLine, final GenesisConfigOptions genesisConfigOptions) {
CommandLineUtils.failIfOptionDoesntMeetRequirement(
commandLine,
"Could not use legacy transaction pool options with layered implementation",
Expand All @@ -252,6 +293,12 @@ public void validate(final CommandLine commandLine) {
"Could not use layered transaction pool options with legacy implementation",
!txPoolImplementation.equals(LEGACY),
CommandLineUtils.getCLIOptionNames(Layered.class));

CommandLineUtils.failIfOptionDoesntMeetRequirement(
commandLine,
"Price bump option is not compatible with zero base fee market",
!genesisConfigOptions.isZeroBaseFee(),
List.of(TX_POOL_PRICE_BUMP));
}

@Override
Expand All @@ -271,6 +318,11 @@ public TransactionPoolConfiguration toDomainObject() {
.txPoolLimitByAccountPercentage(legacyOptions.txPoolLimitByAccountPercentage)
.txPoolMaxSize(legacyOptions.txPoolMaxSize)
.pendingTxRetentionPeriod(legacyOptions.pendingTxRetentionPeriod)
.unstable(
ImmutableTransactionPoolConfiguration.Unstable.builder()
.txMessageKeepAliveSeconds(unstableOptions.txMessageKeepAliveSeconds)
.eth65TrxAnnouncedBufferingPeriod(unstableOptions.eth65TrxAnnouncedBufferingPeriod)
.build())
.build();
}

Expand Down
94 changes: 0 additions & 94 deletions besu/src/main/java/org/hyperledger/besu/cli/options/unstable/TransactionPoolOptions.java
View file
Open in desktop

This file was deleted.

27 changes: 27 additions & 0 deletions besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ public class BesuCommandTest extends CommandTestAbstract {
private static final JsonObject GENESIS_WITH_DATA_BLOBS_ENABLED =
new JsonObject().put("config", new JsonObject().put("cancunTime", 1L));

private static final JsonObject GENESIS_WITH_ZERO_BASE_FEE_MARKET =
new JsonObject().put("config", new JsonObject().put("zeroBaseFee", true));

static {
DEFAULT_JSON_RPC_CONFIGURATION = JsonRpcConfiguration.createDefault();
DEFAULT_GRAPH_QL_CONFIGURATION = GraphQLConfiguration.createDefault();
Expand Down Expand Up @@ -5195,6 +5198,30 @@ public void txpoolSaveFileAbsolutePathOutsideDataPath() throws IOException {
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void txpoolForcePriceBumpToZeroWhenZeroBaseFeeMarket() throws IOException {
final Path genesisFile = createFakeGenesisFile(GENESIS_WITH_ZERO_BASE_FEE_MARKET);
parseCommand("--genesis-file", genesisFile.toString());
verify(mockControllerBuilder)
.transactionPoolConfiguration(transactionPoolConfigCaptor.capture());

final Percentage priceBump = transactionPoolConfigCaptor.getValue().getPriceBump();
assertThat(priceBump).isEqualTo(Percentage.ZERO);

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8)).isEmpty();
}

@Test
public void txpoolPriceBumpOptionIncompatibleWithZeroWhenZeroBaseFeeMarket() throws IOException {
final Path genesisFile = createFakeGenesisFile(GENESIS_WITH_ZERO_BASE_FEE_MARKET);
parseCommand("--genesis-file", genesisFile.toString(), "--tx-pool-price-bump", "5");

assertThat(commandOutput.toString(UTF_8)).isEmpty();
assertThat(commandErrorOutput.toString(UTF_8))
.contains("Price bump option is not compatible with zero base fee market");
}

@Test
public void snapsyncHealingOptionShouldBeDisabledByDefault() {
final TestBesuCommand besuCommand = parseCommand();
Expand Down
11 changes: 3 additions & 8 deletions besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@
import org.hyperledger.besu.chainimport.RlpBlockImporter;
import org.hyperledger.besu.cli.config.EthNetworkConfig;
import org.hyperledger.besu.cli.options.MiningOptions;
import org.hyperledger.besu.cli.options.TransactionPoolOptions;
import org.hyperledger.besu.cli.options.stable.EthstatsOptions;
import org.hyperledger.besu.cli.options.unstable.EthProtocolOptions;
import org.hyperledger.besu.cli.options.unstable.MetricsCLIOptions;
import org.hyperledger.besu.cli.options.unstable.NetworkingOptions;
import org.hyperledger.besu.cli.options.unstable.SynchronizerOptions;
import org.hyperledger.besu.cli.options.unstable.TransactionPoolOptions;
import org.hyperledger.besu.components.BesuComponent;
import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfiguration;
import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfigurationProvider;
Expand Down Expand Up @@ -560,17 +560,12 @@ public EthProtocolOptions getEthProtocolOptions() {
return unstableEthProtocolOptions;
}

public org.hyperledger.besu.cli.options.stable.TransactionPoolOptions
getStableTransactionPoolOptions() {
return stableTransactionPoolOptions;
}

public MiningOptions getMiningOptions() {
return miningOptions;
}

public TransactionPoolOptions getUnstableTransactionPoolOptions() {
return unstableTransactionPoolOptions;
public TransactionPoolOptions getTransactionPoolOptions() {
return transactionPoolOptions;
}

public MetricsCLIOptions getMetricsCLIOptions() {
Expand Down
Loading