adopt columns into the BN, except column syncing
#6945
Conversation
agnxsh
changed the title
columns into the BN, except column syncing
beacon_chain/beacon_chain_file.nim
Outdated
| @@ -291,7 +291,8 @@ proc setTail*(chandle: var ChainFileHandle, bdata: BlockData) = | |||
| chandle.data.tail = Opt.some(bdata) | |||
|
|
|||
| proc store*(chandle: ChainFileHandle, signedBlock: ForkedSignedBeaconBlock, | |||
| blobs: Opt[BlobSidecars]): Result[void, string] = | |||
| blobs: Opt[BlobSidecars]): | |||
| Result[void, string] = | |||
There was a problem hiding this comment.
Its better not to change format style at all. Because this style contradicts with style nph prefers and contradicts with style Nim developers using.
There was a problem hiding this comment.
apologies, must've overlooked while undoing a few things
beacon_chain/beacon_chain_file.nim
Outdated
| @@ -550,6 +551,7 @@ proc decodeBlob( | |||
| return err("Incorrect blob format") | |||
| ok(blob) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I think its some quirk CRLF here.
| @@ -128,7 +128,8 @@ proc setTail*(clist: ChainListRef, bdata: BlockData) = | |||
| clist.handle = Opt.some(handle) | |||
|
|
|||
| proc store*(clist: ChainListRef, signedBlock: ForkedSignedBeaconBlock, | |||
| blobs: Opt[BlobSidecars]): Result[void, string] = | |||
| blobs: Opt[BlobSidecars]): | |||
| Result[void, string] = | |||
| @@ -169,7 +170,8 @@ proc checkBlobs(signedBlock: ForkedSignedBeaconBlock, | |||
|
|
|||
| proc addBackfillBlockData*( | |||
| clist: ChainListRef, signedBlock: ForkedSignedBeaconBlock, | |||
| blobsOpt: Opt[BlobSidecars]): Result[void, VerifierError] = | |||
| blobsOpt: Opt[BlobSidecars]): | |||
| Result[void, VerifierError] = | |||
| cfg: RuntimeConfig | ||
| getBeaconTime: GetBeaconTimeFn | ||
| cfg*: RuntimeConfig | ||
| getBeaconTime*: GetBeaconTimeFn |
There was a problem hiding this comment.
Is there a reason why this become public?
There was a problem hiding this comment.
we don't use RequestManager blob loop post FULU_FORK_EPOCH,is there a better way to do it?
There was a problem hiding this comment.
From my point of view detection of current fork should not be done by time... I think it should be done from what we have in database, if our database is in Electra fork why we start request columns with RequestManager for pre-Fulu fork data?
beacon_chain/nimbus_beacon_node.nim
Outdated
| untrustedBlockVerifier = | ||
| proc(signedBlock: ForkedSignedBeaconBlock, blobs: Opt[BlobSidecars], | ||
| maybeFinalized: bool): Future[Result[void, VerifierError]] {. | ||
| maybeFinalized: bool): | ||
| Future[Result[void, VerifierError]] {. |
There was a problem hiding this comment.
Please revert all the style changes.
|
|
||
| if accumulatedDataColumns.len == 0: | ||
| # no data columns were sent for this post Fulu block, yet | ||
| return await blockProcessor[].addBlock(MsgSource.gossip, signedBlock, |
There was a problem hiding this comment.
Message source is definitely not gossip.
| if not blobQuarantine[].hasBlobs(forkyBlck): | ||
| # We don't have all the blobs for this block, so we have | ||
| # to put it in blobless quarantine. | ||
| if not quarantine[].addBlobless(dag.finalizedHead.slot, forkyBlck): | ||
| err(VerifierError.UnviableFork) | ||
| return err(VerifierError.UnviableFork) |
There was a problem hiding this comment.
I dont think return is needed here.
| else: | ||
| err(VerifierError.MissingParent) | ||
| return err(VerifierError.MissingParent) |
| elif dataColumnQuarantine[].supernode and | ||
| accumulatedDataColumns.len >= (dataColumnQuarantine[].custody_columns.len div 2): | ||
| # We have seen 50%+ data columns, we can attempt to add this block | ||
| let dataColumns = dataColumnQuarantine[].popDataColumns(forkyBlck.root, forkyBlck) |
There was a problem hiding this comment.
If this operation will require costly process of rebuilding columns which could take 800ms it should not be part of verification process.
There was a problem hiding this comment.
Doesn't perform reconstruction here
| @@ -1321,7 +1358,15 @@ proc addElectraMessageHandlers( | |||
|
|
|||
| proc addFuluMessageHandlers( | |||
| node: BeaconNode, forkDigest: ForkDigest, slot: Slot) = | |||
| node.addElectraMessageHandlers(forkDigest, slot) | |||
| node.addCapellaMessageHandlers(forkDigest, slot) | |||
There was a problem hiding this comment.
There should be comment while Fulu message handlers procedure starts with call to Capella handlers instead of Deneb or Electra.
| @@ -2160,6 +2246,9 @@ proc run(node: BeaconNode) {.raises: [CatchableError].} = | |||
|
|
|||
| node.startLightClient() | |||
| node.requestManager.start() | |||
| if node.network.getBeaconTime().slotOrZero.epoch >= | |||
There was a problem hiding this comment.
It looks suspicious to me, are we trying to detect specific fork using time? Why we are not detecting our current fork using database?
| rman.dataColumnLoopFuture = rman.requestManagerDataColumnLoop() | ||
| if not(isNil(rman.blobLoopFuture)): | ||
| rman.blobLoopFuture.cancelSoon() |
There was a problem hiding this comment.
cancelSoon should not be used you should cancel it with cancelAndWait() before spawning new loop.
beacon_chain/sync/sync_queue.nim
Outdated
| @@ -24,7 +24,8 @@ type | |||
| GetBoolCallback* = proc(): bool {.gcsafe, raises: [].} | |||
| ProcessingCallback* = proc() {.gcsafe, raises: [].} | |||
| BlockVerifier* = proc(signedBlock: ForkedSignedBeaconBlock, | |||
| blobs: Opt[BlobSidecars], maybeFinalized: bool): | |||
| blobs: Opt[BlobSidecars], | |||
There was a problem hiding this comment.
This style changes are totally unnecessary
|
|
|
If one updates this to |
Continued from the
columnsbranch. This branch essentially attempts to add column support to the nimbus-eth2 CL post FULU_FORK_EPOCH. However it does NOT contain column syncing baked into existingSyncManagerNote that all column related mechanics, are gated to and after the Fulu fork, would be helpful to have more eyes on the gating.