unit-e icon indicating copy to clipboard operation
unit-e copied to clipboard

Save votes which are 1 epoch in the future

Open kostyantyn opened this issue 6 years ago • 6 comments
trafficstars

Is your feature request related to a problem? Please describe We don't accept votes which are from the future epoch. It sounds OK as the difference between epochs is 50 blocks or 13 min. However, when the node0 is at the checkpoint and the node1 is at the first block of a new epoch, the difference between epochs is 1 block or 0..16 seconds. In this case, node0 rejects the vote and then won't re-ask again for it.

Here is the test that failed precisely because of this: vote_in_the_future.txt node0 and node1 are at a different epoch height.

Because of this race condition, technically early votes might not be included in the block if the block is proposed by the node which was lagging behind. And even theoretically it's possible that such votes won't be included in the epoch at all as finalizer sent the vote to its peers earlier than they received the first block of a new epoch.

Describe the solution you'd like I think we should save votes which are 1 epoch in the future to make sure we don't drop them because of this "race condition" that the first block of the new epoch was received later than the vote.

Describe alternatives you've considered Alternatively, the node can memorize vote IDs of the future and re-ask them.

Additional context Not related to this issue but we should convert our tests to deterministic ones. And also write a deterministic test that covers exactly this case, when a vote is received earlier than the block of a new epoch.

kostyantyn avatar Feb 20 '19 14:02 kostyantyn

I think we should save votes which are 1 epoch in the future to make sure we don't drop them because of this "race condition" that the first block of the new epoch was received later than the vote after it.

What do you mean by "save votes"? Accept to mempool? Because it sounds like you propose to save them somewhere temporary and process them after that. Could you please clarify it in the description?

frolosofsky avatar Feb 20 '19 15:02 frolosofsky

I haven't checked the implementation of mempool. It might be OK to store it there with some kind of flag that such votes can't be included in the block earlier than it should be.

Alternatively, it can be the different structure which collects such votes and transfers to mempool when we initialize new epoch. I think with the different structure is easier/less error-prone because we don't need to take into account such flag in different scenarios.

kostyantyn avatar Feb 20 '19 15:02 kostyantyn

I do think it's a serious issue since it can affect votes being discarded for no reason, they are indeed unique transactions and should be treated this way. totally makes sense to store them. But I wouldn't use a different structure since we should stick to the existing flows, we've been there before thinking on having a separate mempool for Finalization and Dandelion but it doesn't hold. We probably wanna store all future votes, since a vote is very safe and not something that gonna be spammed by validators

thothd avatar Feb 20 '19 15:02 thothd

The problem is that you are gonna accept those votes without being able to validate them, you are still safe from spam (since a validator can only vote once per given epoch) but at the moment the vote validation logic prompted by ATMP is checking the vote target and so on. If we want to implement this we have to modify this logic to be more permissive.

Gnappuraz avatar Feb 20 '19 16:02 Gnappuraz

I think we can run the same validation as now except the one that checks the target_hash/epoch and re-validate such votes every time we process a new tip.

And we need to make sure that the "softer" version of validation is used only in the mempool but not in the block validation.

kostyantyn avatar Feb 20 '19 16:02 kostyantyn

There is an idea that we can save all the votes, not only the one which is 1 epoch in the future as any valid vote should eventually be included in the block or slashed (because of double-vote) and removed from the mempool. We shouldn't have too many of such votes in the mempool once the node is fully synced as they are created once per epoch/finalizer.

Or another way around, we can start with accepting all the votes and if we find out a good reason when it can be abused, then limit to 1 epoch.

kostyantyn avatar Feb 24 '19 19:02 kostyantyn