add: columns to `Eth2Processor` and `BlockProcessor` #6862

agnxsh · 2025-01-20T05:56:29Z

No description provided.

github-actions · 2025-01-20T06:29:04Z

Unit Test Results

      15 files ±0   2 285 suites ±0 1h 6m 40s ⏱️ + 1m 28s
  5 368 tests ±0   5 021 ✔️ ±0 347 💤 ±0 0 ❌ ±0
37 069 runs ±0 36 579 ✔️ ±0 490 💤 ±0 0 ❌ ±0

Results for commit 3ea4f12. ± Comparison against base commit 5547d2a.

♻️ This comment has been updated with latest results.

tersec · 2025-01-20T17:33:19Z

beacon_chain/spec/peerdas_helpers.nim

@@ -199,9 +245,62 @@ proc get_data_column_sidecars*(signed_beacon_block: electra.TrustedSignedBeaconB

  sidecars

+# Additional overload to perform reconstruction at the time of gossip


add URL: https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.0/specs/fulu/das-core.md#get_data_column_sidecars

tersec · 2025-01-20T17:35:57Z

beacon_chain/spec/peerdas_helpers.nim

+    return err("DataColumnSidecar: Length should not be 0")
+
+  var
+    columnCount = data_columns.len


can/should be let

tersec · 2025-01-20T17:36:43Z

beacon_chain/spec/peerdas_helpers.nim

+
+  var
+    columnCount = data_columns.len
+    blobCount = data_columns[0].column.len


also can/should be let

tersec · 2025-01-20T17:51:45Z

beacon_chain/spec/peerdas_helpers.nim

+  for column_index in 0..<NUMBER_OF_COLUMNS:
+    var
+      column_cells: seq[KzgCell]
+      column_proofs: seq[KzgProof]


Both of these could use newSeqOfCap with cellsAndProofs.len

beacon_chain/validators/message_router.nim

tersec · 2025-01-20T18:26:21Z

beacon_chain/validators/message_router.nim

+
+        let kzgCommits =
+          signedBlock.message.body.blob_kzg_commitments.asSeq
+        if dataColumns.len > 0 and kzgCommits.len > 0:


This doesn't seem to actually use kzgCommits though, aside from this check?

(Also, if it's just checking len, it shouldn't need asSeq)

tersec · 2025-01-20T19:28:38Z

beacon_chain/validators/message_router.nim

+      for dc in data_columns:
+        if dc.index in custody_columns:
+          final_columns.add dc
+      dataColumnRefs = Opt.some(final_columns.mapIt(newClone(it)))


Why even create the non-ref final_columns to begin with, only to mapIt immediately?

Once you know you're going to set dataColumnRefs = Opt.some ..., can build it up in place.

tersec · 2025-01-22T13:36:56Z

beacon_chain/nimbus_beacon_node.nim

@@ -1269,6 +1303,12 @@ func getSyncCommitteeSubnets(node: BeaconNode, epoch: Epoch): SyncnetBits =

  subnets + node.getNextSyncCommitteeSubnets(epoch)

+func readCustodyGroupSubnets*(node: BeaconNode): uint64=
+  var res = CUSTODY_REQUIREMENT.uint64


This whole function can be as

func readCustodyGroupSubnets(node: BeaconNode): uint64 = if node.config.peerdasSupernode: NUMBER_OF_CUSTODY_GROUPS.uint64 else: CUSTODY_REQUIREMENT.uint64

without this setting of a maybe incorrect value first.

Also, not sure why it's exported. It's only used in the same module in which it's defined:

beacon_chain/nimbus_beacon_node.nim 1306:func readCustodyGroupSubnets*(node: BeaconNode): uint64= 1349: targetSubnets = node.readCustodyGroupSubnets()

cheatfate · 2025-01-24T12:28:39Z

beacon_chain/beacon_chain_file.nim

@@ -79,6 +79,8 @@ const
    int(ConsensusFork.Phase0) .. int(high(ConsensusFork))
  BlobForkCodeRange =
    MaxForksCount .. (MaxForksCount + int(high(ConsensusFork)) - int(ConsensusFork.Deneb))
+  DataColumnForkCodeRange =
+    MaxForksCount .. (MaxForksCount + int(high(ConsensusFork)) - int(ConsensusFork.Fulu))


This is incorrect range because it overlaps with Blobs, the idea is
[blocks range][blobs range][columns range] ... high(int)
Allocated range for blocks is [0 .. 16383]
Allocated range for blobs is [16384 .. 32767]
So for blobs you should allocate [32768 .. 49151].

DataColumnForkCodeRange = (BlobForkCodeRange.high + 1) .. (BlobForkCodeRange.high + 1 + int(high(ConsensusFork)) - int(ConsensusFork.Fulu))

around this value?

cheatfate · 2025-01-24T12:29:15Z

beacon_chain/beacon_chain_file.nim

+func getDataColumnForkCode(fork: ConsensusFork): uint64 =
+  case fork
+  of ConsensusFork.Fulu:
+    uint64(MaxForksCount)


Because of invalid code range it provides invalid codes which overlaps with blobs range.

cheatfate · 2025-01-24T12:47:11Z

beacon_chain/sync/sync_queue.nim

@@ -132,6 +135,27 @@ proc getShortMap*[T](req: SyncRequest[T],
      res.add('|')
  res

+proc getShortMap*[T](req: SyncRequest[T],
+                     data: openArray[ref DataColumnSidecar]): string =


Looks like this code is based on old and ugly version of blobs map, but it was recently changed to more suitable map format. So the idea is to show how blobs being distributed over the range request, so now it could look like
12.............................., which means that for request range there was returned 1 blob for first block in range and 2 blobs for second one, all other blocks are without blobs, it looks like you still using MAX_BLOBS_PER_BLOCK constant (i'm not sure is it appropriate) but if it so - please change this procedure to new version.

I'm also confused about the usage of MAX_BLOBS_PER_BLOCK. It seems like NUMBER_OF_COLUMNS (https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.1/specs/fulu/p2p-interface.md#configuration for example) is more relevant.

beacon_chain/validators/message_router.nim

tersec · 2025-01-25T14:55:57Z

beacon_chain/sync/sync_queue.nim

+                                  data: openArray[Slot]):
+                                  Result[void, cstring] =
+  if data.len == 0:
+    # Impossible to verify empty response


Maybe a nit, I guess? But it's clearly not impossible, since this function does it (affirmatively).

I'd call this something akin to https://en.wikipedia.org/wiki/Vacuous_truth -- maybe vacuously valid.

tersec · 2025-01-25T14:58:57Z

beacon_chain/sync/sync_queue.nim

+
+  static: doAssert MAX_BLOBS_PER_BLOCK_ELECTRA >= MAX_BLOBS_PER_BLOCK
+
+  if lenu64(data) > (req.count * MAX_BLOBS_PER_BLOCK_ELECTRA):


According to https://github.com/ethereum/consensus-specs/blob/v1.5.0-beta.1/specs/fulu/p2p-interface.md#datacolumnsidecarsbyrange-v1 the correct check is:

The response MUST contain no more than count * NUMBER_OF_COLUMNS data column sidecars.

None of the MAX_BLOBS_PER_BLOCK_ELECTRA/MAX_BLOBS_PER_BLOCK checks apply here.

tersec · 2025-01-25T14:59:16Z

beacon_chain/sync/sync_queue.nim

+      return err("incorrect order")
+    if slot == pSlot:
+      inc counter
+      if counter > MAX_BLOBS_PER_BLOCK_ELECTRA:


See also NUMBER_OF_COLUMNS

tersec · 2025-01-25T15:14:15Z

beacon_chain/nimbus_beacon_node.nim

@@ -429,6 +429,17 @@ proc initFullNode(
      node.network.nodeId.resolve_column_sets_from_custody_groups(
          max(SAMPLES_PER_SLOT.uint64,
          localCustodyGroups))
+    custody_columns_list =
+      node.network.nodeId.resolve_column_list_from_custody_groups(
+          max(SAMPLES_PER_SLOT.uint64,


This max(SAMPLES_PER_SLOT.uint64, localCustodyGroups) computation occurs 3 times in 11 lines of code. Maybe better to extract it once.

tersec · 2025-01-25T15:22:44Z

beacon_chain/nimbus_beacon_node.nim

+    custody = node.network.nodeId.get_custody_groups(max(SAMPLES_PER_SLOT.uint64,
+                                                     targetSubnets.uint64))
+
+  for i in 0'u64..<NUMBER_OF_CUSTODY_GROUPS:


Why not just iterate directly through custody? This retains the ordering but that's not important for node.network.subscribe.

tersec · 2025-01-25T15:43:25Z

beacon_chain/gossip_processing/block_processor.nim

@@ -642,6 +726,24 @@ proc storeBlock(
            msg = r.error()
          return err((VerifierError.Invalid, ProcessingStatus.completed))

+  elif typeof(signedBlock).kind >= ConsensusFork.Fulu:


nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 712 to 745 in 5f169d9

when typeof(signedBlock).kind >= ConsensusFork.Deneb:

if blobsOpt.isSome:

let blobs = blobsOpt.get()

let kzgCommits = signedBlock.message.body.blob_kzg_commitments.asSeq

if blobs.len > 0 or kzgCommits.len > 0:

let r = validate_blobs(kzgCommits, blobs.mapIt(KzgBlob(bytes: it.blob)),

blobs.mapIt(it.kzg_proof))

if r.isErr():

debug "blob validation failed",

blockRoot = shortLog(signedBlock.root),

blobs = shortLog(blobs),

blck = shortLog(signedBlock.message),

kzgCommits = mapIt(kzgCommits, shortLog(it)),

signature = shortLog(signedBlock.signature),

msg = r.error()

return err((VerifierError.Invalid, ProcessingStatus.completed))

elif typeof(signedBlock).kind >= ConsensusFork.Fulu:

if dataColumnsOpt.isSome:

let columns = dataColumnsOpt.get()

let kzgCommits = signedBlock.message.body.blob_kzg_commitments.asSeq

if columns.len > 0 and kzgCommits.len > 0:

for i in 0..<columns.len:

let r =

verify_data_column_sidecar_kzg_proofs(columns[i][])

if r.isErr:

malformed_cols.add(i)

debug "data column validation failed",

blockRoot = shortLog(signedBlock.root),

column_sidecar = shortLog(columns[i][]),

blck = shortLog(signedBlock.message),

signature = shortLog(signedBlock.signature),

msg = r.error()

return err((VerifierError.Invalid, ProcessingStatus.completed))

This has the same ConsensusFork.Deneb/ConsensusFork.Fulu check order issue as the other case, where the Fulu one will never run.

tersec · 2025-01-25T15:55:44Z

beacon_chain/gossip_processing/block_processor.nim

+            MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,
+            Opt.none(DataColumnSidecars))
+
+        var columnsOk = true


nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 597 to 639 in 5f169d9

let

parent_root = signedBlock.message.parent_root

parentBlck = dag.getForkedBlock(parent_root)

if parentBlck.isSome():

var blobsOk = true

let blobs =

withBlck(parentBlck.get()):

when consensusFork >= ConsensusFork.Deneb:

var blob_sidecars: BlobSidecars

for i in 0 ..< forkyBlck.message.body.blob_kzg_commitments.len:

let blob = BlobSidecar.new()

if not dag.db.getBlobSidecar(parent_root, i.BlobIndex, blob[]):

blobsOk = false # Pruned, or inconsistent DB

break

blob_sidecars.add blob

Opt.some blob_sidecars

else:

Opt.none BlobSidecars

if blobsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,

Opt.none(DataColumnSidecars))

var columnsOk = true

let columns =

withBlck(parentBlck.get()):

when consensusFork >= ConsensusFork.Fulu:

var data_column_sidecars: DataColumnSidecars

for i in self.dataColumnQuarantine[].custody_columns:

let data_column = DataColumnSidecar.new()

if not dag.db.getDataColumnSidecar(parent_root, i.ColumnIndex, data_column[]):

columnsOk = false

break

data_column_sidecars.add data_column

Opt.some data_column_sidecars

else:

Opt.none DataColumnSidecars

if columnsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), Opt.none(BlobSidecars),

columns)

It looks like:

for a pre-Deneb block (still potentially relevant while genesis syncing, for example) or a Deneb/Electra block with no blobs (less common than before, but it happens), the blobsOk loop will be vacuously valid (no blobs means no invalid blobs, and blobsOk is initialized to true), so

nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 617 to 619 in 5f169d9

self[].enqueueBlock(

MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,

Opt.none(DataColumnSidecars))

will run, but because the columnsOk-based check separately/independently enqueues blocks and has the same vacuously-valid logic,

nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim

Lines 635 to 637 in 5f169d9

if columnsOk:

debug "Loaded parent block from storage", parent_root

self[].enqueueBlock(

will enqueueBlock(...) the same block again.

In general, I think but haven't enumerated all cases that for the usual valid-blobs-or-columns case (either) the vacuous-truth aspect means that it will typically double-enqueueBlock things (blobs but not columns: the columns check is vacuously true, and the blobs check might be, if the blobs are valid).

Actually, this is even more -- it means that because blocks will have one or the other, all blocks will be either vacuously valid here for either blobs or columns and get enqueued at least once, so this opens a security bypass in block_processor.

agnxsh marked this pull request as draft January 20, 2025 05:56

tersec reviewed Jan 20, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Jan 20, 2025

View reviewed changes

tersec reviewed Jan 22, 2025

View reviewed changes

cheatfate reviewed Jan 24, 2025

View reviewed changes

tersec reviewed Jan 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

agnxsh added 16 commits January 25, 2025 10:54

init: columns to the block/eth2 processor

6eec230

add columns to message router

e67ee1a

add columns to initializers of Eth2 and BlockProcessor

d30e893

save progress

1f222c6

save progress 2

632f7a2

add column to block verifier

eac6fa1

save progress, need to rework untrusted syncing

a5d0362

add column support to light forward sync

24397c8

save progress test sync manager

5ae6bf1

fix createDataColumns

00d8322

fix more

444b915

added fulu message handlers for column subnets

f46733a

activated data column sidecar processing at Fulu

eba1f76

fix compilation issues

3f9edd3

added to T list

7104108

other fixes

45fa2fa

agnxsh added 7 commits January 25, 2025 10:54

fix test

c2f8ea5

fix result situation in get data column sidecars

1196e54

fix message router issue

26e2fee

gate blob publishing upto deneb

c86cef3

fix message router blob and column progressions

8369661

drop dataColumnOpt from message router

4052e6a

reversing rman blockVerifier order

b31706e

agnxsh force-pushed the columns branch from 6db36ca to b31706e Compare January 25, 2025 05:24

agnxsh added 4 commits January 25, 2025 11:11

fixes

512a740

several fixes

a38be17

added debug logs for devnet testing

947a12f

add blobsOpt isSome check

5f169d9

tersec reviewed Jan 25, 2025

View reviewed changes

fix copyright years

3ea4f12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: columns to `Eth2Processor` and `BlockProcessor` #6862

add: columns to `Eth2Processor` and `BlockProcessor` #6862

agnxsh commented Jan 20, 2025

github-actions bot commented Jan 20, 2025 •

edited

Loading

tersec Jan 20, 2025

tersec Jan 20, 2025

tersec Jan 20, 2025

tersec Jan 20, 2025

tersec Jan 20, 2025

tersec Jan 20, 2025

tersec Jan 22, 2025 •

edited

Loading

tersec Jan 22, 2025

cheatfate Jan 24, 2025

agnxsh Jan 24, 2025

cheatfate Jan 24, 2025

cheatfate Jan 24, 2025

tersec Jan 25, 2025

tersec Jan 25, 2025 •

edited

Loading

tersec Jan 25, 2025

tersec Jan 25, 2025

tersec Jan 25, 2025

tersec Jan 25, 2025

tersec Jan 25, 2025

tersec Jan 25, 2025

		@@ -199,9 +245,62 @@ proc get_data_column_sidecars*(signed_beacon_block: electra.TrustedSignedBeaconB

		sidecars

		# Additional overload to perform reconstruction at the time of gossip


		static: doAssert MAX_BLOBS_PER_BLOCK_ELECTRA >= MAX_BLOBS_PER_BLOCK

		if lenu64(data) > (req.count * MAX_BLOBS_PER_BLOCK_ELECTRA):

	when typeof(signedBlock).kind >= ConsensusFork.Deneb:
	if blobsOpt.isSome:
	let blobs = blobsOpt.get()
	let kzgCommits = signedBlock.message.body.blob_kzg_commitments.asSeq
	if blobs.len > 0 or kzgCommits.len > 0:
	let r = validate_blobs(kzgCommits, blobs.mapIt(KzgBlob(bytes: it.blob)),
	blobs.mapIt(it.kzg_proof))
	if r.isErr():
	debug "blob validation failed",
	blockRoot = shortLog(signedBlock.root),
	blobs = shortLog(blobs),
	blck = shortLog(signedBlock.message),
	kzgCommits = mapIt(kzgCommits, shortLog(it)),
	signature = shortLog(signedBlock.signature),
	msg = r.error()
	return err((VerifierError.Invalid, ProcessingStatus.completed))

	elif typeof(signedBlock).kind >= ConsensusFork.Fulu:
	if dataColumnsOpt.isSome:
	let columns = dataColumnsOpt.get()
	let kzgCommits = signedBlock.message.body.blob_kzg_commitments.asSeq
	if columns.len > 0 and kzgCommits.len > 0:
	for i in 0..<columns.len:
	let r =
	verify_data_column_sidecar_kzg_proofs(columns[i][])
	if r.isErr:
	malformed_cols.add(i)
	debug "data column validation failed",
	blockRoot = shortLog(signedBlock.root),
	column_sidecar = shortLog(columns[i][]),
	blck = shortLog(signedBlock.message),
	signature = shortLog(signedBlock.signature),
	msg = r.error()
	return err((VerifierError.Invalid, ProcessingStatus.completed))

	let
	parent_root = signedBlock.message.parent_root
	parentBlck = dag.getForkedBlock(parent_root)
	if parentBlck.isSome():
	var blobsOk = true
	let blobs =
	withBlck(parentBlck.get()):
	when consensusFork >= ConsensusFork.Deneb:
	var blob_sidecars: BlobSidecars
	for i in 0 ..< forkyBlck.message.body.blob_kzg_commitments.len:
	let blob = BlobSidecar.new()
	if not dag.db.getBlobSidecar(parent_root, i.BlobIndex, blob[]):
	blobsOk = false # Pruned, or inconsistent DB
	break
	blob_sidecars.add blob
	Opt.some blob_sidecars
	else:
	Opt.none BlobSidecars
	if blobsOk:
	debug "Loaded parent block from storage", parent_root
	self[].enqueueBlock(
	MsgSource.gossip, parentBlck.unsafeGet().asSigned(), blobs,
	Opt.none(DataColumnSidecars))

	var columnsOk = true
	let columns =
	withBlck(parentBlck.get()):
	when consensusFork >= ConsensusFork.Fulu:
	var data_column_sidecars: DataColumnSidecars
	for i in self.dataColumnQuarantine[].custody_columns:
	let data_column = DataColumnSidecar.new()
	if not dag.db.getDataColumnSidecar(parent_root, i.ColumnIndex, data_column[]):
	columnsOk = false
	break
	data_column_sidecars.add data_column
	Opt.some data_column_sidecars
	else:
	Opt.none DataColumnSidecars
	if columnsOk:
	debug "Loaded parent block from storage", parent_root
	self[].enqueueBlock(
	MsgSource.gossip, parentBlck.unsafeGet().asSigned(), Opt.none(BlobSidecars),
	columns)

add: columns to Eth2Processor and BlockProcessor #6862

Are you sure you want to change the base?

add: columns to Eth2Processor and BlockProcessor #6862

Conversation

agnxsh commented Jan 20, 2025

github-actions bot commented Jan 20, 2025 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tersec Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tersec Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

add: columns to `Eth2Processor` and `BlockProcessor` #6862

add: columns to `Eth2Processor` and `BlockProcessor` #6862

github-actions bot commented Jan 20, 2025 •

edited

Loading

tersec Jan 22, 2025 •

edited

Loading

tersec Jan 25, 2025 •

edited

Loading