-
Notifications
You must be signed in to change notification settings - Fork 860
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
Implementation of Issue #3810 QBFT no empty block #6944
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java
Show resolved
Hide resolved
consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java
Show resolved
Hide resolved
...t/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java
Show resolved
Hide resolved
Yes, I saw that, I'll do it today asap. Cheers. |
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...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java
Outdated
Show resolved
Hide resolved
besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java
Outdated
Show resolved
Hide resolved
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:
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... |
6314530
to
664ddf7
Compare
* mining parameter metrics Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> * update changelog Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> * remove unwanted code Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> --------- Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
…ledger#3810) Signed-off-by: amsmota <antonio.mota@citi.com>
…ledger#3810) Signed-off-by: amsmota <antonio.mota@citi.com>
…ledger#3810) Signed-off-by: amsmota <antonio.mota@citi.com>
* removes commenting on pr action, in favor of an updated pr template. also updates gradle build actions * adds fix for nightly docker builds * uses consolidation status to determine re-runs * mention docker image registry in changelog --------- Signed-off-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
* Add Besu version to DB metadata. Check for downgrades and reject if version < version recorded in DB metadata. Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Add --allow-downgrade CLI arg. If set it allows the downgrade and updates the Besu version in the metadata file to the downgraded version. Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update gradle verification XML Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Add and update tests Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Refactoring Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Remove versioning from RocksDB, now in separate VERSION_DATADATA.json Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Tidy up and tests for the new class Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Move downgrade logic into VersionMetadata as BesuCommand is already very big Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Add more tests Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Refactor the naming of the option to version-compatibility-protection Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Remove remaining references to allow-downgrade Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Rename test Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update comments Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Metadata verification update Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * gradle fix Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Enable version downgrade protection by default for non-named networks Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Fix default logic Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Matt Whitehead <matthew1001@hotmail.com> * Update ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/VersionMetadata.java Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Matt Whitehead <matthew1001@hotmail.com> * mock-maker-inline no longer needed Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> --------- Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io> Signed-off-by: Matt Whitehead <matthew1001@hotmail.com> Co-authored-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
…ger#6557) The BerlinGasCalculator had a side effect of warming addresses when calculating call gas costs. By introducing a boolean we can keep the side effects in the operation instead of in the gas calculator. This makes gas cost estimation by downstream users of the API safer. Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com> Signed-off-by: amsmota <antonio.mota@citi.com>
* append docker pull command for the release to notes --------- Signed-off-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: amsmota <antonio.mota@citi.com>
…null (hyperledger#6599) Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
* invalid payload error code * use non-zero timestamp * use invalidPayloadAttributes error for V2 if non-null beacon root pre cancun Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
…r#6605) Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
* repo owner didn't include repo name * switches back to docker.io * specify registry and login consistently * artifacts workflow can be manually executed --------- Signed-off-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: amsmota <antonio.mota@citi.com>
…6603) Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Jason Frame <jason.frame@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Antonio Mota <antonio.mota@citi.com> Signed-off-by: amsmota <antonio.mota@citi.com>
…amic options (hyperledger#6611) Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Preparatory for EOF. Refactor the gas calculation logic so that it does not depend on specific stack entries, and break out the pieces of the calculation for readability. Signed-off-by: Danno Ferrin <danno@numisight.com> Co-authored-by: Justin Florentine <justin+github@florentine.us> Co-authored-by: Danno Ferrin <danno@numisight.com> Signed-off-by: amsmota <antonio.mota@citi.com>
* add rlp decode subcommand Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> --------- Signed-off-by: Brindrajsinh-Chauhan <brindrajsinh@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
…rledger#6930) Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Execution-spec-tests and reference tests have migrated Merge to Paris. Support both. Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: amsmota <antonio.mota@citi.com>
* verify Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Make handling of absurdly large nonces more consistent across txpool and EVM. Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: amsmota <antonio.mota@citi.com>
…ger#6943) * fix default disabled for downgrade protection on named networks implement comparable for VersionMetadata, add explicit coverage Signed-off-by: garyschulte <garyschulte@gmail.com> * test method rename Signed-off-by: garyschulte <garyschulte@gmail.com> * remove errant comment Signed-off-by: garyschulte <garyschulte@gmail.com> * do not use static version metadata, to facilitate unit tests Signed-off-by: garyschulte <garyschulte@gmail.com> --------- Signed-off-by: garyschulte <garyschulte@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
Publish to dockerhub on merge to the main branch Signed-off-by: garyschulte <garyschulte@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
…ine with Info level (hyperledger#6939) Signed-off-by: Ameziane H <ameziane.hamlat@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: amsmota <antonio.mota@citi.com>
This reverts commit 7e46889. Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: amsmota <antonio.mota@citi.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
664ddf7
to
aa7e38f
Compare
This now done! |
Signed-off-by: amsmota <amsmota@gmail.com>
Guys, I have a question, in MiningParameters there is
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... |
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 It's very easy to use it, just
There are 3 ways to start the network
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. |
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. |
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. Thank youse. |
PR description
Implementing
emptyblockperiodseconds
for producing empty blocks at a specific interval independently of the value of the existingblockperiodseconds
settingFixed Issue(s)
#3810
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests