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

Set accum of freshly added validator -(total voting power) #2785

Merged
merged 19 commits into from
Nov 28, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lite/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ If we cannot update directly from H -> H' because there was too much change to
the validator set, then we can look for some Hm (H < Hm < H') with a validator
set Vm. Then we try to update H -> Hm and then Hm -> H' in two steps. If one
of these steps doesn't work, then we continue bisecting, until we eventually
have to externally validate the valdiator set changes at every block.
have to externally validate the validator set changes at every block.

Since we never trust any server in this protocol, only the signatures
themselves, it doesn't matter if the seed comes from a (possibly malicious)
Expand Down
16 changes: 8 additions & 8 deletions state/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,29 +326,29 @@ func updateValidators(currentSet *types.ValidatorSet, abciUpdates []abci.Validat
if err != nil {
return err
}

// these are tendermint types now
totalVotingPower := currentSet.TotalVotingPower()
for _, valUpdate := range updates {
if valUpdate.VotingPower < 0 {
return fmt.Errorf("Voting power can't be negative %v", valUpdate)
}

address := valUpdate.Address
_, val := currentSet.GetByAddress(address)
if valUpdate.VotingPower == 0 {
// remove val
if valUpdate.VotingPower == 0 { // remove val
_, removed := currentSet.Remove(address)
if !removed {
return fmt.Errorf("Failed to remove validator %X", address)
}
} else if val == nil {
// add val
} else if val == nil { // add val
// TODO: issue #1558 update spec according to the following:
// Set Accum to -totalVotingPower to make sure validators can't unbond/rebond to reset their (potentially
melekes marked this conversation as resolved.
Show resolved Hide resolved
// previously negative) Accum to zero.
valUpdate.Accum = -totalVotingPower
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should multiply by a constant (maybe 2 or 3)

Without a constant, it will be advantageous to still unbond / rebond in a lot of scenarios to increase your accum.

Copy link
Contributor Author

@liamsi liamsi Nov 14, 2018

Choose a reason for hiding this comment

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

The scenario is what you describe in #2718 (comment) right?

By using a multiplicative factor, e.g. 2, this would rather mean you can split up in 3 or more validators and do a similar thing you described, is that right? I can change it to 3. And we can do a proper decoupling after launch, OK? (I find such a constant a bit weird though; we need a more solid argument for choosing 2 or 3 explaining the rational behind the choice) @ebuchman @ValarDragon @milosevic @cwgoes

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the attack is mitigated in all cases by using such a constant. (It is not eliminated, solely mitigated) The mitigation comes from unbonding always having a cost for a validator that would normally be proposing, whereas in the described system, there is no cost / it may be profitable to do so anyway. (As suppose your the lowest, then unbonding / rebonding still helps you become not the lowest)

The idea being simulated is when you initially bond, there is a period where you aren't eligible to propose and your accum isn't increasing, but you do contribute to total stake. Perhaps its better to just do that directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would propose to properly write down the math and fix this more complex attack (which involves splitting up validators) in a follow-up PR. How does that sound to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess. I feel like its worked out in the issue, but given the discussion there perhaps I didn't explain it well enough or I'm missing something. Also correctness of the entire thing can't really be proven, since the algorithm itself is quite biasable. (We'd need a careful definition to prove any deterministic algorithm here secure) So at best it can show that it mitigates the gain from this attack. The constant was just meant as an easy way of simulating you not getting to be a proposer for the first n blocks after you bond, so that would still be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, how about multiply by 10 and divide by 9?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm, we can multiply by 1.125 quite efficiently, x + (x >> 3)

Copy link
Contributor

Choose a reason for hiding this comment

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

that works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, x + (x >> 3) works but we'd need to make sure we do not overflow here. vals.TotalVotingPower() is only bound by MaxInt64 (via clipping). Computing x + (x >> 3) could potentially overflow. A consistent way to deal with this would be to clip (again) in the case of an overflow. Alternatively, we could enforce a stricter bound on totalVotingPower.

Choose a reason for hiding this comment

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

added := currentSet.Add(valUpdate)
if !added {
return fmt.Errorf("Failed to add new validator %v", valUpdate)
}
} else {
// update val
} else { // update val
updated := currentSet.Update(valUpdate)
if !updated {
return fmt.Errorf("Failed to update validator %X to %v", address, valUpdate)
Expand Down
4 changes: 2 additions & 2 deletions types/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ func (v *Validator) Hash() []byte {
// as its redundant with the pubkey. This also excludes accum which changes
// every round.
func (v *Validator) Bytes() []byte {
return cdcEncode((struct {
return cdcEncode(struct {
PubKey crypto.PubKey
VotingPower int64
}{
v.PubKey,
v.VotingPower,
}))
})
}

//----------------------------------------
Expand Down