Skip to content
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

don't require optional fields importing slashing protection information #4997

Merged
merged 2 commits into from
May 31, 2023

Conversation

tersec
Copy link
Contributor

@tersec tersec commented May 26, 2023

#4149 changed this, but it's unclear why.

Meanwhile, since then:

$ build/nimbus_beacon_node slashingdb import ~/eip3076_example.json
Traceback (most recent call last, using override)
nimbus-eth2/vendor/nim-libp2p/libp2p/stream/bufferstream.nim(507) main
nimbus-eth2/vendor/nim-libp2p/libp2p/stream/bufferstream.nim(500) NimMain
nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(2149) main
nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(2002) handleStartUpCmd
nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(1989) doSlashingInterchange
nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(1962) doSlashingImport
Json load issue for file "eip3076_example.json"
eip3076_example.json(16, -7) Not all required fields were specified when reading 'SPDIR_SignedBlock'

using from https://eips.ethereum.org/EIPS/eip-3076#example-json-instance

{
  "metadata": {
    "interchange_format_version": "5",
    "genesis_validators_root": "0x4b363db94e286120d76eb905340fdd4e54bfe9f06bf33ff6cf5ad27f511bfe95"
  },
  "data": [
    {
      "pubkey": "0xb845089a1457f811bfc000588fbb4e713669be8ce060ea6be3c6ece09afc3794106c91ca73acda5e5457122d58723bed",
      "signed_blocks": [
        {
          "slot": "81952",
          "signing_root": "0x4ff6f743a43f3b4f95350831aeaf0a122a1a392922c45d804280284a69eb850b"
        },
        {
          "slot": "81951"
        }
      ],
      "signed_attestations": [
        {
          "source_epoch": "2290",
          "target_epoch": "3007",
          "signing_root": "0x587d6a4f59a58fe24f406e0502413e77fe1babddee641fda30034ed37ecc884d"
        },
        {
          "source_epoch": "2290",
          "target_epoch": "3008"
        }
      ]
    }
  ]
}

(genesis_validators_root modified for mainnet compatibility for easier testing)

With this PR's change:

$ build/nimbus_beacon_node slashingdb import ~/eip3076_example.json
INF 2023-05-26 21:30:34.479+00:00 No slashing protection data for validator - initiating topics="antislash" validator=b845089a1457f811bfc000588fbb4e713669be8ce060ea6be3c6ece09afc3794106c91ca73acda5e5457122d58723bed
Import finished: 'eip3076_example.json' into '.cache/nimbus/BeaconNode/validators/slashing_protection.sqlite3'

https://eips.ethereum.org/EIPS/eip-3076#json-schema confirms that these signing_root fields are, in fact, optional and not required.

https://github.com/status-im/nimbus-eth2/blob/unstable/tests/slashing_protection/test_fixtures.nim and https://github.com/status-im/nimbus-eth2/blob/unstable/tests/slashing_protection/test_slashing_protection_db.nim test that these schema variations work, but in this case, nimbus_beacon_node has been preventing the slashing protection database import code from ever seeing the JSON.

@github-actions
Copy link

github-actions bot commented May 26, 2023

Unit Test Results

         9 files  ±0    1 071 suites  ±0   34m 23s ⏱️ - 1m 0s
  3 701 tests ±0    3 420 ✔️ ±0  281 💤 ±0  0 ±0 
15 772 runs  ±0  15 458 ✔️ ±0  314 💤 ±0  0 ±0 

Results for commit 31719d4. ± Comparison against base commit 750722d.

♻️ This comment has been updated with latest results.

@zah
Copy link
Contributor

zah commented May 30, 2023

Since requireAllFields honors the optionality of fields using the Option[T] type, another fix is to explicitly mark the optional fields as such. requireAllFields = false is a more dangerous policy that may allow genuinely invalid SPDIR files to be processed. Please note that I haven't looked at the specifics of the SPDIR format before making this comment - I assume that not all fields are optional.

@tersec
Copy link
Contributor Author

tersec commented May 30, 2023

Since requireAllFields honors the optionality of fields using the Option[T] type, another fix is to explicitly mark the optional fields as such. requireAllFields = false is a more dangerous policy that may allow genuinely invalid SPDIR files to be processed. Please note that I haven't looked at the specifics of the SPDIR format before making this comment - I assume that not all fields are optional.

31719d4

Indeed, this marks only optional fields as optional.

The specific stand-in value of 0x0 was what, in practice, it already used when the signing_root field was missing, so this doesn't change the semantics of that case.

@zah zah merged commit bc45892 into unstable May 31, 2023
@zah zah deleted the G07 branch May 31, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants