Restore previous behaviour for preMergeBesuControllerBuilder

[lucassaldanha]

PR description

Let me start by saying that I don't fully understand the intricacies of the TransitionCoodinator, so it is possible that this PR is going in the wrong direction. However, after analyzing the changes on #6027 I noticed that we have changed the existing behaviour when we are on a pre-merge network. This was caught by one of our tests in Teku that tests the merge transition, when I was trying to upgrade the version of Besu we use in our ATs (we are using 23.10.1).

In the code before commit 20a82d4, we were building a copy of MiningParameters using the instance of mining parameter that was passed as a parameter, that contains the options specified by the user. In this particular case, I am focusing on the miner-coinbase option. This is the snippet of code that we were using before:

final TransitionCoordinator composedCoordinator =
    new TransitionCoordinator(
        preMergeBesuControllerBuilder.createMiningCoordinator(
            transitionProtocolSchedule.getPreMergeSchedule(),
            protocolContext,
            transactionPool,
            new MiningParameters.Builder(miningParameters).miningEnabled(false).build(), // HERE!!!
            syncState,
            ethProtocolManager),
        mergeBesuControllerBuilder.createTransitionMiningCoordinator(
            transitionProtocolSchedule,
            protocolContext,
            transactionPool,
            transitionMiningParameters,
            syncState,
            transitionBackwardsSyncContext,
            metricsSystem));

Using this option, the new instance would preserve all other options specified by the user (including miner-coinbase), but set the flag mining-enabled to false.

In the current version of the code, this is the snippet being used:

final TransitionCoordinator composedCoordinator =
    new TransitionCoordinator(
        preMergeBesuControllerBuilder.createMiningCoordinator(
            transitionProtocolSchedule.getPreMergeSchedule(),
            protocolContext,
            transactionPool,
            MiningParameters.MINING_DISABLED,  // HERE!!!
            syncState,
            ethProtocolManager),
        mergeBesuControllerBuilder.createTransitionMiningCoordinator(
            transitionProtocolSchedule,
            protocolContext,
            transactionPool,
            transitionMiningParameters,
            syncState,
            transitionBackwardsSyncContext,
            metricsSystem));

This way, using the static MiningParameters.MINING_DISABLED value, does not preserve any of the other options sey by the user, including the miner-coinbase value.

The consequence of such change is that when we follow the execution, there is a call to:

composedCoordinator.getPreMergeObject().start();

And that eventually lead us to this code from PowMinerExecutior.java:

@Override
public Optional<PoWBlockMiner> startAsyncMining(
    final Subscribers<MinedBlockObserver> observers,
    final Subscribers<PoWObserver> ethHashObservers,
    final BlockHeader parentHeader) {
  if (miningParameters.getCoinbase().isEmpty()) {
    throw new CoinbaseNotSetException("Unable to start mining without a coinbase.");
  }
  return super.startAsyncMining(observers, ethHashObservers, parentHeader);
}

And at this point, because we have completely wiped the user options in mining parameters, we do not have a coinbase set and the node exits with an error.

I am not 100% sure if this is caused by a configuration change that I need to do on my side or not. But what it looks like is that we have somewhat broken our ability to start from a pre-merged network.

I am going to attach one config file (besu.toml) and one genesis file (genesis.json) that I was using testing this issue. Maybe they will help.

Fixed Issue(s)

N/A

Thanks for sending a pull request! Have you done the following?

• Checked out our contribution guidelines?
• Considered documentation and added the doc-change-required label to this PR if updates are required.
• Considered the changelog and included an update if required.
• For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• unit tests: ./gradlew build
• acceptance tests: ./gradlew acceptanceTest
• integration tests: ./gradlew integrationTest
• reference tests: ./gradlew ethereum:referenceTests:referenceTests

[lucassaldanha]

Here are the supporting files:

besu.toml

engine-rpc-port = '8550'
discovery-enabled = false
rpc-http-cors-origins = ['*']
rpc-http-api = ['ETH,NET,WEB3,ENGINE,ADMIN']
rpc-http-enabled = true
miner-enabled = true
engine-host-allowlist = ['*']
miner-coinbase = '0xfe3b557e8fb62b89f4916b721be55ceb828dbd73'
rpc-ws-enabled = true
host-allowlist = ['*']
engine-jwt-disabled = false
rpc-ws-api = ['ETH,NET,WEB3,ENGINE,ADMIN']
genesis-file = '/Users/lucas/Desktop/besudir/genesis.json'
rpc-http-port = '8545'

genesis.json

[fab-10]

Indeed there was an issue with my change, that was overriding any existing configuration.

Logically there should be only one instance of MiningParameters since it also contains some updatable at runtime conf, so creating another one, the runtime conf is no more synchronized and this could result in strange behavior at runtime. Your change is not changing anything in this regard, since also before another instance was passed to preMergeBesuControllerBuilder.

Said that, does it make sense to still have this pre-merge transition mechanism in place? are all the new devnets starting from post-merge? if yes we could consider removing this transition code, so we do not need to maintain it anymore.

FYI: @non-fungible-nelson

[lucassaldanha]

Maybe this is something we want to discuss in a ACD. If we do not want to support pre-merge networks and the transition etc, there is a lot of simplification we can have on both sides.

[non-fungible-nelson]

@julien-marchand @FlorianHuc we should consider the implications. I am still OOO but let's discuss