eth-clients / slashing-protection-interchange-tests

Tests for the EIP-3076 slashing protection interchange format
https://eips.ethereum.org/EIPS/eip-3076
9 stars 6 forks source link

Should we initialize the DB once per test case or once per step? #16

Closed nalepae closed 1 year ago

nalepae commented 1 year ago

Tests specify:

To run a test, first initialize a new (empty) slashing protection database. Then for each entry in steps, import the interchange, process the blocks and attestations, and continue to the next step.

(A test can contain one to multiple steps)

My understanding reading these 2 sentences are:

And so, do not re-initialize the DB at each step.

However, it seems tests themselves contradict each other:

  1. In test Multiple Interchanges Single Validator Single Message Gap:

  2. In test Multiple Interchanges Overlapping Validators Merge Stale:


Or maybe Multiple Interchanges Single Validator Single Message Gap has an issue and these signatures should be allowed?

nalepae commented 1 year ago

I listed all tests with more than 1 step for version v2.5.1.

Test case Is OK with one import per test case? Is OK with one import per step?
multiple_interchanges_single_validator_single_message_gap
multiple_interchanges_multiple_validators_repeat_idem
multiple_interchanges_overlapping_validators_merge_stale
multiple_interchanges_overlapping_validators_repeat_idem
multiple_interchanges_single_validator_fail_iff_imported
multiple_interchanges_single_validator_first_surrounds_second
multiple_interchanges_single_validator_second_surrounds_first
multiple_interchanges_single_validator_single_att_out_of_order
multiple_interchanges_single_validator_single_block_out_of_order

==> My conclusion here is: multiple_interchanges_single_validator_single_message_gap contains errors.

If we have a agreement on this, I can propose a change on this test case.

nalepae commented 1 year ago

If we have a consensus, I can modify multiple_interchanges_single_validator_single_message_gap like this.

michaelsproul commented 1 year ago

One initialisation per test case (not per step) is what was intended. I think the issue in #17 is separate. If we resolve that, then I'll close this issue.

nalepae commented 1 year ago

I think the issue in https://github.com/eth-clients/slashing-protection-interchange-tests/issues/17 is separate.

Yes totally, those 2 issues are separated. That's why I created 2 tickets.

One initialisation per test case (not per step) is what was intended.

Thanks, perfectly clear.


I will focus on the test multiple_interchanges_single_validator_single_message_gap, since it is the only one which may be problematic with "one initialisation per test case".

For pubkey 0xa99a76ed7796f7be22d5b7e85deeb7...

For blocks:

==> At this point, for this pubkey, min(b.slot for b in data.signed_blocks) == 40 So when we process blocks at slots 41, 45 & 49 (steps[1].blocks), should_success should be set to true since no rule is violated.

I agree processing block at slot 50 (steps[1].blocks[3]) should fail, because we already have in our DB a block for this slot, but with no signing root.

For attestations:

==> At this point, already known attestations are:

       2 -----------------> 30
               10 ----------------------------->50

For this key:

==> Importing 3 -> 31 or 9 -> 49 do not break

According to that, I tend to say, for these 2 attestations import, should_success should be set to true.

michaelsproul commented 1 year ago

Ah right, I didn't spot that you were talking about a different test case. I think the fundamental issue is the same as #17: Complete vs Minimal. I'll try to amend this test case per your suggestions when I edit #17

Thanks!