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

Implementing CN timeout bonifications #704

Merged
merged 8 commits into from
Apr 20, 2019

Conversation

vncoelho
Copy link
Member

Closes #664

igormcoelho
igormcoelho previously approved these changes Apr 16, 2019
Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

This idea is very good and will probably remove most of the existing change views, in situations where the nodes are clearly evolving.

@vncoelho
Copy link
Member Author

image

igormcoelho
igormcoelho previously approved these changes Apr 16, 2019
Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Added some extra protections, and better parametrization based on M.
This is the most stable version until now on our private network.

@erikzhang
Copy link
Member

Why do we change timer everywhere?

@vncoelho
Copy link
Member Author

vncoelho commented Apr 17, 2019

Change timer were inserted in 4 different places, ensuring that all payload checks were done correctly and no attacks would be possible for increasing a node timer:

  • When prepreq is received with sucess;
  • When prepresp are received with sucess;
  • When commits are received with sucess;

It looked like to be the simplest way to implement.
Do you recommend another way, @erikzhang?

It will not affect liveness.

@igormcoelho
Copy link
Contributor

Why do we change timer everywhere?

@erikzhang , we can rename it to IncreaseTimer, perhaps it's a better name.

@erikzhang
Copy link
Member

Can anyone review this PR? However, I am worried that if we continue to optimize the consensus, dBFT2.0 may never be able to go to the mainnet.

@igormcoelho
Copy link
Contributor

@erikzhang this is not just optimization... we designed this in order to prevent common change views, that we thought could help avoiding the last stall on test net.
But now, after reviewing the logs, we found an actual fix, on #706 / #707

My suggestion is to ask NGD for a report over this... if network behaves so much better, we advise putting this on TestNet together with the pending fix. If it's not deemed too great, then it can be kept for later. It looks like an arbitrary change, but we thought about that from a long time, this was the only way to make our priv net work "perfectly".

@igormcoelho
Copy link
Contributor

@jsolman @shargon can you check this, if you agree on the logic?

@vncoelho
Copy link
Member Author

vncoelho commented Apr 18, 2019

These guys have no limits, @erikzhang...aheuaheuaea
It is going to be an endless journey of optimization :D Let's make science !! 💃
I am joking, @erikzhang....aehauheuaea

This is a nice feature/mechanism. The performance improved considerably as @igormcoelho highlighted.
Several unnecessary changeview are being avoided with this.
Let's move it forward as soon as possible. There are no negative impacts for real-world operation and it also represents a good step towards scalability.

@Qiao-Jin
Copy link
Contributor

Excuse me, but will this have negative effect upon onchain TPS (Transactions per second)? Thx!

@jsolman
Copy link
Contributor

jsolman commented Apr 19, 2019

Excuse me, but will this have negative effect upon onchain TPS (Transactions per second)? Thx!

No, in general it would probably improve TPS. These extensions only apply when the protocol is making progress, which in the case of a working network should be coming to consensus without needing a view change.

Copy link
Contributor

@jsolman jsolman left a comment

Choose a reason for hiding this comment

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

My suggested changes are just naming and comment text. The code implementation seems reasonable to me.

neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
neo/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member Author

Travis problems, how to re-run Travis? Sometime ago I saw that button on their system.

@shargon
Copy link
Member

shargon commented Apr 19, 2019

Restarted

@vncoelho
Copy link
Member Author

I just noticed I was not logged in there, @shargon. Thanks,

@jsolman jsolman dismissed their stale review April 19, 2019 21:40

My concerns were addressed.

@shargon
Copy link
Member

shargon commented Apr 20, 2019

travis is not working on neo-vm too...

@vncoelho
Copy link
Member Author

Thanks everybody.
Byzantine is byzantine, agreement is agreement, fly we go.

@vncoelho vncoelho merged commit f3c7af0 into master Apr 20, 2019
@vncoelho vncoelho deleted the feat/consensus-timeout-bonifications branch April 20, 2019 13:16
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
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.

CN Timeout bonifications
6 participants