Implementation of Issue #3810 QBFT no empty block

[amsmota]

PR description

Implementing emptyblockperiodseconds for producing empty blocks at a specific interval independently of the value of the existing blockperiodseconds setting

Fixed Issue(s)

#3810

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

[macfarla]

hey @amsmota DCO check is failing - you need to add signoff to all your commits - instructions here https://github.com/hyperledger/besu/pull/6944/checks?check_run_id=23799447511

I haven't had a chance to look at the contents of your PR yet but pinging @matthew1001 since he's been looking at QBFT lately

[siladu]

I've done a first pass, a few comments.

Is this working for a network of > 1 node?
Can you sync an extra node with the network?

Might be handy to post some examples of your debug logs during e.g. a full round robin of a two node block production.

[amsmota]

hey @amsmota DCO check is failing - you need to add signoff to all your commits - instructions here https://github.com/hyperledger/besu/pull/6944/checks?check_run_id=23799447511

I haven't had a chance to look at the contents of your PR yet but pinging @matthew1001 since he's been looking at QBFT lately

Yes, I saw that, I'll do it today asap.

Cheers.

[amsmota]

I've done a first pass, a few comments.

Is this working for a network of > 1 node? Can you sync an extra node with the network?

Yes, in my local copy I have a network directory (not commited) where I can start a network of 4 nodes. If you think it's usefull for you and other reviweres I can commit&push, providing I don't forget to remove it after the review is done...

Might be handy to post some examples of your debug logs during e.g. a full round robin of a two node block production.

[matthew1001]

I've pulled your branch and tested it out locally, and the basics seem to work fine:

• 2 second block with transactions
• 30 second block without transactions

I'm not sure if the behaviour is correct when a new TX arrives during the the empty-block period. If I send a few transactions in about 5 seconds after the last block, Besu waits the remaining 25 second empty block period before mining a block with them in. I'm wondering if the arrival of new TXs ought to revert the timer to the non-empty timer, or some other behaviour for the transition back from empty to non-empty?

[amsmota]

I've pulled your branch and tested it out locally, and the basics seem to work fine:

• 2 second block with transactions
• 30 second block without transactions

I'm not sure if the behaviour is correct when a new TX arrives during the the empty-block period. If I send a few transactions in about 5 seconds after the last block, Besu waits the remaining 25 second empty block period before mining a block with them in. I'm wondering if the arrival of new TXs ought to revert the timer to the non-empty timer, or some other behaviour for the transition back from empty to non-empty?

That's not the behaviour I see, I posted in the Issue a comment showing the behaviour of empty/non-empty before (emptyBlock = default 0) and after the transition that sets the emptyblock value. After the transition you can see the behaviour, let me copy here the relevant part:

After transition: (after block 49) the transition kicks in and empty blocks take 30s to create, non-empty every 5 sec, see block at 15:12:02)

