Make VoteRecorder independent DB storage

[kostyantyn]

This commit makes the following changes:

1. ~VoteRecorder is now a simple DB without a logic about slashing. It is responsible to store votes and provide function to check for offending ones. FinalizationState is responsible to call VoteRecorder when it's appropriate and create slash transactions when needed. Having this one-way relationship removes circular dependency~ Fixed via https://github.com/dtr-org/unit-e/pull/1019

   Circular dependency: esperanza/finalizationstate -> validation -> finalization/vote_recorder -> esperanza/finalizationstate

2. FinalizationState validates the vote only once (used to be twice on master) and eliminates the following duplicate log.

   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

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

[kostyantyn]

@frolosofsky

It adds weird functionality which has nothing common with things finalization state is supposed to do

FinalizationState is responsible now to process all the commits, I found it reasonable that it takes care of slashing too.

Besides, I put a lot of effort to remove any global states and relationships from here,

On master, we still have a global state for VoteRecorder. This PR doesn't introduce a new one. However, with this change, it's now easier to add it as DI to FinalizationState. Was planning to do it in a follow-up PR.

I don't see your NACK is well elaborated. You say that you don't like it and its weird. This PR solves 2 concrete problems. VoteRecorder is used globally (same as in master) but if I put more effort to convert it to DI, what is the concrete issue you see when FinalizerState is calling VoteRecorder and not another way around. Could you, please, elaborate on this more?

[frolosofsky]

FinalizationState is responsible now to process all the commits, I found it reasonable that it takes care of slashing too.

It takes care of slash commits on the master too. This PR mixes responsibilities from different areas in one single point: finalization state. Instead, finalization state must be a simple class that represents the finalization from particular block point of view.

On master, we still have a global state for VoteRecorder. This PR doesn't introduce a new one. However, with this change, it's now easier to add it as DI to FinalizationState. Was planning to do it in a follow-up PR.

Please read carefully: I removed any global states and relationships from FinalizationState. I don't see how it can be easier now to create component, honestly. You GetComponent<VoteRecorder>()->RecordVote() from validation.cpp or from finalizationstate.cpp, what is the difference? But again, my claim is not about DI at all, it's about injecting alien responsibilities to the finalization state. The fact that you call this function from outside of the state, and the only positive effect of calling this is a side effect, also says a lot. It's alien, it must not live in FinalizationState.

I don't see your NACK is well elaborated. You say that you don't like it and its weird. This PR solves 2 concrete problems. VoteRecorder is used globally (same as in master) but if I put more effort to convert it to DI, what is the concrete issue you see when FinalizerState is calling VoteRecorder and not another way around. Could you, please, elaborate on this more?

Hope, I elaborated bit more above. Also, in the first comment I pointed out how the first issue could be fixed simply in like 10 lines diff change.

[kostyantyn]

@frolosofsky I see we disagree which object calls which, it's simple as that, we either have: FinalizationState -> VoteRecorder or VoteRecorder -> FinalizationState.

I encourage you to work on your idea (but which also fixes circular dependency and double-validation), open a PR and we as a team will pick one or another approach and will maintain it.

[kostyantyn]

@AM5800 @Gnappuraz VoteRecorder could be passed as a parameter to one of validation function and be optional, but I see we are strongly against having any mentioning of VoteRecorder in the FinalizationState. That's good.

I'd like to see a working alternative which doesn't do double-validation for votes. Or if we say double-validation with double-logging is a good alternative (what we have in master), then we can close this PR.

[frolosofsky]

Circular dependencies have been removed in #1019.

As for double validating, I for one, do not see any harm here.

@kostyantyn This PR is a second try to silently replace finalization state we use in vote recording, I'm worrying about that a bit. Maybe you could factor out this change in a separate PR so that we can discuss and (hopefully) accept it?

[kostyantyn]

@frolosofsky yes, the circular dependency issue is fixed. Crossed out that point.

About opening a separate PR, I see we tend to accept double-validation for votes. Let's get one more feedback from @AM5800 or @Gnappuraz if they are on the same side and then we can simply close this PR instead of coming up with an alternative approach/design.

[Gnappuraz]

RecordVote calls ValidateVote that does a simple set of checks ( no sig check involved ), so I would not worry too much. The reason for the double check is that the VoteRecorded actually is fine recording stuff that is not valid as long as is parseable and well-formed. We could get rid of the double validation, passing more information out of the CheckContextualFinalizationCommits and using that to decide to record the vote or not, and removing then the check from the RecordVote method. Still NACK for me on this specific implementation on the same grounds as before.

[kostyantyn]

Closing it as don't have time to work on an alternative approach now.