Finalizers can be slashed for legitimate behavior

[scravy]

As discussed in https://github.com/dtr-org/unit-e/pull/852/files#r272530687, it looks like finalizers can be slashed for valid behavior.

#852 even produces a test that succeeds, but apparently should not.

[frolosofsky]

There're two interconnected values in the vote transaction: target_hash and target_epoch. Both are set up in this way:

    m_recommended_target_hash = block_index.GetBlockHash();
    m_recommended_target_epoch = GetEpoch(block_index);

and then these values are propagated to the vote transaction through FinalizationState::GetRecommendedVote().

But on the "server" side we do not have a check that vote.target_epoch and vote.target_hash refers to the same block in the blockchain. We have the check which doesn't guarantee these values consistency.

  if (targetHash != m_recommended_target_hash) {
    return fail(Result::VOTE_WRONG_TARGET_HASH,
                "%s: validator=%s is voting for target=%s instead of the "
                "recommended_target=%s.\n",
                __func__, validatorAddress.GetHex(), targetHash.GetHex(),
                m_recommended_target_hash.GetHex());
  }

We need at least one more check that vote.target_epoch == m_recommended_target_epoch.

The current implementation of double vote detection is also incorrect, I guess.

    // Check for double votes
    const auto recordIt = voteMap.find(vote.m_target_epoch);
    if (recordIt != voteMap.end()) {
      if (recordIt->second.vote.m_target_hash != vote.m_target_hash) {
        return recordIt->second;
      }
    }

This effectively means that to be slashes, finalizer must send double vote with incorrect target hash. At the moment, sending double votes with correct hashes is legit.

I think, the correct behavior is:

• detect inconsistency in target_hash and target_epoch and reject such transaction.
• do not slash when target_hash and target_epoch differs.
• slash when target epochs are same and target hashes are same.

[thothd]

@frolosofsky , to make it clear when looking at the history of #852 and this issue and make sure we converge to clear rules when to slash, we can't see it in this issue since it only refers to a comment and the test code in the PR changed but the discussion @scravy pointed at was about slashing when the target epochs are different, as was also described in the other PR's test.
The suspicion was that we slash for the wrong reasons, then @kostyantyn mentioned that it was actually a slashable condition since it's a surround vote.

You are describing another issue related to the target hash. I wouldn't say it's a definite case of wrong implementation of the slashing mechanism (as with target epoch @scravy mentioned). A suggestion not to slash when the target hash and epoch are different. I understand the idea why we probably shouldn't slash in this case since it looks more like an invalid vote than something that can be processed other than capital punishment. But this case has more room for interpretation.

[kostyantyn]

The reason we have target_epoch besides target_hash is that we don't want to download the entire fork to figure out if we want to slash or not, so we rely on target_epoch in this case.

I think it's fine to slash based on the target_source/target_epoch as this is what finalizer tells you and signs it with his key. If we want to guarantee consistently, it makes more sense then to drop target_epoch and download all the headers to rely on target_hash only. But it increases the data transfer usage.

[frolosofsky]

Ok I realized why do we have target_hash, and the intention of why vote recorder is implemented in this way: we slash double votes on different branches and ignore them on the same branch — we use target hash to distinguish forks! Yeah, I think it's absolutely correct.