feat: add fixed basefee options

[suraneti]

PR description

Add fixedBaseFee options to forced baseFee same as gasPrice value.

I'm new to the Java language, please guide any aspects I can further improve or modify.

Fixed Issue(s)

#6335

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests
• I thought about running CI.
• If I did not run CI, I ran as much locally as possible before pushing.

[matthew1001]

Thanks for raising this PR @suraneti, I think it's generally looking good so far. I've got a few general comments which I've put below, and I thought I would create a branch in my own repo (see https://github.com/matthew1001/besu/tree/fixed-basefee) with some suggested refactoring for you to take a look at. I'm happy to submit my commit to your branch if you're OK with that, or for you just to use it as an example of suggested changes and you push your own commits, just let me know what you prefer.

• The new FixedBaseFeeMarket is a good idea as it fits naturally into the existing classes (LondonFeeMarket, ZeroBaseFeeMarket et.c)
• It might make sense for the existing ZeroBaseFeeMarket to become an extension of the new FixedBaseFeeMarket. This would mean we're treating zero just as a special case of a fixed base fee. See my branch which shows this refactoring.
• In MainnetProtocolSpecs you're right to pass in the fixed base fee depending on whether genesisConfigOptions.isFixedBaseFee() is set, but you are trying to pass the string that is used to set the gas price - "--tx-pool-min-gas-price" - not the gas price number itself. I think you will need to update the code in MainnetProtocolSpecs so that the MiningParameters instance is passed in. Then your code can call FeeMarket.fixedBaseFee(londonForkBlockNumber, miningParameters.getMinTransactionGasPrice()) which passes in the actual min gas price number.
  • For an example you could look at the BftBlockCreatorFactory() constructor. miningParameters is passed in to that class and stored as a local variable miningParameters. Then later on in the class, miningParameters.getMinTransactionGasPrice() is used to retrieve the configured gas price.
• Most PRs require tests to be added or updated so that the new function is being tested in every build. You could look at ZeroBaseFeeMarketTest for an example of how the existing ZeroBaseFeeMarket is tested, and maybe create a new FixedBaseFeeMarketTest class containing some tests of the new behaviour. It's probably also good to add the new option to GenesisConfigOptionsTest. There may be other tests that are worth adding or updated, but at the very least it would be good to add the ones above.

[suraneti]

@matthew1001, thank you for providing detailed guidelines. I would be grateful if you could submit the code to my branch. I will try to create the unit-test files following your instructions.

[matthew1001]

I've just pushed my commit to your branch @suraneti - let me know how you get on with it.

[suraneti]

@matthew1001, thank you for your commit. I just pushed my latest commit. Could you please review it when you have time?
.
Currently, I'm stuck on how to pass miningParameters to MainnetProtocolSpecs. Should I pass them through ProtocolScheduleBuilder? However, this would require editing all protocols to include the miningParameters argument, right?
.
I've noticed that updating the ProtocolScheduleBuilder to include a new miningParameters argument affects related classes, such as CliqueProtocolSchedule, BaseBftProtocolScheduleBuilder, IbftProtocolScheduleBuilder and others. This results in too many files to edit.
.
Perhaps I am implementing it in the wrong way.

[matthew1001]

I'll take a look at your updated branch in the next day or two @suraneti

[suraneti]

@matthew1001 Great, thanks! I appreciate you taking the time.

[matthew1001]

@suraneti the PR is looking good, a few new comments:

• I think your observation that a lot of files need touching to pass the mining parameters through is correct, and it's probably OK. If we're saying with this new feature that the baseFee (which is part of the protocol spec) is linked to the configured gas price (which is part of the mining parameters) then I think miningParameters needs passing through to the protocol schedule builder. The fact that it affects a lot of files & tests doesn't feel like a problem to me, and we shouldn't architect the code to reduce the number of files that are changed. So basically I think what you've done is fine
• I've added a commit to your branch which fixes a few more of the tests which were failing
• I'm not sure I could see a test which actually sets --min-gas-price and checks that the baseFee is correct. I may have missed it, but it's just worth checking that we're actually testing the new function as well as fixing up the existing tests
• Your commits need signing off as-per the Besu contributor guidelines here: https://github.com/hyperledger/besu/blob/main/CONTRIBUTING.md. Usually adding -s to your commits with your git config set up correctly is enough. Given that you have a lot of commits in this PR, I'm not sure if it's easy/possible to sign off the existing ones, but all commits will need to be be signed off for the PR to merge.

[suraneti]

@matthew1001, thanks for your guidance and commit!
.
I've rebased and signed off on all commits, but two have issues:

Commit sha: [7ee3288](https://github.com/hyperledger/besu/pull/6562/commits/7ee32888eb35d395b18783827446e2fd5800b671), Author: ahamlat, Committer: Suraneti Rodsuwan; Expected "ahamlat [[email protected]](mailto:[email protected])", but got "Ameziane H [[email protected]](mailto:[email protected])".
Commit sha: [03061f7](https://github.com/hyperledger/besu/pull/6562/commits/03061f7a9eab62e2b372fb098004df81a40312f7), Author: Danno Ferrin, Committer: Suraneti Rodsuwan; Expected "Danno Ferrin [[email protected]](mailto:[email protected])", but got "Danno Ferrin [[email protected]](mailto:[email protected])".

My apologies for the conflicts caused by the recent rebase.
.
I will take a look to ensure that unit tests cover the set --min-gas-price, and the baseFee value is correct.

[matthew1001]

It looks like you've got a lot of changes in your branch now that aren't related to the PR itself. It looks like you didn't end up with a clean rebase on top of Besu main?

[suraneti]

@matthew1001 I might have made a mistake, I followed the DCO bot guideline and resolved the conflicts manually.

Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
In your local branch, run: git rebase HEAD~49 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin feature/fixed-basefee

Can you guide me on how to resolve an issue?

[matthew1001]

@suraneti - would you mind trying a force-push of your new branch suraneti:feat/fixed-basefee to this branch suraneti:feature/fixed-basefee? That should mean you have the correct commit list with correct DCO sign-off, but has the benefit that we can retain all of the above discussion in this PR.

You could then close the new PR https://github.com/hyperledger/besu/pull/6624/files

Then I'll do a final review of this PR with the new commit list.

[suraneti]

@matthew1001, I have updated this branch. My apologies for the PR issue.

Please review when you have time. Many thanks.

[matthew1001]

Thanks for all the tidy up with the commits. I think the PR looks good now. There's one small comment I left about the wording in a code-comment, but happy to approve the PR now.

[non-fungible-nelson]

What is needed to get this across the line? @matthew1001 @suraneti

[matthew1001]

I think it's good to go @non-fungible-nelson but I'll let @suraneti confirm if that's the case as he's been doing the bulk of the development and test on the PR. @suraneti - is there anything left to do on this now?

[macfarla]

looks ok to me. vast majority of the changes in this PR is to wire through the mining params so have opened a ticket to refactor that - but can be done after this as a separate PR

[suraneti]

@matthew1001 @non-fungible-nelson @macfarla Thanks so much for following up and providing the feedback. I've confirmed my implementation part has been done.

Have a good weekend!

[matthew1001]

@suraneti that's great. If you're happy with it, could you update the branch with latest main and merge it please?

[suraneti]

@matthew Thanks so much, I've merged the pull request with the latest main branch.

[joaniefromtheblock]

Hi @matthew1001 and @suraneti. We are hoping to add this to the docs in this PR. Can you please take a look? Do you think It should be documented in other sections