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

qbft: add new block period setting used when the block is empty #1433

Merged
merged 14 commits into from
Jul 28, 2022

Conversation

baptiste-b-pegasys
Copy link
Contributor

When the mined block is empty, the period will be longer but a new transaction will mine a non-empty block with the block period.

As a result, we mine less frequently empty blocks.

from review

block, ok := request.Proposal.(*types.Block)
if ok && len(block.Transactions()) == 0 { // if empty block
delay = -time.Since(time.Unix(int64(block.Time()), 0)) + time.Duration(c.config.EmptyBlockPeriod-c.config.BlockPeriod)*time.Second
Copy link

@saltiniroberto saltiniroberto Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block.Time() should be replaced with the timestamp in the header of the parent of block

Also, is delay going negative going to create any issues?

Copy link
Contributor Author

@baptiste-b-pegasys baptiste-b-pegasys Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the parent block information at this place, block time should be the parent block time + block period as value.
I am checking for the negative value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem for negative value or 0, it is executed immediately
https://go.dev/play/p/QB80T3p4KRa

Copy link
Contributor Author

@baptiste-b-pegasys baptiste-b-pegasys Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delta between block's time and now should be 0, I will check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quorum-examples-node1-1  | INFO [07-08|14:58:32.176] qbft block                               time=1657292312 since=176.173958ms
quorum-examples-node1-1  | INFO [07-08|14:58:44.035] qbft block                               time=1657292324 since=35.466547ms
quorum-examples-node1-1  | INFO [07-08|14:58:53.048] qbft block                               time=1657292333 since=48.515009ms
quorum-examples-node1-1  | INFO [07-08|14:59:02.044] qbft block                               time=1657292342 since=44.105264ms
quorum-examples-node1-1  | INFO [07-08|14:59:11.016] qbft block                               time=1657292351 since=16.593976ms
quorum-examples-node1-1  | INFO [07-08|14:59:20.078] qbft block                               time=1657292360 since=78.740772ms
quorum-examples-node1-1  | INFO [07-08|14:59:29.062] qbft block                               time=1657292369 since=62.659776ms
quorum-examples-node1-1  | INFO [07-08|14:59:38.058] qbft block                               time=1657292378 since=58.693822ms
quorum-examples-node1-1  | INFO [07-08|14:59:47.045] qbft block                               time=1657292387 since=45.062118ms
quorum-examples-node1-1  | INFO [07-08|14:59:56.047] qbft block                               time=1657292396 since=47.224956ms
quorum-examples-node1-1  | INFO [07-08|15:00:05.019] qbft block                               time=1657292405 since=19.969544ms
quorum-examples-node1-1  | INFO [07-08|15:00:14.035] qbft block                               time=1657292414 since=35.883881ms
quorum-examples-node1-1  | INFO [07-08|15:00:23.028] qbft block                               time=1657292423 since=28.577886ms
quorum-examples-node1-1  | INFO [07-08|15:00:32.038] qbft block                               time=1657292432 since=38.90014ms

so as it is in seconds, there is no need to add time.Since(time.Unix(int64(block.Time()), 0)), it is negligeable, and we are back to my original formula

@antonydenyer antonydenyer merged commit 72cd58f into Consensys:master Jul 28, 2022
@baptiste-b-pegasys baptiste-b-pegasys deleted the fix/no-empty-block branch September 8, 2022 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants