From 7966ab6be2b3acf9e17378af23f8da5a4549538f Mon Sep 17 00:00:00 2001
From: henridf <henridf@gmail.com>
Date: Wed, 25 Jan 2023 18:35:46 +0100
Subject: [PATCH] Some EIP4844 fixes (#4549)

* debug log upon sidecar validation failure

* Fill in signature catch upon SignedBeaconBlockAndBlobsSidecar deser

* Always fill blobssidecar slot and root

* Skip lastFCU when eth1monitor is nil

* fix

* Use cached root
---
 .../gossip_processing/block_processor.nim     | 18 +++++----
 beacon_chain/spec/eth2_ssz_serialization.nim  |  4 ++
 beacon_chain/validators/validator_duties.nim  | 39 +++++++++++--------
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/beacon_chain/gossip_processing/block_processor.nim b/beacon_chain/gossip_processing/block_processor.nim
index ffc43928e..722d7a83e 100644
--- a/beacon_chain/gossip_processing/block_processor.nim
+++ b/beacon_chain/gossip_processing/block_processor.nim
@@ -168,7 +168,7 @@ proc storeBackfillBlock(
       when typeof(signedBlock).toFork() >= BeaconBlockFork.EIP4844:
         blobs.isSome() and not
           validate_blobs_sidecar(signedBlock.message.slot,
-                                 hash_tree_root(signedBlock.message),
+                                 signedBlock.root,
                                  signedBlock.message
                                  .body.blob_kzg_commitments.asSeq,
                                  blobs.get()).isOk()
@@ -403,13 +403,15 @@ proc storeBlock*(
   # Establish blob viability before calling addHeadBlock to avoid
   # writing the block in case of blob error.
   when typeof(signedBlock).toFork() >= BeaconBlockFork.EIP4844:
-    if blobs.isSome() and not
-         validate_blobs_sidecar(signedBlock.message.slot,
-                                hash_tree_root(signedBlock.message),
-                                signedBlock.message
-                                .body.blob_kzg_commitments.asSeq,
-                                blobs.get()).isOk():
-       return err((VerifierError.Invalid, ProcessingStatus.completed))
+    if blobs.isSome():
+      let res = validate_blobs_sidecar(signedBlock.message.slot,
+                                       signedBlock.root,
+                                       signedBlock.message
+                                       .body.blob_kzg_commitments.asSeq,
+                                       blobs.get())
+      if res.isErr():
+        debug "blobs sidecar validation failed", err = res.error()
+        return err((VerifierError.Invalid, ProcessingStatus.completed))
 
   type Trusted = typeof signedBlock.asTrusted()
   let blck = dag.addHeadBlock(self.verifier, signedBlock, payloadValid) do (
diff --git a/beacon_chain/spec/eth2_ssz_serialization.nim b/beacon_chain/spec/eth2_ssz_serialization.nim
index d680bfe02..fd44c5d8a 100644
--- a/beacon_chain/spec/eth2_ssz_serialization.nim
+++ b/beacon_chain/spec/eth2_ssz_serialization.nim
@@ -59,6 +59,10 @@ template readSszBytes*(
 template readSszBytes*(
     data: openArray[byte], val: var eip4844.TrustedSignedBeaconBlock, updateRoot = true) =
   readAndUpdateRoot(data, val, updateRoot)
+template readSszBytes*(
+    data: openArray[byte], val: var eip4844.SignedBeaconBlockAndBlobsSidecar, updateRoot = true) =
+  readSszValue(data, val)
+  val.beacon_block.root = hash_tree_root(val.beacon_block.message)
 
 template readSszBytes*(
     data: openArray[byte], val: var auto, updateRoot: bool) =
diff --git a/beacon_chain/validators/validator_duties.nim b/beacon_chain/validators/validator_duties.nim
index 3aaa5db99..c3ba0deb6 100644
--- a/beacon_chain/validators/validator_duties.nim
+++ b/beacon_chain/validators/validator_duties.nim
@@ -879,29 +879,34 @@ proc proposeBlock(node: BeaconNode,
     return head # already logged elsewhere!
 
   var forkedBlck = newBlock.get()
-  var blobs_sidecar = eip4844.BlobsSidecar()
 
   withBlck(forkedBlck):
+    var blobs_sidecar = eip4844.BlobsSidecar(
+      beacon_block_root: hash_tree_root(blck),
+      beacon_block_slot: slot,
+    )
     when blck is eip4844.BeaconBlock and const_preset != "minimal":
-      let
-        lastFcU = node.consensusManager.forkchoiceUpdatedInfo
-        payload_id = bellatrix.PayloadID(lastFcU.get.payloadId)
-        bundle = await getBlobsBundle(node, slot.epoch, validator_index, default(PayloadID))
+      # TODO when lastfcu is none, getExecutionPayload re-queries the EE.
+      # We don't do that here, which could lead us to propose invalid blocks
+      # (with a payload but no blobs).
+      if not (node.eth1Monitor.isNil) and
+         node.consensusManager.forkchoiceUpdatedInfo.isSome():
 
-        # todo: actually compute proof over blobs using nim-kzg-4844
-        kzg_aggregated_proof = default(KZGProof)
+        let
+          lastFcU = node.consensusManager.forkchoiceUpdatedInfo
+          payload_id = bellatrix.PayloadID(lastFcU.get.payloadId)
+          bundle = await getBlobsBundle(node, slot.epoch, validator_index, default(PayloadID))
 
-      blck.body.blob_kzg_commitments =
-          List[eip4844.KZGCommitment, Limit MAX_BLOBS_PER_BLOCK].init(
-            mapIt(bundle.kzgs, eip4844.KzgCommitment(it)))
+          # todo: actually compute proof over blobs using nim-kzg-4844
+          kzg_aggregated_proof = default(KZGProof)
 
-      blobs_sidecar = BlobsSidecar(
-        beacon_block_root: hash_tree_root(blck),
-        beacon_block_slot: slot,
-        blobs: List[eip4844.Blob, Limit MAX_BLOBS_PER_BLOCK].init(
-          mapIt(bundle.blobs, eip4844.Blob(it))),
-        kzg_aggregated_proof: kzg_aggregated_proof
-      )
+        blck.body.blob_kzg_commitments =
+            List[eip4844.KZGCommitment, Limit MAX_BLOBS_PER_BLOCK].init(
+              mapIt(bundle.kzgs, eip4844.KzgCommitment(it)))
+
+        blobs_sidecar.blobs = List[eip4844.Blob, Limit MAX_BLOBS_PER_BLOCK].init(
+          mapIt(bundle.blobs, eip4844.Blob(it)))
+        blobs_sidecar.kzg_aggregated_proof = kzg_aggregated_proof
 
     let
       blockRoot = hash_tree_root(blck)