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

Set default --tx-pool-price-bump to 0 for zeroBaseFee chains #6043

Closed
matthew1001 opened this issue Oct 17, 2023 · 4 comments · Fixed by #6079
Closed

Set default --tx-pool-price-bump to 0 for zeroBaseFee chains #6043

matthew1001 opened this issue Oct 17, 2023 · 4 comments · Fixed by #6079
Labels
enhancement New feature or request non mainnet (private networks) not related to mainnet features - covers privacy, permissioning, IBFT2, QBFT

Comments

@matthew1001
Copy link
Contributor

Description

See discord discussion https://discord.com/channels/905194001349627914/905205502940696607/1163795216256471042

After discussion with @fab-10 and @ajsutton there was a general consensus that for free gas chains, the price bump % ought to be set to a default value of 0%, instead of 10%. Replacement of transactions in a free gas chain where priceBump != 0 is effectively impossible and relies on unpleasant work-arounds (such as setting TX pool retention to 1 hour and waiting for it to be removed).

Acceptance Criteria

When I create a new Besu node with zeroBaseFee=0 and/or --max-gas-price=0 the value of --tx-pool-price-bump is set to a default value of 0. I can change it back to 10% or any other valid value by setting it manually.

@matthew1001 matthew1001 added enhancement New feature or request non mainnet (private networks) not related to mainnet features - covers privacy, permissioning, IBFT2, QBFT labels Oct 17, 2023
@matthew1001
Copy link
Contributor Author

Having raised this, I re-tested the effect of --tx-pool-price-bump=0 with non-EIP1559 transactions, and even setting it to 0 does not allow you to replace a transaction.

It would require the addition of e.g.

if (priceBump.getValue() == 0
        && existingPendingTransaction.getGasPrice().compareTo(newPendingTransaction.getGasPrice())
            == 0) {
      return true;
}

to TransactionReplacementByGasPriceRule.shouldReplace() for it to accept a new transaction as the replacement for an old one with the same gasPrice.

I would have added the above under this or another issue, but my concern is that the EIP1559 test (TransactionReplacementByFeeMarketRule.shouldReplace()) has a specific check for the new TX having 0:

// bail early if price is not strictly positive
    if (newEffPrice.equals(Wei.ZERO)) {
      return false;
}

So I'm reticent to put a change like ^^ in since anyone who switched from gasPrice to maxBaseFee/maxPriorityFee would suddenly see a breaking change in TX replacement behaviour.

@fab-10
Copy link
Contributor

fab-10 commented Oct 24, 2023

I think, that we just miss the specific transaction replacement rule for zero base fee chains, like we have it for the other fee market. Such rule can just accept any replacement without checking the price.

@fab-10
Copy link
Contributor

fab-10 commented Oct 25, 2023

@matthew1001 Following you initial investigation, I have made this PR #6079 that should make --tx-pool-price-bump=0 works as expected, could you give it a try?

@matthew1001
Copy link
Contributor Author

Thanks @fab-10 Sorry for the delay, just back from a week off. Yes I'd be happy to give it a try, I'll pull the branch now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request non mainnet (private networks) not related to mainnet features - covers privacy, permissioning, IBFT2, QBFT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants