-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable snapshot and finalizer commits integration #852
Conversation
b84fff0
to
b16975d
Compare
4c6e0c5
to
c938268
Compare
Rebased with master, updated test to epoch=1 feature. Picked updated #856. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConceptACK 473e9ee
We need to extend LookupFinalizedBlockIndex
https://github.com/dtr-org/unit-e/blob/master/src/snapshot/p2p_processing.cpp#L19-L28 and check that the snapshot hash is part of the finalized epoch.
We should also extend feature_snapshot.py if we see it's not enough. The added feature_snapshot_finalization.py
seems to duplicate the same functionality.
In p2p_snapshot.py
we check snapshot sending on a P2P level and trying to catch edge-cases. Would be good to extend test_invalid_snapshot
https://github.com/dtr-org/unit-e/blob/master/test/functional/p2p_snapshot.py#L452 and provide the snapshot which is not from the finalized epoch.
473e9ee
to
7d21e75
Compare
It does not duplicate. It tests snapshot and commits integration, i.e., checks that finalization state transfer complete and node can operate after fast sync, e.g., accept votes and process finalization state further. |
@frolosofsky we don't have 2 ways to sync snapshot. We used headers first, now we switched to commits first, so the initial test needs to be adjusted. From the description of that test we cover:
At the time of writing that test, we didn't have commits so we didn't assert finalizationstate but now we do have so let's add |
@kostyantyn It's not enough to add I'm not a big fan of having all-in-one tests. Let's split that test on three files and then merge |
c65c646
to
680a00b
Compare
Extended feature_snapshot_finalization with a checks that test double votes conditions during fast-sync. |
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
7b4070d
to
a1dfe94
Compare
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
Signed-off-by: Stanislav Frolov <stanislav@thirdhash.com>
LOCK(dummyNode1.cs_vSend); | ||
peerLogic->SendMessages(&dummyNode1, 0, 1, interruptDummy); // should result in getheaders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, why the current changes impacted on this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a race condition detected in this unit test after adding https://github.com/dtr-org/unit-e/pull/852/files#diff-eff7adeaec73a769788bb78858815c91R3273
|
||
return generated_blocks | ||
|
||
@classmethod | ||
def generate_epoch(cls, proposer, finalizer, count=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest keeping this function in the current test feature_snapshot_finalization.py
because it's not generic enough to use in multiple scenarios and then work on it separately.
- it won't work correctly if we are at height=0 as we will generate 4 blocks (we will be at the checkpoint) and node can't vote at a checkpoint
- It can be useful to provide the list of finalizers and then everyone must vote. I see a few examples then when this function can be used.
- I'd like to control where the votes are included. In most of tests, we vote after the 1st block, here we vote at checkpoint-1.
- Would expect to return votes of my finalizer(s) but not all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be used in #883. I used similar algorithms in couple of tests already.
it won't work correctly if we are at height=0 as we will generate 4 blocks (we will be at the checkpoint) and node can't vote at a checkpoint
It can be useful to provide the list of finalizers and then everyone must vote. I see a few examples then when this function can be used.
Thanks, I will fix it when move this function al long as wait_for_vote_and_disconnect
to the util.py. Let's focus on this in different PR.
I'd like to control where the votes are included. In most of tests, we vote after the 1st block, here we vote at checkpoint-1.
If you like to control, you probably write your code explicitly. This function is a variation of generatetoaddress(X * EPOCH_LENGTH)
which ensures finalizer is in time to vote for every epoch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d90c996
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 120b1e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConceptACK 120b1e2
This PR adds integration code for snapshot and finalizer commits that effectively enables fast-sync with minor restrictions (#852 (review)). On the first step of fast-sync, node restores finalization state by downloading finalizer commits from peers. On the second step, node downloads UTXO snapshot from peers and then downloads last missing blocks.
TODO:
Chapter in the #836 story.