Skip to content

Commit d207850

Browse files
cheatfatecheatfate
authored
Refactor fetchMissingColumnsByRoot() and add tests. (#7410)
1 parent eca062a commit d207850

File tree

3 files changed

+103
-150
lines changed

3 files changed

+103
-150
lines changed

‎beacon_chain/consensus_object_pools/blob_quarantine.nim‎

Lines changed: 19 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -636,85 +636,20 @@ func fetchMissingSidecars*(
636636
blockRoot: Eth2Digest,
637637
blck: fulu.SignedBeaconBlock,
638638
peerCustodyColumns: openArray[ColumnIndex] = []
639-
): seq[DataColumnIdentifier] =
640-
## Function returns sequence of DataColumnIdentifier's for data columns which
641-
## are missing for block associated with root ``blockRoot`` and block ``blck``.
642-
var res: seq[DataColumnIdentifier]
643-
let record = quarantine.roots.getOrDefault(blockRoot)
644-
645-
if len(blck.message.body.blob_kzg_commitments) == 0:
646-
# Fast-path if block do not have any columns
647-
return res
648-
649-
let
650-
supernode = (len(quarantine.custodyColumns) == NUMBER_OF_COLUMNS)
651-
columnsCount =
652-
if supernode:
653-
(NUMBER_OF_COLUMNS div 2 + 1)
654-
else:
655-
len(quarantine.custodyColumns)
656-
657-
if supernode:
658-
let
659-
columns =
660-
if len(peerCustodyColumns) > 0:
661-
@peerCustodyColumns
662-
else:
663-
quarantine.custodyColumns
664-
if len(record.sidecars) == 0:
665-
var columnsRequested = 0
666-
for column in columns:
667-
if columnsRequested >= columnsCount:
668-
# We don't need to request more than (NUMBER_OF_COLUMNS div 2 + 1)
669-
# columns.
670-
break
671-
res.add(DataColumnIdentifier(block_root: blockRoot, index: column))
672-
inc(columnsRequested)
673-
else:
674-
if record.count >= columnsCount:
675-
return res
676-
var columnsRequested = 0
677-
for column in columns:
678-
if record.count + columnsRequested >= columnsCount:
679-
# We don't need to request more than (NUMBER_OF_COLUMNS div 2 + 1)
680-
# columns.
681-
break
682-
let index = quarantine.getIndex(column)
683-
if (index == -1) or record.sidecars[index].isEmpty():
684-
res.add(DataColumnIdentifier(block_root: blockRoot, index: column))
685-
inc(columnsRequested)
686-
else:
687-
let peerMap =
688-
if len(peerCustodyColumns) > 0:
689-
ColumnMap.init(peerCustodyColumns)
690-
else:
691-
ColumnMap.init(quarantine.custodyColumns)
692-
if len(record.sidecars) == 0:
693-
for column in (peerMap and quarantine.custodyMap).items():
694-
res.add(DataColumnIdentifier(block_root: blockRoot, index: column))
695-
else:
696-
for column in (peerMap and quarantine.custodyMap).items():
697-
let index = quarantine.getIndex(column)
698-
if (index == -1) or record.sidecars[index].isEmpty():
699-
res.add(DataColumnIdentifier(block_root: blockRoot, index: column))
700-
res
701-
702-
func fetchMissingColumnsByRoot*(
703-
quarantine: ColumnQuarantine,
704-
blockRoot: Eth2Digest,
705-
blck: fulu.SignedBeaconBlock,
706-
peerCustodyColumns: openArray[ColumnIndex] = []
707-
): seq[DataColumnsByRootIdentifier] =
708-
## Function returns a sequence of DataColumnsByRootIdentifier for data columns
709-
## which are missing for the block associated with root ``blockRoot`` and block ``blck``.
710-
var
711-
res: seq[DataColumnsByRootIdentifier]
712-
missingIndices: DataColumnIndices
639+
): DataColumnsByRootIdentifier =
640+
## Function returns a DataColumnsByRootIdentifier for data columns
641+
## which are missing for the block associated with root ``blockRoot`` and
642+
## block ``blck``.
643+
##
644+
## Note: If there is no missing columns - DataColumnByRootIdentifier.indices
645+
## array will be empty.
646+
var res: seq[ColumnIndex]
713647
let record = quarantine.roots.getOrDefault(blockRoot)
714648

