From 2d9447825d68ec0ec4b856f2d48772e818c78678 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Mon, 18 Sep 2023 22:39:14 +0200 Subject: [PATCH 1/7] add blob validation to `test_fixture_fork_choice` Preparation for processing new tests from: - https://github.com/ethereum/consensus-specs/pull/3463 --- .../test_fixture_fork_choice.nim | 64 ++++++++++++++----- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index a3bd37879e..7eb146f67b 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -9,7 +9,7 @@ import # Status libraries - stew/results, chronicles, + stew/[byteutils, results], chronicles, taskpools, # Internals ../../beacon_chain/spec/[helpers, forks, state_transition_block], @@ -28,7 +28,7 @@ import from std/json import JsonNode, getBool, getInt, getStr, hasKey, items, len, pairs, `$`, `[]` -from std/sequtils import toSeq +from std/sequtils import mapIt, toSeq from std/strutils import contains # Test format described at https://github.com/ethereum/consensus-specs/tree/v1.3.0/tests/formats/fork_choice @@ -45,6 +45,10 @@ type opInvalidateRoot opChecks + BlobData = object + blobs: seq[KzgBlob] + proofs: seq[KzgProof] + Operation = object valid: bool # variant specific fields @@ -55,6 +59,7 @@ type att: Attestation of opOnBlock: blck: ForkedSignedBeaconBlock + blobData: Opt[BlobData] of opOnMergeBlock: powBlock: PowBlock of opOnAttesterSlashing: @@ -69,7 +74,7 @@ from ../../beacon_chain/spec/datatypes/capella import BeaconBlock, BeaconState, SignedBeaconBlock from ../../beacon_chain/spec/datatypes/deneb import - BeaconBlock, BeaconState, SignedBeaconBlock + KzgBlob, KzgProof, BeaconBlock, BeaconState, SignedBeaconBlock proc initialLoad( path: string, db: BeaconChainDB, @@ -134,6 +139,8 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = result = @[] for step in steps[0]: + var numExtraFields = 0 + if step.hasKey"tick": result.add Operation(kind: opOnTick, tick: step["tick"].getInt()) @@ -147,12 +154,14 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = att: att) elif step.hasKey"block": let filename = step["block"].getStr() + doAssert step.hasKey"blobs" == step.hasKey"proofs" case fork of ConsensusFork.Phase0: let blck = parseTest( path/filename & ".ssz_snappy", SSZ, phase0.SignedBeaconBlock ) + doAssert not step.hasKey"blobs" result.add Operation(kind: opOnBlock, blck: ForkedSignedBeaconBlock.init(blck)) of ConsensusFork.Altair: @@ -160,6 +169,7 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = path/filename & ".ssz_snappy", SSZ, altair.SignedBeaconBlock ) + doAssert not step.hasKey"blobs" result.add Operation(kind: opOnBlock, blck: ForkedSignedBeaconBlock.init(blck)) of ConsensusFork.Bellatrix: @@ -167,6 +177,7 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = path/filename & ".ssz_snappy", SSZ, bellatrix.SignedBeaconBlock ) + doAssert not step.hasKey"blobs" result.add Operation(kind: opOnBlock, blck: ForkedSignedBeaconBlock.init(blck)) of ConsensusFork.Capella: @@ -174,15 +185,27 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = path/filename & ".ssz_snappy", SSZ, capella.SignedBeaconBlock ) + doAssert not step.hasKey"blobs" result.add Operation(kind: opOnBlock, blck: ForkedSignedBeaconBlock.init(blck)) of ConsensusFork.Deneb: - let blck = parseTest( - path/filename & ".ssz_snappy", - SSZ, deneb.SignedBeaconBlock - ) + let + blck = parseTest( + path/filename & ".ssz_snappy", + SSZ, deneb.SignedBeaconBlock) + blobData = + if step.hasKey"blobs": + numExtraFields += 2 + Opt.some BlobData( + blobs: distinctBase(parseTest( + path/(step["blobs"].getStr()) & ".ssz_snappy", + SSZ, List[KzgBlob, Limit MAX_BLOBS_PER_BLOCK])), + proofs: step["proofs"].mapIt(KzgProof.fromHex(it.getStr()))) + else: + Opt.none(BlobData) result.add Operation(kind: opOnBlock, - blck: ForkedSignedBeaconBlock.init(blck)) + blck: ForkedSignedBeaconBlock.init(blck), + blobData: blobData) elif step.hasKey"attester_slashing": let filename = step["attester_slashing"].getStr() let attesterSlashing = parseTest( @@ -205,10 +228,10 @@ proc loadOps(path: string, fork: ConsensusFork): seq[Operation] = doAssert false, "Unknown test step: " & $step if step.hasKey"valid": - doAssert step.len == 2 + doAssert step.len == 2 + numExtraFields result[^1].valid = step["valid"].getBool() elif not step.hasKey"checks" and not step.hasKey"payload_status": - doAssert step.len == 1 + doAssert step.len == 1 + numExtraFields result[^1].valid = true proc stepOnBlock( @@ -218,10 +241,21 @@ proc stepOnBlock( state: var ForkedHashedBeaconState, stateCache: var StateCache, signedBlock: ForkySignedBeaconBlock, + blobData: Opt[BlobData], time: BeaconTime, invalidatedRoots: Table[Eth2Digest, Eth2Digest]): Result[BlockRef, VerifierError] = - # 1. Move state to proper slot. + # 1. Validate blobs + when typeof(signedBlock).toFork() >= ConsensusFork.Deneb: + let kzgCommits = signedBlock.message.body.blob_kzg_commitments.asSeq + if kzgCommits.len > 0 or blobData.isSome: + if blobData.isNone or kzgCommits.validate_blobs( + blobData.get.blobs, blobData.get.proofs).isErr: + return err(VerifierError.Invalid) + else: + doAssert blobData.isNone, "Pre-Deneb test with specified blob data" + + # 2. Move state to proper slot doAssert dag.updateState( state, dag.getBlockIdAtSlot(time.slotOrZero).expect("block exists"), @@ -229,7 +263,7 @@ proc stepOnBlock( stateCache ) - # 2. Add block to DAG + # 3. Add block to DAG when signedBlock is phase0.SignedBeaconBlock: type TrustedBlock = phase0.TrustedSignedBeaconBlock elif signedBlock is altair.SignedBeaconBlock: @@ -272,12 +306,12 @@ proc stepOnBlock( blckRef: BlockRef, signedBlock: TrustedBlock, epochRef: EpochRef, unrealized: FinalityCheckpoints): - # 3. Update fork choice if valid + # 4. Update fork choice if valid let status = fkChoice[].process_block( dag, epochRef, blckRef, unrealized, signedBlock.message, time) doAssert status.isOk() - # 4. Update DAG with new head + # 5. Update DAG with new head var quarantine = Quarantine.init() let newHead = fkChoice[].get_head(dag, time).get() dag.updateHead(dag.getBlockRef(newHead).get(), quarantine, []) @@ -377,7 +411,7 @@ proc doRunTest(path: string, fork: ConsensusFork) = let status = stepOnBlock( stores.dag, stores.fkChoice, verifier, state[], stateCache, - blck, time, invalidatedRoots) + blck, step.blobData, time, invalidatedRoots) doAssert status.isOk == step.valid of opOnAttesterSlashing: let indices = From 298d54f9148e3d16e901e46911dec782ccc10d0b Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 15:01:05 +0200 Subject: [PATCH 2/7] load trusted setup --- tests/consensus_spec/test_fixture_fork_choice.nim | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index 7eb146f67b..6d43227a06 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -470,5 +470,8 @@ template fcSuite(suiteName: static[string], testPathElem: static[string]) = for kind, path in walkDir(basePath, relative = true, checkDir = true): runTest(suiteName, basePath/path, fork) +from ../../beacon_chain/conf import loadKzgTrustedSetup +doAssert loadKzgTrustedSetup().isOk # Required for Deneb tests + fcSuite("ForkChoice", "fork_choice") fcSuite("Sync", "sync") From 2a18ca5dd35f063f9e8ed0f6fe872a6377c538c6 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 15:14:23 +0200 Subject: [PATCH 3/7] skip broken test --- tests/consensus_spec/test_fixture_fork_choice.nim | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index 6d43227a06..a68b241aaf 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -440,6 +440,10 @@ proc runTest(suiteName: static[string], path: string, fork: ConsensusFork) = "too_late_for_merge", "block_lookup_failed", "all_valid", + + # 1.4.0-beta.2: Broken test, misses `tick` + # - https://github.com/ethereum/consensus-specs/pull/3463 + "simple_blob_data", ] test suiteName & " - " & path.relativePath(SszTestsDir): From 152e7d86561eca0a65068a24178b5bc7d3708154 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 19:04:36 +0200 Subject: [PATCH 4/7] Revert "skip broken test" This reverts commit 2a18ca5dd35f063f9e8ed0f6fe872a6377c538c6. --- tests/consensus_spec/test_fixture_fork_choice.nim | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index a68b241aaf..6d43227a06 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -440,10 +440,6 @@ proc runTest(suiteName: static[string], path: string, fork: ConsensusFork) = "too_late_for_merge", "block_lookup_failed", "all_valid", - - # 1.4.0-beta.2: Broken test, misses `tick` - # - https://github.com/ethereum/consensus-specs/pull/3463 - "simple_blob_data", ] test suiteName & " - " & path.relativePath(SszTestsDir): From ad30f001156ceccebdd733879da69c7d396ced69 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 21:27:56 +0200 Subject: [PATCH 5/7] `loadKzgTrustedSetup().isOk` inside `doAssert` fails on `minimal`... --- tests/consensus_spec/test_fixture_fork_choice.nim | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index 6d43227a06..0330709a58 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -471,7 +471,8 @@ template fcSuite(suiteName: static[string], testPathElem: static[string]) = runTest(suiteName, basePath/path, fork) from ../../beacon_chain/conf import loadKzgTrustedSetup -doAssert loadKzgTrustedSetup().isOk # Required for Deneb tests +let res = loadKzgTrustedSetup() # Required for Deneb tests +doAssert res.isOk fcSuite("ForkChoice", "fork_choice") fcSuite("Sync", "sync") From 98dbda99d4aa8af5145238b2457586d19dcd944c Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 22:09:27 +0200 Subject: [PATCH 6/7] load trusted setup on demand --- tests/consensus_spec/test_fixture_fork_choice.nim | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/consensus_spec/test_fixture_fork_choice.nim b/tests/consensus_spec/test_fixture_fork_choice.nim index 0330709a58..e0fc77d7ab 100644 --- a/tests/consensus_spec/test_fixture_fork_choice.nim +++ b/tests/consensus_spec/test_fixture_fork_choice.nim @@ -471,8 +471,7 @@ template fcSuite(suiteName: static[string], testPathElem: static[string]) = runTest(suiteName, basePath/path, fork) from ../../beacon_chain/conf import loadKzgTrustedSetup -let res = loadKzgTrustedSetup() # Required for Deneb tests -doAssert res.isOk +discard loadKzgTrustedSetup() # Required for Deneb tests fcSuite("ForkChoice", "fork_choice") fcSuite("Sync", "sync") From 1bd5287abea4f4925fb6fb9f14f9c0e08001bea3 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Tue, 19 Sep 2023 23:47:45 +0200 Subject: [PATCH 7/7] set `FIELD_ELEMENTS_PER_BLOB=4` for `make consensus_spec_tests_minimal` --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e8f07e01a2..94cb0e9a75 100644 --- a/Makefile +++ b/Makefile @@ -321,7 +321,7 @@ consensus_spec_tests_minimal: | build deps MAKE="$(MAKE)" V="$(V)" $(ENV_SCRIPT) scripts/compile_nim_program.sh \ $@ \ "tests/consensus_spec/consensus_spec_tests_preset.nim" \ - $(NIM_PARAMS) -d:const_preset=minimal $(TEST_MODULES_FLAGS) && \ + $(NIM_PARAMS) -d:const_preset=minimal -d:FIELD_ELEMENTS_PER_BLOB=4 $(TEST_MODULES_FLAGS) && \ echo -e $(BUILD_END_MSG) "build/$@" # Tests we only run for the default preset