search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Configure on which block of the epoch a finalizer starts voting #909

Closed kostyantyn closed 5 years ago

kostyantyn commented 5 years ago

This PR introduces -finalizervotefromepochblocknumber parameter that allows configuring on which block of the epoch finalizer can start voting.

It can make sense to start voting a bit later than after the checkpoint is processed as if re-orgs happens on a checkpoint and finalizer switches to that fork, it can't vote on this epoch again as it will be double-voting. Delaying the vote reduces the chance of voting on the fork that won't be final for the finalizer but delaying for too long can risk that vote won't be included by proposer at all.

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

Ruteri commented 5 years ago

I would like to see a test as follows (case from the testnet):

Looking at the code I think the validator will vote while doing the reorg.

kostyantyn commented 5 years ago

@Ruteri correct, we check that we start voting at a particular point. Not voting is part of the vote recorder; it must detect double/surrounded votes and prevent from voting.

~I'll take care of writing such test and will open a separate PR as I believe it's better to do independently because if we discover the bug, it's not related to this feature. WDYS?~

OK, after reviewing and discovering an issue, this PR can't be merged before we fix the re-org problem. I'll extend this PR but if we see it's hard to read then will split PRs.

kostyantyn commented 5 years ago

@frolosofsky correct, it only mitigates the issue as it is less likely we will keep switching after 2/3 of blocks are processed rather than after the 1st block is processed. But if that happens then bad luck, it will be counted as finalizer didn't vote. Everyone who runs the finalizer, will be able to decide when to vote depends on internal metrics.

scravy commented 5 years ago

Since there is a command line option defined for it (-finalizervoteonepochblocknumber) this seems to be a user-definable setting and as such should not go to blockchain::Parameters but to Settings. If you want to define default settings per network you can do that in the default settings in blockchain::Parameters, but it is a user definable setting.

blockchain Parameters are consensus critical values which always come with validation and are the same for everyone on the same network. They can nevertheless be injected in testnet and regtest using the injectable/configurable chainparameters. This mechanism is generic.

frolosofsky commented 5 years ago

Would be great to have a test that shows how 

finalizer_vote_from_epoch_block_number = 35

is better than

finalizer_vote_from_epoch_block_number = 1;

Otherwise this PR looks like an attempt to fix non-existent issue.

kostyantyn commented 5 years ago

@frolosofsky we have a separate PR which shows how finalizer behaves during re-orgs https://github.com/dtr-org/unit-e/pull/914

This PR doesn't fix an issue. It delays voting that can be convenient to do so to reduce the chance that finalizer votes are not included in the epoch. We will use testnet to figure out what is the optimal default value. For now, we picked 35 because of 50*2/3=33.333... and we rounded to the nicer number.

scravy commented 5 years ago

We actually have research on that, I think @gfanti and @Gnappuraz know more about this. There is if I'm not mistaken a good strategy for the network as a whole and throughput and everything for a time for a validator/finalizer to vote. This is a step in that right direction.

frolosofsky commented 5 years ago

utACK 3ae6a425ed6c8b1c2f1f5a07d5a50616f5dd2cfb

We actually have research on that, I think @gfanti and @Gnappuraz know more about this. There is if I'm not mistaken a good strategy for the network as a whole and throughput and everything for a time for a validator/finalizer to vote. This is a step in that right direction.

Thanks, it is what I was asking for. If we do have a research, simulation, or whatever that proves this PR, it is fine to go. @kostyantyn Btw, would be nice to attach a reference to the research to the PR description.

kostyantyn commented 5 years ago

Because of conflicts with the master, had to rebase and force-push

thothd commented 5 years ago

utACK 3ae6a42

We actually have research on that, I think @gfanti and @Gnappuraz know more about this. There is if I'm not mistaken a good strategy for the network as a whole and throughput and everything for a time for a validator/finalizer to vote. This is a step in that right direction.

Thanks, it is what I was asking for. If we do have a research, simulation, or whatever that proves this PR, it is fine to go. @kostyantyn Btw, would be nice to attach a reference to the research to the PR description.

It's not really based on research but on voting deep enough approaching the end, if there's a reorg larger than this testnet default (35) we're in a very bad network state anyway, in this case you'll just lose your vote. it's a default and the client (each validator) can tweak it further, it's a decentralized network after all. similar to how you wanna split / combine your outputs or other aspects as an active participant