715649
if len(blck.message.body.blob_kzg_commitments) == 0:
716650
# Fast-path if block does not have any columns
717-
return res
651+
return DataColumnsByRootIdentifier(
652+
block_root: blockRoot, indices: DataColumnIndices(res))
718653

719654
let
720655
supernode = (len(quarantine.custodyColumns) == NUMBER_OF_COLUMNS)
@@ -738,11 +673,13 @@ func fetchMissingColumnsByRoot*(
738673
# We don't need to request more than (NUMBER_OF_COLUMNS div 2 + 1)
739674
# columns.
740675
break
741-
discard missingIndices.add(column)
676+
res.add(column)
742677
inc(columnsRequested)
743678
else:
744679
if record.count >= columnsCount:
745-
return res
680+
return
681+
DataColumnsByRootIdentifier(
682+
block_root: blockRoot, indices: DataColumnIndices(res))
746683
var columnsRequested = 0
747684
for column in columns:
748685
if record.count + columnsRequested >= columnsCount:
@@ -751,7 +688,7 @@ func fetchMissingColumnsByRoot*(
751688
break
752689
let index = quarantine.getIndex(column)
753690
if (index == -1) or record.sidecars[index].isEmpty():
754-
discard missingIndices.add(column)
691+
res.add(column)
755692
inc(columnsRequested)
756693
else:
757694
let peerMap =
@@ -761,16 +698,15 @@ func fetchMissingColumnsByRoot*(
761698
ColumnMap.init(quarantine.custodyColumns)
762699
if len(record.sidecars) == 0:
763700
for column in (peerMap and quarantine.custodyMap).items():
764-
discard missingIndices.add(column)
701+
res.add(column)
765702
else:
766703
for column in (peerMap and quarantine.custodyMap).items():
767704
let index = quarantine.getIndex(column)
768705
if (index == -1) or (record.sidecars[index].isEmpty()):
769-
discard missingIndices.add(column)
706+
res.add(column)
770707

771-
if missingIndices.len > 0:
772-
res.add(DataColumnsByRootIdentifier(block_root: blockRoot, indices: missingIndices))
773-
res
708+
DataColumnsByRootIdentifier(
709+
block_root: blockRoot, indices: DataColumnIndices(res))
774710

775711
proc pruneAfterFinalization*(
776712
quarantine: var BlobQuarantine,

‎beacon_chain/sync/request_manager.nim‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -584,13 +584,11 @@ proc getMissingDataColumns(rman: RequestManager): seq[DataColumnsByRootIdentifie
584584

585585
let
586586
commitmentsCount = len(forkyBlck.message.body.blob_kzg_commitments)
587-
missing =
588-
rman.dataColumnQuarantine[].fetchMissingColumnsByRoot(columnless.root, forkyBlck)
587+
ident = rman.dataColumnQuarantine[].fetchMissingSidecars(
588+
columnless.root, forkyBlck)
589589

590-
if len(missing) > 0:
591-
for ident in missing:
592-
if ident notin fetches:
593-
fetches.add(ident)
590+
if len(ident.indices) > 0:
591+
fetches.add(ident)
594592
else:
595593
if commitmentsCount == 0:
596594
# this is a programming error should it occur.

‎tests/test_quarantine.nim‎

Lines changed: 80 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ func compareSidecarsByValue(
115115

116116
func compareSidecars(
117117
blockRoot: Eth2Digest,
118-
a: openArray[ref BlobSidecar|ref DataColumnSidecar],
119-
b: openArray[BlobIdentifier|DataColumnIdentifier]
118+
a: openArray[ref BlobSidecar],
119+
b: openArray[BlobIdentifier]
120120
): bool =
121121
if len(a) != len(b):
122122
return false
@@ -127,14 +127,32 @@ func compareSidecars(
127127
return false
128128
true
129129

130-
func compareIdentifiers(
131-
a, b: openArray[DataColumnIdentifier]): bool =
132-
if len(a) != len(b):
130+
func compareSidecars(
131+
blockRoot: Eth2Digest,
132+
a: openArray[ref DataColumnSidecar],
133+
b: DataColumnsByRootIdentifier
134+
): bool =
135+
if len(a) != len(b.indices):
133136
return false
134137
if len(a) == 0:
135138
return true
139+
if b.block_root != blockRoot:
140+
return false
136141
for i in 0 ..< len(a):
137-
if (a[i].block_root != b[i].block_root) or (a[i].index != b[i].index):
142+
if (a[i][].index != b.indices[i]):
143+
return false
144+
true
145+
146+
func compareIdentifiers(
147+
a, b: DataColumnsByRootIdentifier): bool =
148+
if len(a.indices) != len(b.indices):
149+
return false
150+
if a.block_root != b.block_root:
151+
return false
152+
if len(a.indices) == 0:
153+
return true
154+
for i in 0 ..< len(a.indices):
155+
if (a.indices[i] != b.indices[i]):
138156
return false
139157
true
140158

@@ -1475,37 +1493,33 @@ suite "ColumnQuarantine data structure test suite " & preset():
14751493
broot1 = genBlockRoot(1)
14761494
broot2 = genBlockRoot(2)
14771495
expected1 = [
1478-
@[
1479-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(63)),
1480-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(64)),
1481-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1482-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1483-
@[
1484-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(63)),
1485-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(64)),
1486-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1487-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1488-
@[
1489-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(63)),
1490-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(64)),
1491-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1492-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1493-
@[
1494-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(63)),
1495-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(64)),
1496-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1497-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1498-
@[
1499-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(64)),
1500-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1501-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1502-
@[
1503-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(95)),
1504-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1505-
@[
1506-
DataColumnIdentifier(block_root: broot1, index: ColumnIndex(96))],
1507-
@[],
1508-
@[]
1496+
DataColumnsByRootIdentifier(
1497+
block_root: broot1,
1498+
indices: DataColumnIndices @[ColumnIndex(63), 64, 95, 96]),
1499+
DataColumnsByRootIdentifier(
1500+
block_root: broot1,
1501+
indices: DataColumnIndices @[ColumnIndex(63), 64, 95, 96]),
1502+
DataColumnsByRootIdentifier(
1503+
block_root: broot1,
1504+
indices: DataColumnIndices @[ColumnIndex(63), 64, 95, 96]),
1505+
DataColumnsByRootIdentifier(
1506+
block_root: broot1,
1507+
indices: DataColumnIndices @[ColumnIndex(63), 64, 95, 96]),
1508+
DataColumnsByRootIdentifier(
1509+
block_root: broot1,
1510+
indices: DataColumnIndices @[ColumnIndex(64), 95, 96]),
1511+
DataColumnsByRootIdentifier(
1512+
block_root: broot1,
1513+
indices: DataColumnIndices @[ColumnIndex(95), 96]),
1514+
DataColumnsByRootIdentifier(
1515+
block_root: broot1,
1516+
indices: DataColumnIndices @[ColumnIndex(96)]),
1517+
DataColumnsByRootIdentifier(
1518+
block_root: broot1,
1519+
indices: DataColumnIndices @[]),
1520+
DataColumnsByRootIdentifier(
1521+
block_root: broot1,
1522+
indices: DataColumnIndices @[])
15091523
]
15101524
sidecars1 =
15111525
block:
@@ -1534,10 +1548,10 @@ suite "ColumnQuarantine data structure test suite " & preset():
15341548
let
15351549
missing1 = bq.fetchMissingSidecars(broot1, fuluBlock1)
15361550
missing2 = bq.fetchMissingSidecars(broot2, fuluBlock2)
1537-
missing3 = bq.fetchMissingSidecars(broot1, fuluBlock1,
1538-
peerCustodyColumns1)
1539-
missing4 = bq.fetchMissingSidecars(broot2, fuluBlock2,
1540-
peerCustodyColumns2)
1551+
missing3 =
1552+
bq.fetchMissingSidecars(broot1, fuluBlock1, peerCustodyColumns1)
1553+
missing4 =
1554+
bq.fetchMissingSidecars(broot2, fuluBlock2, peerCustodyColumns2)
15411555

15421556
check:
15431557
compareSidecars(
@@ -1549,7 +1563,7 @@ suite "ColumnQuarantine data structure test suite " & preset():
15491563

15501564
check:
15511565
compareIdentifiers(expected1[i], missing3)
1552-
len(missing4) == 0
1566+
len(missing4.indices) == 0
15531567

15541568
if i >= len(sidecars1):
15551569
break
@@ -1597,7 +1611,7 @@ suite "ColumnQuarantine data structure test suite " & preset():
15971611
func checkSupernodeExpected(
15981612
root: Eth2Digest,
15991613
index: int,
1600-
missing: openArray[DataColumnIdentifier]
1614+
missing: DataColumnsByRootIdentifier
16011615
): bool =
16021616
const ExpectedVectors = [
16031617
(@[63, 64, 65, 66, 95, 96, 97, 98], 0 .. 57),
@@ -1614,11 +1628,12 @@ suite "ColumnQuarantine data structure test suite " & preset():
16141628
doAssert(index in 0 .. 65)
16151629
for expect in ExpectedVectors:
16161630
if index in expect[1]:
1617-
if len(expect[0]) != len(missing):
1631+
if len(expect[0]) != len(missing.indices):
16181632
return false
1619-
for i in 0 ..< len(missing):
1620-
if (missing[i].block_root != root) or
1621-
(int(missing[i].index) != expect[0][i]):
1633+
for i in 0 ..< len(missing.indices):
1634+
if missing.block_root != root:
1635+
return false
1636+
if (int(missing.indices[i]) != expect[0][i]):
16221637
return false
16231638
return true
16241639
false
@@ -1627,8 +1642,8 @@ suite "ColumnQuarantine data structure test suite " & preset():
16271642
let
16281643
missing1 = bq.fetchMissingSidecars(broot1, fuluBlock1)
16291644
missing2 = bq.fetchMissingSidecars(broot2, fuluBlock2)
1630-
missing3 = bq.fetchMissingSidecars(broot1, fuluBlock1,
1631-
peerCustodyColumns1)
1645+
missing3 =
1646+
bq.fetchMissingSidecars(broot1, fuluBlock1, peerCustodyColumns1)
16321647
check:
16331648
compareSidecars(
16341649
broot1,
@@ -1832,34 +1847,38 @@ suite "ColumnQuarantine data structure test suite " & preset():
18321847

18331848
check:
18341849
len(bq) == 0
1835-
len(bq.fetchMissingSidecars(broot1, fuluBlock1, custodyColumns)) ==
1836-
len(custodyColumns)
1837-
len(bq.fetchMissingSidecars(broot2, fuluBlock2, custodyColumns)) ==
1838-
len(custodyColumns)
1850+
len(bq.fetchMissingSidecars(
1851+
broot1, fuluBlock1, custodyColumns).indices) == len(custodyColumns)
1852+
len(bq.fetchMissingSidecars(
1853+
broot2, fuluBlock2, custodyColumns).indices) == len(custodyColumns)
18391854

18401855
for index in 0 ..< len(custodyColumns):
18411856
bq.put(broot1, sidecars1[index])
18421857
check:
18431858
len(bq) == (index + 1)
1844-
len(bq.fetchMissingSidecars(broot1, fuluBlock1, custodyColumns)) ==
1845-
len(custodyColumns) - (index + 1)
1859+
len(bq.fetchMissingSidecars(
1860+
broot1, fuluBlock1, custodyColumns).indices) ==
1861+
len(custodyColumns) - (index + 1)
18461862
bq.put(broot1, sidecars1d[index])
18471863
check:
18481864
len(bq) == (index + 1)
1849-
len(bq.fetchMissingSidecars(broot1, fuluBlock1, custodyColumns)) ==
1850-
len(custodyColumns) - (index + 1)
1865+
len(bq.fetchMissingSidecars(
1866+
broot1, fuluBlock1, custodyColumns).indices) ==
1867+
len(custodyColumns) - (index + 1)
18511868

18521869
for index in 0 ..< len(custodyColumns):
18531870
bq.put(broot2, sidecars2[index])
18541871
check:
18551872
len(bq) == len(custodyColumns) + (index + 1)
1856-
len(bq.fetchMissingSidecars(broot2, fuluBlock2, custodyColumns)) ==
1857-
len(custodyColumns) - (index + 1)
1873+
len(bq.fetchMissingSidecars(
1874+
broot2, fuluBlock2, custodyColumns).indices) ==
1875+
len(custodyColumns) - (index + 1)
18581876
bq.put(broot2, sidecars2d[index])
18591877
check:
18601878
len(bq) == len(custodyColumns) + (index + 1)
1861-
len(bq.fetchMissingSidecars(broot2, fuluBlock2, custodyColumns)) ==
1862-
len(custodyColumns) - (index + 1)
1879+
len(bq.fetchMissingSidecars(
1880+
broot2, fuluBlock2, custodyColumns).indices) ==
1881+
len(custodyColumns) - (index + 1)
18631882

18641883
bq.remove(broot2)
18651884
check len(bq) == len(custodyColumns)

0 commit comments

Comments
 (0)