Remove duplicate vote validation during vote processing

[kostyantyn]

While reviewing logs on a testnet noticed that we have duplicate records, e.g:

2019-04-16 14:32:14 [finalization] ValidateVote: valid. validator=a7f4737d9a64c37a721fff3d360a537d0f51686d target=b67ca892128e553c627c86ee7ee5645e7eec09776beed18f5ab104bf83653dfc source_epoch=18 target_epoch=19
2019-04-16 14:32:14 [finalization] ValidateVote: valid. validator=a7f4737d9a64c37a721fff3d360a537d0f51686d target=b67ca892128e553c627c86ee7ee5645e7eec09776beed18f5ab104bf83653dfc source_epoch=18 target_epoch=19

All these duplicate records generate a redundant noise. And it happens because finalization::RecordVote and esperanza::ContextualCheckVoteTxboth use the same FinalizationState::ValidateVote function.

The solution is to invoke low-level function VoteRecorder::RecordVote instead of finalization::RecordVote from esperanza::ContextualCheckVoteTx which already does the necessary validation.

Signed-off-by: Kostiantyn Stepaniuk kostia@thirdhash.com

[frolosofsky]

I won't return side effects back in the utility "check" function. As a caller of this function you have no intuition that it is gonna change global state. I tend to NACK this proposal.

I think we can adjust logging code instead.

[kostyantyn]

@frolosofsky WDYS if we pass VoteRecorder as a pointer and if it's passed, it will record what must be recorded? Would you accept this design?

[frolosofsky]

I'm not sure I like, but it's bit better. In general, I prefer having small functions with no side effects which do their job well. There is no big difference how to produce side effect: by calling global function or by injecting it via callback/object. For sure, the second approach more flexible but it is still misleading.

Now we have three blocks: check, contextual-check, and record-vote. From their names you can understand what they do and what result produce. Injecting record-vote in contextual-check breaks simple and obvious design we have, and I'm definitely not happy about this change.

If you think that check is a heavy function, and we must avoid calling it twice, we can add caching of its result. This approach will also eliminate double logging.

[kostyantyn]

@frolosofsky introducing is_validated flag in the vote was my 2nd approach in mind :) similar to how CBlockIndex is validated, but this one is a bit more complex as then we need to ensure that we extract the vote once and then pass it down the stream it (not re-fetching it again from the scriptsig).

I think removing duplicated checks/validation will have a positive impact on performance and will make the code more straightforward, so the issue is not only about logs. I'll look at what would be the optimal approach to tackle it and re-work this PR. I also identified the issue with circular dependencies https://github.com/dtr-org/unit-e/pull/999#discussion_r275892615 which I'd like to eliminate too.

[kostyantyn]

@frolosofsky @thothd I am closing this PR, will open another one soon, where will also tackle a circular dependency issue