Use weights to CONVERGE VRF ticket comparison #145
Conversation
ranchalp
force-pushed
the
ranchalp/vrf-ticket
branch
from
064fa28
to
61650d7
Compare
gpbft/gpbft.go
Outdated
| func (c *convergeState) findMinTicketProposal() ECChain { | ||
| var minTicket Ticket | ||
| func (c *convergeState) findMinTicketProposal(table PowerTable) ECChain { | ||
| var minTicket *big.Float |
There was a problem hiding this comment.
I don't think we should be using floats anywhere, even big ones. Lotus has some ticket arithmetic, e.g. in https://github.com/filecoin-project/lotus/blob/master/chain/types/electionproof.go. Can we implement this in integral arithmetic so we don't need to specify floating point behaviour, which may differ in different languages?
There was a problem hiding this comment.
Let us just use the max function then instead of min? This way we just multiply. This must be changed in the FIP though. Using min or max has no impact anywhere else.
There was a problem hiding this comment.
As discussed, max seems fine. You can go back to min if implemented with truncating integer division, too.
gpbft/gpbft.go
Outdated
| @@ -1026,40 +1027,49 @@ type convergeState struct { | |||
| // Chains indexed by head CID | |||
| values map[TipSet]ECChain | |||
| // Tickets provided by proposers of each chain. | |||
| tickets map[TipSet][]Ticket | |||
| tickets map[TipSet]map[ActorID]Ticket | |||
There was a problem hiding this comment.
The inner map is never looked up, only iterated. This suggests it should be a list of tuples instead.
Making it a map also changed the semantics so that an equivocating value from the same sender will be overwritten, rather than both maintained. In this particular case it might be ok to drop a CONVERGE message, but in general throughout, equivocations are both maintained since we don't know which value/s other nodes might have seen. We could change it to preserve the max ticket, but since an upcoming change will use the max ticket with a valid value, that's not 100% safe either.
(There's still the general #12 DOS issue to address, but let's do that systematically elsewhere. If cryptography means a sender can't produce two different tickets then great, we still don't need a map here).
There was a problem hiding this comment.
but in general throughout, equivocations are both maintained since we don't know which value/s other nodes might have seen
Change made, but let me clarify on using maps instead of storing tuples:
We do not need to know what other value/s other nodes might have seen (other nodes justify their messages). Whether we store one vote or multiple equivocating votes from the same sender in the quorum state has no effect on the correctness of the protocol. Keeping equivocating messages makes sense though, probably in a different data structure for a later point in time in order to punish misbehavior, but I would not conflate it with the same data structure we use to reason about strong quorums.
By the proofs, having a map and only considering one message per node does not affect correctness. Termination and progress occurs thanks to reaching a round in which there is synchrony and the max ticket is held by a correct. Also, having differing proposal values at the end of CONVERGE cannot lead to inconsistencies other than not deciding in a round, so it is safe for different correct nodes to disagree on what is the max ticket.
There was a problem hiding this comment.
Thanks for clarifying, and elsewhere where you clarified that the protocol spec and implementation should be changed to explicitly dropping equivocations (tracked as #153). In that case, a map was the right structure after all, but please implement so that a subsequent value from the same actor is dropped rather than overwriting the first one (as this is a consistent policy we can apply everywhere).
ranchalp
force-pushed
the
ranchalp/vrf-ticket
branch
from
afab3bd
to
cb5ea11
Compare
|
@anorth can we get this approved? |
| if value.IsZero() { | ||
| return fmt.Errorf("bottom cannot be justified for CONVERGE") | ||
| } | ||
| key := value.Key() | ||
| c.values[key] = value | ||
| c.tickets[key] = append(c.tickets[key], ticket) | ||
|
|
||
| c.tickets[key] = append(c.tickets[key], ActorTicket{Actor: sender, Ticket: ticket}) |
There was a problem hiding this comment.
Please leave a comment referencing #12 that we need to drop duplicates rather than accumulate an unbounded list of them.
See #144