status-im / nimbus-eth2

Nim implementation of the Ethereum Beacon Chain
https://nimbus.guide
Other
543 stars 233 forks source link

Slashing protection v2 would fail interchange tests under Nim 1.2.10 #2393

Closed tersec closed 3 years ago

tersec commented 3 years ago
diff --git a/beacon_chain/validators/slashing_protection_v2.nim b/beacon_chain/validators/slashing_protection_v2.nim
index 74a50051..26599740 100644
--- a/beacon_chain/validators/slashing_protection_v2.nim
+++ b/beacon_chain/validators/slashing_protection_v2.nim
@@ -991,6 +991,10 @@ proc pruneAttestations*(
   ## Prune all blocks from a validator before the specified newMinSlot
   ## This is intended for interchange import.
   let valID = db.getOrRegisterValidator(validator)
+
+  doAssert newMinSourceEpoch <= high(int64).uint64
+  doAssert newMinTargetEpoch <= high(int64).uint64
+
   let status = db.sqlPruneValidatorAttestations.exec(
     (valID, int64 newMinSourceEpoch, int64 newMinTargetEpoch))
   doAssert status.isOk(),

interchange_test_failures.txt

This came out of https://github.com/status-im/nimbus-eth2/pull/2392.

It's part of https://github.com/status-im/nimbus-eth2/issues/2366 and blocks updating to Nim 1.2.10 because https://github.com/nim-lang/Nim/issues/15210.

mratsim commented 3 years ago

It seems like the failure happens here on line 995

https://github.com/status-im/nimbus-eth2/blob/c0d8ecd7c3951c72880e4fd47fea1740fc1ca34f/beacon_chain/validators/slashing_protection_v2.nim#L984-L1005

For example for the input [FAILED] Slashing test: single_validator_slashable_blocks_no_root.json

{
  "name": "single_validator_slashable_blocks_no_root",
  "genesis_validators_root": "0x0000000000000000000000000000000000000000000000000000000000000000",
  "steps": [
    {
      "should_succeed": true,
      "allow_partial_import": true,
      "interchange": {
        "metadata": {
          "interchange_format_version": "5",
          "genesis_validators_root": "0x0000000000000000000000000000000000000000000000000000000000000000"
        },
        "data": [
          {
            "pubkey": "0xa99a76ed7796f7be22d5b7e85deeb7c5677e88e511e0b337618f8c4eb61349b4bf2d153f649f7b53359fe8b94a38e44c",
            "signed_blocks": [
              {
                "slot": "10"
              },
              {
                "slot": "10"
              }
            ],
            "signed_attestations": []
          }
        ]
      },
      "blocks": [],
      "attestations": []
    }
  ]
}

This is called here when deserializing the interchange format https://github.com/status-im/nimbus-eth2/blob/c0d8ecd7c3951c72880e4fd47fea1740fc1ca34f/beacon_chain/validators/slashing_protection_common.nim#L399-L445

but signed_attestation is empty so it shouldn't have been called at all

mratsim commented 3 years ago

Ah it seems like the line 995 in the attachment throw me off.

So the reason why there is a failure is because I use -1 as a special value here

 var maxValidSourceEpochSeen = -1 
 var maxValidTargetEpochSeen = -1 

And if there is no attestation, this special value ends up here https://github.com/status-im/nimbus-eth2/blob/c0d8ecd7c3951c72880e4fd47fea1740fc1ca34f/beacon_chain/validators/slashing_protection_common.nim#L447-L449

mratsim commented 3 years ago

Closed in #2395