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
Avoid proposing chains that are longer than max. #206
Conversation
gpbft/participant.go
Outdated
@@ -145,6 +145,8 @@ func (p *Participant) ReceiveAlarm() (err error) { | |||
|
|||
func (p *Participant) beginInstance() error { | |||
chain, power, beacon := p.host.GetCanonicalChain() | |||
// Limit length of the chain to be proposed. | |||
chain = chain.Prefix(CHAIN_MAX_LEN - 1) |
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 chain includes the base, right? This should be CHAIN_MAX_LEN
in that case (no -1
).
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 would be nice to assert that the base matches the end of our previous proposal...
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 Prefix
method adds one: Prefix(0)
is the base chain. Yes perhaps this isn't the ideal semantics, but it makes more sense at some other call sites. I wouldn't object to you overhauling that in another PR, but it's below my threshold at the moment.
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.
Or did you mean that the max length should be CHAIN_MAX_LEN tips plus one for the base?
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.
Ah, no. I misunderstood prefix. LGTM.
15198a7
to
cea0df8
Compare
Closes #203