-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adding a fast synchronisation protocol #14
Conversation
core/ordering/cosipbft/proc.go
Outdated
cancel() | ||
} | ||
} | ||
panic("Should not get here") |
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.
please either change this to a more meaningful error message, e.g. including the reason why or remove it
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.
It only gets there if the channel is closed, but there is no code to do that. Should I keep the panic
anyway, but with a message to say that the channel got closed?
mino/option.go
Outdated
func RandomFilter(count int) FilterUpdater { | ||
return func(filters *Filter) { | ||
if len(filters.Indices) < count { | ||
return |
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.
doesn't that mean that
- the caller cannot know that the elements have not been chosen at random if |filter.Indices| < count
- the caller cannot distinguish between randomly chosen elements and the re-ordered initial array if |filter.Indice| == count
unless the caller explicitly tests for these two cases? is that a problem?
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.
The goal of the RandomFilter
is to choose count
random elements of the Indices
. It returns the Indices
sorted.
Given these constraints, if the length of the Indices
is <=
count, there is nothing to be done.
So I think I only need to update the <=
and the comment...
Does that look reasonable?
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.
yes, sounds good
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.
apart from the open question about whether to sort the RandomFilter output it looks good to me
Use RandomFilter to select a given amount of indexes randomly. Added suggestion from @PascalinDe
Adds the fastsync as default protocol to cosipbft. Adds a 'WithBlockSync' option for the old, slow protocol. Updates all the tests.
Great - waiting for the tests to pass, then I merge. |
This PR adds a faster synchronisation protocol which only gets called if a node
thinks it's out of sync with the rest of the chain.
The messages passed back and forth are kept minimal to avoid using too much bandwidth.
The cosipbft can now be run either with the old blocksync, or using fastsync.
For reviewing, please:
core/ordering/cosipbft/fastsync/*.go
- sub-directoriestypes
andjson
less importantcore/ordering/cosipbft/*.go
are reasonableThis has been tested with the voting benchmark and shows great promise: 700ms for the 1000th cast, instead of 5.5s with the old blocksync!
Closes c4dt/d-voting#54