Issue in `multiple_interchanges_single_validator_multiple_blocks_out_of_order`?

[nalepae]

In test multiple_interchanges_single_validator_multiple_blocks_out_of_order, the label should_success of the last block is set to false, as we can see here.

What rule this block proposal does not comply with?

I suspect

"slot": "29"

should be replaced by

"slot": "20"

If we have a consensus, I can propose a PR with this file.

[nalepae]

Maybe the finger slipped?

[michaelsproul]

What rule this block proposal does not comply with?

It's rule #2:

Refuse to sign any block with slot <= min(b.slot for b in data.signed_blocks if b.pubkey == proposer_pubkey), except if it is a repeat signing as determined by the signing_root.

There is a slot 30 block in the first interchange that's imported, so the block at slot 29 should not be signed:

https://github.com/eth-clients/slashing-protection-interchange-tests/blob/658f7196b0bf82419ba2d69aff55bc9ce66f3ddc/tests/generated/multiple_interchanges_single_validator_multiple_blocks_out_of_order.json#L38-L43

[nalepae]

Thanks for you answer!

There is a slot 30 block in the first interchange that's imported,

It's not what I understand actually.

Fur pubkey 0xa99a76ed7796f7be22d5b7e...

• In the first interchange that's imported (at steps[0].interchange.data), we import only one block, at slot 0.
• Then, in steps[0].blocks, we process 3 blocks: At slots 10, 20 & 30.

The slot 30 block does not come from the first interchange that's imported, but from blocks processed after importing the first interchange. (But, in fine, it does not change the issue IMO.)

==> After only importing steps[0].interchange.data, we already know that, for that pubkey, min(b.slot for b in data.signed_blocks) == 0

So, when we try to process block at slot 29 (steps[1].blocks[0]), we definitly have 29 > min(b.slot for b in data.signed_blocks) (since min(b.slot for b in data.signed_blocks) == 0) for this pubkey.

==> Importing block at slot 29 does no brake the rule #2:

Refuse to sign any block with slot <= min(b.slot for b in data.signed_blocks if b.pubkey == proposer_pubkey), except if it is a repeat signing as determined by the signing_root.

Or there is something else I don't get?

[michaelsproul]

Then, in steps[0].blocks, we process 3 blocks: At slots 10, 20 & 30.

Ah yeah, sorry, I missed that.

We are slightly outside the domain of the EIP-3076 spec here, because it only specifies how to handle signed messages with respect to an interchange file. It is arguably valid to use either of the following strategies when signing extra blocks after importing a file:

• Complete: record every signed message, and do not sign anything that is slashable wrt to them.
• Minimal: record only high watermarks for signed block slot, source epoch and target epoch, and use the EIP-3076 rules to prevent signing of anything that violates these minimums.

The test case as currently written assumes Minimal while I guess your implementation assumes Complete. I tried to make the spec & tests compatible with both strategies, which is why the test README includes this paragraph:

Clients using a slashing protection mechanism that admits false positives may consider a rejection as success even if should_succeed is true.

I think this is probably the semantics we should rely on here: set should_succeed: true for the block at slot 29 so that the Complete impls can sign it, and then allow the Minimal impls to reject it. The test generator lives inside Lighthouse, which previously used a Complete strategy but switched to Minimal. I suspect that test case was added after we switched to minimal, so the discrepancy was never spotted. The relevant test is here:

https://github.com/sigp/lighthouse/blob/441fc1691b69f9edc4bbdc6665f3efab16265c9b/validator_client/slashing_protection/src/bin/test_generator.rs#L227-L239

As far as I know most other implementations in-the-wild use minimal, because it is simpler than the complete strategy, more space efficient, and has virtually no downsides (it is rarely useful to be able to go back and sign an old message between ones you've already signed).

[michaelsproul]

I'll have a go at re-working multiple_interchanges_single_validator_multiple_blocks_out_of_order and multiple_interchanges_single_validator_single_message_gap, and generate some new tests for you to run. If you can confirm they work for you then we can cut a release and ensure they pass on the other clients too. I suspect some re-working of the test runners in the other clients might be required, as it seems we were assuming properties of the Minimal strategy.

[nalepae]

The test case as currently written assumes Minimal while I guess your implementation assumes Complete.

Oh it is exactly the source of the confusion!

To give you a little bit more of context:

• Currently Prysm implements the complete DB
• My final purpose is to implement the minimal DB in Prysm as well
• However, currently Prysm does not use the latest version of this EIP-3076 tests. So, before starting the implementation of the minimal DB, I wanted to be sure all tests of the latest version of EIP-3076 were green as well.
• I tried to update the tests to the latest version, I now I have an issue with the slot 29 of multiple_interchanges_single_validator_multiple_blocks_out_of_order, precisely because the test assumes a minimal DB is used, while Prysm uses a complete DB.

Clients using a slashing protection mechanism that admits false positives may consider a rejection as success even if should_succeed is true.

In this specific case, we are in the opposite state: should_success is actually set to false.

If you can confirm they work for you then we can cut a release and ensure they pass on the other clients too.

Sure with pleasure! Or maybe an other solution is to have: Idea 1:

• 1 directory with tests both compatible with minimal AND complete slashing DB (the very big majority of them)
• 1 directory with tests specific to minimal DB
• 1 directory with tests specific to complete DB

Or an other idea would be to have 2 should_succeed fields: Idea 2.1:

          ...
          "signing_root": "0x0000000000000000000000000000000000000000000000000000000000000000",
          "should_succeed_minimal": true
          "should_succeed_complete": false

The downside of Idea 2.1 is that it will brake the current compatibility

Idea 2.2:

          ...
          "signing_root": "0x0000000000000000000000000000000000000000000000000000000000000000",
          "should_succeed": true
          "should_succeed_complete": false

So:

• should_succeed is implicitly for minimal DB.
• should_succeed_complete is for complete DB.

==> Idea 2.2 does not break the current compatibility.

[nalepae]

Actually the more I'm thinking about it, the more I guess tests should really rely on a net an clear should_succeed state, without having to _may consider a rejection as success even if shouldsucceed is true.

I guess Idea 2.2 is a good compromise between back-compatibility and distinction between complete and minimal anti-slashing DB.

[nalepae]

Implementation of Idea 2.2: https://github.com/sigp/lighthouse/pull/4958