-
Notifications
You must be signed in to change notification settings - Fork 296
attempt to solve columndb issue + fix cgc with non-validator issue #7507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
| for i in 0..<columns.len: | ||
| let r = verify_data_column_sidecar_kzg_proofs(columns[i][]) | ||
| if r.isErr: | ||
| malformed_cols.incl(i) | ||
| if r.isErr(): | ||
| debug "backfill data column validation failed", | ||
| blockRoot = shortLog(signedBlock.root), | ||
| column_sidecar = shortLog(columns[i][]), | ||
| blck = shortLog(signedBlock.message), | ||
| signature = shortLog(signedBlock.signature), | ||
| msg = r.error() | ||
|
|
||
| columnsOk = (dataColumnsOpt.get.lenu64 - malformed_cols.lenu64) >= | ||
| (self.consensusManager.dag.cfg.NUMBER_OF_COLUMNS div 2) | ||
| columnsOk = r.isOk() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't columnsOk be always the result of the last columns if without break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is fine actually, if it is the result of the last columns, we want the entirety of DataColumnSidecarto be columnsOk = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally, these "ok" style variables with nested if conditions are pretty hard to read - avoid if possible, ie pull the logic out into its own function and return an error directly instead - see #7518
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ok style variables existed before because we didn't have overloaded functions for storeBlock and storeBackfillBlock, which is why there were different decisions of block addition based on the combination of blobsOk + columnsOk. i agree, later on, once simplified cleaning up makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one of the best opportunities to do cleanup work like this is when you're reading the "previous" version of the code to understand what it's doing, as part of the due diligence to add the new version - this turns the passive reading into an active endeavor which helps cement the understanding of the existing code - ie just before adding the fulu fork - this way, the existing code gets cleaned up at a regular basis and the new fork you're adding starts out clean.
…7507) * attempt to solve columndb issue + fix cgc with non-validator issue * some more changes * oops * Update beacon_chain/gossip_processing/block_processor.nim --------- Co-authored-by: tersec <[email protected]>
…7507) * attempt to solve columndb issue + fix cgc with non-validator issue * some more changes * oops * Update beacon_chain/gossip_processing/block_processor.nim --------- Co-authored-by: tersec <[email protected]>
No description provided.