2024-04-13 **15:10:57**.174+01:00 | QbftBesuControllerBuilder | Imported empty block #49 / 0 tx / 3 pending / 0 (0.0%) gas / (0x0ee851152f1a6faaa199d3f595234141c050966f7be46a65241f01908ce34d07)
2024-04-13 **15:11:27**.233+01:00 | PeerDiscoveryAgent | Writing node record to disk. NodeRecord{seq=2, publicKey=0x027c19f5309e268264c9a986704a2f24e01f466a541ef910f68cc831324ba1ee2c, udpAddress=Optional[/127.0.0.1:30303], tcpAddress=Optional[/127.0.0.1:30303], asBase64=-Je4QKHhLqyEBF13upUKqj-i_cALkD-_-h3yPWZgkdFdnsjZZZCyzpd63wDaWlVWriBpGu_JPciWtEhwgqy-XSiQp0YCg2V0aMfGhOcD3GaAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQJ8GfUwniaCZMmphnBKLyTgH0ZqVB75EPaMyDEyS6HuLIN0Y3CCdl-DdWRwgnZf, nodeId=0xc92329278474852a32bd72b721afbac4d309b7fa6e40862eaee847cc8f544045, customFields={tcp=30303, udp=30303, ip=0x7f000001, eth=[[0xe703dc66, 0x]], id=V4, secp256k1=0x027c19f5309e268264c9a986704a2f24e01f466a541ef910f68cc831324ba1ee2c}}
2024-04-13 **15:11:27**.258+01:00 | QbftBesuControllerBuilder | Imported empty block #50 / 0 tx / 3 pending / 0 (0.0%) gas / (0x4a43cebe16dc6a3c341cb92b109404580bb1bbe756cd2c855ecdc826d14ea9c1)
2024-04-13 **15:11:57**.359+01:00 | QbftBesuControllerBuilder | Imported empty block #51 / 0 tx / 3 pending / 0 (0.0%) gas / (0x02b83a1b6d33dcbafed7c6ceae9bd32cd757874151fb9232124681ac41befba1)
2024-04-13 **15:12:02**.377+01:00 | QbftBesuControllerBuilder | **Produced block #52** / 1 tx / 3 pending / 3,000,000 (63.8%) gas / (0x32dd8d703933b1de951c7f99d3a5e50b66e573494ecf8eafe003df578df1e204)
2024-04-13 **15:12:32**.236+01:00 | QbftBesuControllerBuilder | Imported empty block #53 / 0 tx / 3 pend

You can see that block 52 with 1 transaction was created 5 seconds after the last empty block, and before and after it took 30 secs for empty-blocks...

[amsmota]

hey @amsmota DCO check is failing - you need to add signoff to all your commits - instructions here https://github.com/hyperledger/besu/pull/6944/checks?check_run_id=23799447511
I haven't had a chance to look at the contents of your PR yet but pinging @matthew1001 since he's been looking at QBFT lately

Yes, I saw that, I'll do it today asap.

Cheers.

This now done!

[amsmota]

Guys, I have a question, in MiningParameters there is

      getGenesisBlockPeriodSeconds(getActualGenesisConfigOptions())
          .ifPresent(miningParameters::setBlockPeriodSeconds);

should I add the same for the emptyBlockPeriodSeconds? I initially added and then removed it since this PR is for QBFT only, but it's better to know youse opinions...

[amsmota]

Guys, I just uploaded my network dir for local testing, is very easy to use and youse can see the logs in realtime and change configurations, everything really...

The commit message is
UPLOADED NETWORK FOR TESTING ONLY -- TO REMOVE BEFORE THE FINAL DRAFT -- DO NOT MERGE

It's very easy to use it, just

1. checkout
2. cd to the root dir
3. ./gradlew installDist
4. cd network
5. sh start.sh

There are 3 ways to start the network

• sh start.sh
  • starts the 4 nodes with the current config and data
• sh start.sh --no-data
  • starts the 4 nodes with no data
• sh start.sh --no-data --only
  • removes all 4 nodes data, do not start nodes

The --no-data is useful when changing the genesis settings to restart the network with no blocks.

The console that appears when the network starts is the log for Node-1. The logs for the remaining nodes are being sent to Node-[1,2,4]/node.log

Node -2 log is in DEBUG level.

Node-1 is ready for remote debugging.

Any questions please let me know, but please look at readme file first.

Cheers.

[amsmota]

Well guys, I think I did something wrong with my commits and with my updates from main and according to my OSS Guru @JamieSlome I souldn't have signed-off all those commits above that are from those updates and not from my own code. On top of that, I still have 2 commits unsigned/not signed-off and to rebase it causes dozens of conflicts with code that is not from my branch.

So I think it is better if I close this PR and open a new one after local merging from hyperledger/besu:main and not update anymore until the PR is ready to merge. All changes from your reviews/comments, for which I'm much appreciated, will be there of course.

Please let me know if this is the best way to avoid more issues. If nobody opposes I'll close this one sometime tomorrow.

Thanks guys.

[amsmota]

Hi @siladu @matthew1001 and all, as mentioned yesterday I am closing this PR now, please continue the review on the PR below, that include all the changes from yours reviews/comment in this PR, plus the test network I mentioned above.

#6965

Thank youse.