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 all 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: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
- Clique config option `createemptyblocks` to not create empty blocks [#6082](https://github.com/hyperledger/besu/pull/6082)
- Upgrade EVM Reference Tests to v13 (Cancun) [#6114](https://github.com/hyperledger/besu/pull/6114)
- Add `yParity` to GraphQL and JSON-RPC for relevant querise. [6119](https://github.com/hyperledger/besu/pull/6119)
- Force tx replacement price bump to zero when zero base fee market is configured or `--min-gas-price` is set to 0. This allows for easier tx replacement in networks where there is not gas price. [#6079](https://github.com/hyperledger/besu/pull/6079)

### Bug fixes

- Upgrade netty to address CVE-2023-44487, CVE-2023-34462 [#6100](https://github.com/hyperledger/besu/pull/6100)
- Upgrade grpc to address CVE-2023-32731, CVE-2023-33953, CVE-2023-44487, CVE-2023-4785 [#6100](https://github.com/hyperledger/besu/pull/6100)
- Fix blob gas calculation in reference tests [#6107](https://github.com/hyperledger/besu/pull/6107)
Expand Down
37 changes: 23 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,26 @@ 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()) {
Copy link
Contributor

@matthew1001 matthew1001 Oct 30, 2023

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?

Copy link
Contributor Author

@fab-10 fab-10 Nov 2, 2023

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?

Copy link
Contributor

@matthew1001 matthew1001 Nov 6, 2023

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.

Copy link
Contributor Author

@fab-10 fab-10 Nov 7, 2023

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.

logger.info(
"Forcing price bump for transaction replacement to 0, since we are on a zero basefee network");
txPoolConfBuilder.priceBump(Percentage.ZERO);
}

if (getMiningParameters().getMinTransactionGasPrice().equals(Wei.ZERO)
&& !transactionPoolOptions.isPriceBumpSet(commandLine)) {
logger.info(
"Forcing price bump for transaction replacement to 0, since min-gas-price is set to 0");
txPoolConfBuilder.priceBump(Percentage.ZERO);
}

return txPoolConfBuilder.build();
}

private MiningParameters getMiningParameters() {
Expand Down
68 changes: 65 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
Copy link
Contributor

@matthew1001 matthew1001 Oct 30, 2023

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?

Copy link
Contributor Author

@fab-10 fab-10 Nov 2, 2023

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,11 +318,26 @@ public TransactionPoolConfiguration toDomainObject() {
.txPoolLimitByAccountPercentage(legacyOptions.txPoolLimitByAccountPercentage)
.txPoolMaxSize(legacyOptions.txPoolMaxSize)
.pendingTxRetentionPeriod(legacyOptions.pendingTxRetentionPeriod)
.unstable(
ImmutableTransactionPoolConfiguration.Unstable.builder()
.txMessageKeepAliveSeconds(unstableOptions.txMessageKeepAliveSeconds)
.eth65TrxAnnouncedBufferingPeriod(unstableOptions.eth65TrxAnnouncedBufferingPeriod)
.build())
.build();
}

@Override
public List<String> getCLIOptions() {
return CommandLineUtils.getCLIOptions(this, new TransactionPoolOptions());
}

/**
* Is price bump option set?
*
* @param commandLine the command line
* @return true is tx-pool-price-bump is set
*/
public boolean isPriceBumpSet(final CommandLine commandLine) {
return CommandLineUtils.isOptionSet(commandLine, TransactionPoolOptions.TX_POOL_PRICE_BUMP);
}
}
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.

13 changes: 13 additions & 0 deletions besu/src/main/java/org/hyperledger/besu/cli/util/CommandLineUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,17 @@ private static boolean isOptionSet(final CommandLine.Model.OptionSpec option) {
return false;
}
}

/**
* Is the option with that name set on the command line?
*
* @param commandLine the command line
* @param optionName the option name to check
* @return true if set
*/
public static boolean isOptionSet(final CommandLine commandLine, final String optionName) {
return commandLine.getCommandSpec().options().stream()
.filter(optionSpec -> Arrays.stream(optionSpec.names()).anyMatch(optionName::equals))
.anyMatch(CommandLineUtils::isOptionSet);
}
}
Loading