✅ Add optimistic sync tests #3489

wenceslas-sanchez · 2023-08-23T19:20:22Z

The goal of this PR is to add more cases for optimistic sync feature.

Test TODO:

Global TODO:

refactoring
doc

wenceslas-sanchez · 2023-08-23T19:26:45Z

Hi @hwwhww and @mkalinin,

I tried to implement the first case from this issue; which is in my case the below:

3 branches are syncing
A is the one with the most attestation, C the one with the few
A is the optimistic head
A is set to invalid => B become the optimistic head
B is set to invalid => C become the optimistic head

Does it match what you asked ? I am not sure the way I am manipulating weights is the best way to do it (I used the parameter participation_fn in the method state_transition_with_full_block. Also the test is not well written (a lot of duplicated code etc.); If you are ok with what I implemented, I will revert with a clean version of this.

…k position)

mkalinin · 2023-08-24T09:01:25Z

The test looks good in general but I am not sure that participation_fn will add an attestation on chain. I would probable use on_attestation handler to add attestations to the test vector so a client will be able to apply them to its FC store.

wenceslas-sanchez · 2023-08-27T16:07:52Z

The test looks good in general but I am not sure that participation_fn will add an attestation on chain. I would probable use on_attestation handler to add attestations to the test vector so a client will be able to apply them to its FC store.

I did manage attestation instead of participation indices; but it can't run with mainnet config because of the function get_committee_count_per_slot that cap the number of committee indices to 1. I think to make it works on the mainnet configuration, we could manage manually each validator indices from each committee to attest this or that branch.

wenceslas-sanchez · 2023-08-29T19:35:35Z

The test looks good in general but I am not sure that participation_fn will add an attestation on chain. I would probable use on_attestation handler to add attestations to the test vector so a client will be able to apply them to its FC store.

I did manage attestation instead of participation indices; but it can't run with mainnet config because of the function get_committee_count_per_slot that cap the number of committee indices to 1. I think to make it works on the mainnet configuration, we could manage manually each validator indices from each committee to attest this or that branch.

Now it run whatever the preset + added a test with equal weight (1 validator attestation per branch)

mkalinin · 2023-09-01T09:02:05Z

@wenceslas-sanchez is this PR ready for review or are you expecting to do more changes to it?

wenceslas-sanchez · 2023-09-01T09:03:13Z

@wenceslas-sanchez is this PR ready for review or are you expecting to do more changes to it?

Nothing more than code refactoring, so you can review it.

mkalinin

Test scenarios look good to me! 👍
I haven't looked deep into implementation design though, I hope @hwwhww will do that 😜

mkalinin · 2023-09-01T13:50:04Z

tests/core/pyspec/eth2spec/test/bellatrix/sync/test_optimistic.py

@@ -112,3 +114,209 @@ def test_from_syncing_to_invalid(spec, state):
    assert mega_store.opt_store.head_block_root == signed_blocks_a[-1].message.hash_tree_root()

    yield 'steps', test_steps
+
+
+def add_attestation_and_sign_block_with_aggregation_bit_list(spec, state, store, block, index, aggregation_bit_list):


This function should be moved to one of the helper packages I believe

Isn't this method too specific to what I am testing ? For instance I complete the aggregation_bits full of zeros to match the expected list size.

I think this method can potentially be reused in other tests, cc @hwwhww

Slightly inclined to move to fork_choice.py.

tests/core/pyspec/eth2spec/test/bellatrix/sync/test_optimistic.py

hwwhww · 2023-09-11T03:29:27Z

Sorry for the late reply! Great work @wenceslas-sanchez! I'm now picking up the reviews after the long conference weeks. 🙏

There was an unrelated issue (#3495) that broke our CI. I fixed it and the linter errors. But now we can see that the test_multiple_branches_sync_all_invalidated_but_one_equal_weight test case failed with capella, eip6110, and eip7002 forks, but succeeded with bellatrix and deneb. My intuition is somehow there is a tie.

wenceslas-sanchez · 2023-09-12T20:21:48Z

Sorry for the late reply! Great work @wenceslas-sanchez! I'm now picking up the reviews after the long conference weeks. 🙏

There was an unrelated issue (#3495) that broke our CI. I fixed it and the linter errors. But now we can see that the test_multiple_branches_sync_all_invalidated_but_one_equal_weight test case failed with capella, eip6110, and eip7002 forks, but succeeded with bellatrix and deneb. My intuition is somehow there is a tie.

It seems that you are right, and that's because each preset/fork generates a different spec.Root(signed_block.message.hash_tree_root()). Below is the result per fork/preset.

	Bellatrix	Capella	Deneb	EIP6110
Minimal	✅	❌	✅	❌
Mainnet	❌	❌	❌	❌

(It's seems to have a lot of red cross, but sometimes it happens at the first assertion, and sometimes to the second.)

What we can do to manage this:

build a test per fork/preset (not elegant)
since we know the value of spec.Root(signed_block.message.hash_tree_root()), we can set the assertion following the hash tree root orders.
build one unique test for one fork and one/two preset.

I will try to revert very soon with the second solution.

wenceslas-sanchez · 2023-09-12T20:32:39Z

Sorry for the late reply! Great work @wenceslas-sanchez! I'm now picking up the reviews after the long conference weeks. 🙏
There was an unrelated issue (#3495) that broke our CI. I fixed it and the linter errors. But now we can see that the test_multiple_branches_sync_all_invalidated_but_one_equal_weight test case failed with capella, eip6110, and eip7002 forks, but succeeded with bellatrix and deneb. My intuition is somehow there is a tie.

It seems that you are right, and that's because each preset/fork generates a different spec.Root(signed_block.message.hash_tree_root()). Below is the result per fork/preset.

Bellatrix Capella Deneb EIP6110
Minimal ✅ ❌ ✅ ❌
Mainnet ❌ ❌ ❌ ❌
(It's seems to have a lot of red cross, but sometimes it happens at the first assertion, and sometimes to the second.)

What we can do to manage this:

build a test per fork/preset (not elegant)

since we know the value of spec.Root(signed_block.message.hash_tree_root()), we can set the assertion following the hash tree root orders.

build one unique test for one fork and one/two preset.

I will try to revert very soon with the second solution.

Ok I think I found a solution but it's not super elegant.. what do you think ?

hwwhww · 2023-09-15T07:38:32Z

tests/core/pyspec/eth2spec/test/bellatrix/sync/test_optimistic.py

@@ -112,3 +114,209 @@ def test_from_syncing_to_invalid(spec, state):
    assert mega_store.opt_store.head_block_root == signed_blocks_a[-1].message.hash_tree_root()

    yield 'steps', test_steps
+
+
+def add_attestation_and_sign_block_with_aggregation_bit_list(spec, state, store, block, index, aggregation_bit_list):


Slightly inclined to move to fork_choice.py.

hwwhww · 2023-09-16T05:48:32Z

tests/core/pyspec/eth2spec/test/bellatrix/sync/test_optimistic.py

+        data=attestation_data,
+    )
+    sign_attestation(spec, state, attestation)
+    spec.on_attestation(store, attestation, is_from_block=True)


It's implicit (sorry!), but I made all add_* helpers that yield something from the fork choice spec on_* helpers. If we yield here, it will output the attestation SSZ file to the test vectors.

In the given test cases, I think we don't need the spec.on_attestation call because the block will include the attestations?

If so, we should:

remove this line

rename add_attestation_and_sign_block_with_aggregation_bit_list to sign_block_with_aggregation_bit_list

If not, we should:

replace this line with fork_choice::add_attestation helper

yield from add_attestation(spec, store, attestation, test_steps)

replace signed_block = add_attestation_and_sign_block_with_aggregation_bit_list with signed_block = yield from add_attestation_and_sign_block_with_aggregation_bit_list

Oh okay understoood, I did rename the method + remove the on_attestation call. I also moved it to fork_choice.py

… minimal spec

✅ add multiple invalid branch but one test

9c9d41a

wenceslas-sanchez closed this Aug 23, 2023

🐛 use the index j (linked to branch) instead of i (linkde to the bloc…

126fcc8

…k position)

wenceslas-sanchez reopened this Aug 23, 2023

wenceslas-sanchez closed this Aug 23, 2023

♻️ add attestation instead of managing particpation indices

b754989

wenceslas-sanchez reopened this Aug 27, 2023

wenceslas-sanchez added 3 commits August 27, 2023 18:11

💡 comment the last proportion

789b9d6

🔥 remove test code

d02df0a

✅ test on both minimal and mainnet + add equal weight test

d88d820

wenceslas-sanchez added 2 commits August 29, 2023 21:42

🐛 remove used import

5134fbc

✅ invert branch C and B to make clearer the optimistic head switch

11f777f

mkalinin approved these changes Sep 1, 2023

View reviewed changes

hwwhww added 2 commits September 11, 2023 10:49

Merge branch 'dev' into pr3489

67427d3

Make linter happy

a0bb838

✅ add dynamic chain head check

f1adc04

hwwhww reviewed Sep 16, 2023

View reviewed changes

wenceslas-sanchez added 4 commits September 18, 2023 15:31

🔧 find attestation bit list configuration that match both mainnet and…

647d018

… minimal spec

🎨 rename method

45873fb

🔥 useless usage of on_attestation

88f4325

🚚 move sign_block_with_aggregation_bit_list to fork_choice.py

0273ffa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✅ Add optimistic sync tests #3489

✅ Add optimistic sync tests #3489

wenceslas-sanchez commented Aug 23, 2023 •

edited

Loading

wenceslas-sanchez commented Aug 23, 2023

mkalinin commented Aug 24, 2023

wenceslas-sanchez commented Aug 27, 2023

wenceslas-sanchez commented Aug 29, 2023

mkalinin commented Sep 1, 2023

wenceslas-sanchez commented Sep 1, 2023

mkalinin left a comment

mkalinin Sep 1, 2023

wenceslas-sanchez Sep 3, 2023 •

edited

Loading

mkalinin Sep 4, 2023

hwwhww Sep 15, 2023

hwwhww commented Sep 11, 2023

wenceslas-sanchez commented Sep 12, 2023

wenceslas-sanchez commented Sep 12, 2023

hwwhww Sep 15, 2023

hwwhww Sep 16, 2023 •

edited

Loading

wenceslas-sanchez Sep 18, 2023

✅ Add optimistic sync tests #3489

Are you sure you want to change the base?

✅ Add optimistic sync tests #3489

Conversation

wenceslas-sanchez commented Aug 23, 2023 • edited Loading

wenceslas-sanchez commented Aug 23, 2023

mkalinin commented Aug 24, 2023

wenceslas-sanchez commented Aug 27, 2023

wenceslas-sanchez commented Aug 29, 2023

mkalinin commented Sep 1, 2023

wenceslas-sanchez commented Sep 1, 2023

mkalinin left a comment

Choose a reason for hiding this comment

mkalinin Sep 1, 2023

Choose a reason for hiding this comment

wenceslas-sanchez Sep 3, 2023 • edited Loading

Choose a reason for hiding this comment

mkalinin Sep 4, 2023

Choose a reason for hiding this comment

hwwhww Sep 15, 2023

Choose a reason for hiding this comment

hwwhww commented Sep 11, 2023

wenceslas-sanchez commented Sep 12, 2023

wenceslas-sanchez commented Sep 12, 2023

hwwhww Sep 15, 2023

Choose a reason for hiding this comment

hwwhww Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

wenceslas-sanchez Sep 18, 2023

Choose a reason for hiding this comment

wenceslas-sanchez commented Aug 23, 2023 •

edited

Loading

wenceslas-sanchez Sep 3, 2023 •

edited

Loading

hwwhww Sep 16, 2023 •

edited

Loading