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

[Consensus] Include the consensus time into block interval. #3627

Open
Jim8y opened this issue Dec 16, 2024 · 14 comments
Open

[Consensus] Include the consensus time into block interval. #3627

Jim8y opened this issue Dec 16, 2024 · 14 comments
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@Jim8y
Copy link
Contributor

Jim8y commented Dec 16, 2024

Summary or problem description
Currently we wait block interval seconds after receiving the previous block before starting a new round of consensus, this excludes the consensus time from the block interval and makes the actual block time longer than the block interval. With an expected of block time as block interval seconds, the actual block time is always larger than that, 1-2 seconds and more.

Do you have any solution you want to propose?
We can include the consensus time into the block interval by waiting block interval seconds after receiving the previous consensus reparerequest before starting a new round of consensus. This way, as long as the consensus delay is less than the block interval seconds, we can ensure a fixed block time, with minor deviation.

image

Where in the software does this update applies to?

  • Consensus
@Jim8y Jim8y added the Discussion Initial issue state - proposed but not yet accepted label Dec 16, 2024
@Jim8y Jim8y changed the title [Consensus] Include the consensus time into consensus interval. [Consensus] Include the consensus time into block interval. Dec 16, 2024
@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 16, 2024

This is the implementation: #3622

@roman-khimov
Copy link
Contributor

Hi, nspcc-dev/dbft#55, nspcc-dev/dbft#56.

@vncoelho
Copy link
Member

This is the implementation: #3622

Please create a PR just with this change.

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 17, 2024

This is the implementation: #3622

Please create a PR just with this change.

@vncoelho it will not be applied to current 15 seconds consensus, but be considered to be applied to 3 seconds solution.

@nan01ab
Copy link
Contributor

nan01ab commented Dec 17, 2024

That's how it should be.

@shargon
Copy link
Member

shargon commented Dec 18, 2024

This is the implementation: #3622

Please create a PR just with this change.

Agree

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 19, 2024

This is the implementation: #3622

Please create a PR just with this change.

Agree

Can't be, create a seperate pr only contain this change means it will be applied to current system, while this will increase the gas distrubution by 10-12%. It should only be applied to the 3 seconds consensus.

@roman-khimov
Copy link
Contributor

It should only be applied to the 3 seconds consensus

This makes no sense to me. All of the current GAS distribution assumes 15s blocks by design. The fact that some blocks deviate from these 15s is certainly not a feature that needs to be kept. The lower the delay the better, absolutely irrespective of block time.

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 19, 2024

It should only be applied to the 3 seconds consensus

This makes no sense to me. All of the current GAS distribution assumes 15s blocks by design. The fact that some blocks deviate from these 15s is certainly not a feature that needs to be kept. The lower the delay the better, absolutely irrespective of block time.

Yes, but vitor is claming that 3-seconds can slow the gas distribution already, so what i want to focus is only how to minimize the impact to gas distribution in 3-seconds consenus, instead of fixing current one. Yes, currenly it assumes 15 seconds, but as you also know it never happened, the average block time for mainnet is around 16.8 seconds recently, so applying this will directly cause 10% more gas being minted, even if its actually expected, the impact to the existing gas holders is unpredictable, not a thing that i, just personally, want to touch.

@roman-khimov
Copy link
Contributor

Have you tried running the same 3s network without this change and with the same tests?

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 20, 2024

Have you tried running the same 3s network without this change and with the same tests?

No, cause just as what we have observed in the mainnet, consensus itself takes 1-2 seconds, so, for me, running that test is meanless due to high delay.

@roman-khimov
Copy link
Contributor

You can't rely on mainnet data for this, mainnet is a very different network.

@Jim8y
Copy link
Contributor Author

Jim8y commented Dec 20, 2024

You can't rely on mainnet data for this, mainnet is a very different network.

for dbft they are the same, yes, they may have minor difference, but that does not affect the dbft process itself. Like it takes 1-3 seconds to run the whole dbft steps in mainnet, it makes no sense it takes less time in 3-seconds-consensus. I understand that it is best to do it to make sure.....but it is not in a hurry, right.

@roman-khimov
Copy link
Contributor

If you want to have any representative results for #3637 this is just the way to test it: take a network (whatever that is), run it without a patch (with/without load), collect metrics, run the same network with a patch with the same tests and then compare the result. There is no other way to do it if you care about real results. Because

it takes 1-3 seconds to run the whole dbft steps in mainnet

is read properly as "we don't know how long it takes on our test network".

Mainnet is estimated as 100+ nodes network. Mainnet has different node implementations and versions including ones that are completely forgotten, stale and misbehaving. Comparing things happening there with things going on in a network of seven uniform fresh nodes directly is not correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

No branches or pull requests

5 participants