Skip to content

Create database table for blob data sidecar quarantine #7108

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

Open
wants to merge 23 commits into
base: unstable
Choose a base branch
from

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented Apr 25, 2025

Add low-level interface to persist blob data sidecars by block root + index. Also support deleting junk that's been finalized and deletion of all data associated with a block root.

Can pass db.getQuarantineDB() into the quarantine to expose the new DB tables while hiding APIs that deal with trusted data, and then use the new beacon_chain_db_quarantine.nim APIs to store / fetch / delete.

Copy link

github-actions bot commented Apr 25, 2025

Unit Test Results

       15 files  ±  0    2 645 suites  +5   1h 22m 9s ⏱️ + 2m 34s
  6 542 tests +  4    6 018 ✔️ +  4  524 💤 ±0  0 ±0 
45 386 runs  +30  44 638 ✔️ +30  748 💤 ±0  0 ±0 

Results for commit 3d7c3af. ± Comparison against base commit a8f02a3.

♻️ This comment has been updated with latest results.

@etan-status etan-status changed the title Create database tables for block / blob data sidecar quarantine Create database tables for blob data sidecar quarantine May 2, 2025
@etan-status etan-status changed the title Create database tables for blob data sidecar quarantine Create database table for blob data sidecar quarantine May 2, 2025
@etan-status
Copy link
Contributor Author

The quarantine DB works as is for blobs (and also for blocks in earlier commit), but it seems that a custom solution based on SQLite implicit rowid is preferred over explicit rowid through primary key.

@etan-status etan-status closed this May 3, 2025
@etan-status etan-status deleted the dev/etan/df-bq branch May 3, 2025 18:01
@cheatfate cheatfate restored the dev/etan/df-bq branch May 6, 2025 09:39
@cheatfate cheatfate reopened this May 6, 2025
template tableName(sidecar: typedesc[ForkyDataSidecar]): string =
when sidecar is deneb.BlobSidecar:
"electra_sidecars_quarantine"
else:
Copy link
Contributor

@tersec tersec Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all of these, it would be more future-proof to adopt the structure used elsewhere in the CL of:

  when sidecar is deneb.BlobSidecar:
    ...
  elif sidecar is fulu.DataColumnSidecar:
    ...
  else:
    static: doAssert false/static: raiseAssert/{.error.}/...

because the next DAS approach after PeerDAS likely won't be based around fulu.DataColumnSidecar and it'd be good to be able to catch all such issues at compile-time

): KvResult[QuarantineDB] =
let
electraDataSidecar =
? backend.initDataSidecarStore(tableName(deneb.BlobSidecar))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe comment explicitly in some places that this means there's no compatibility guarantees, forward or backward, around this schema.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively just be very explicit, indeed, about the lack of durability/persistence between instances, so any Fulu quarantine tables made now don't constrain the future.

name: string
): KvResult[DataSidecarStore] =
if name == "":
return ok(DataSidecarStore())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this early-exit path actually function, i.e. when something tries to use it?


# Without this export compilation fails with error
# vendor\nim-chronicles\chronicles.nim(352, 21) Error: undeclared identifier: 'activeChroniclesStream'
# It actually does not needed, because chronicles is not used in this file,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • It actually isn't needed; or
  • It actually is not needed

# Without this export compilation fails with error
# vendor\nim-chronicles\chronicles.nim(352, 21) Error: undeclared identifier: 'activeChroniclesStream'
# It actually does not needed, because chronicles is not used in this file,
# but because decodeSZSSZ() generic and uses chronicles generic expansion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decodeSZSSZ() is generic

etan-status and others added 14 commits June 12, 2025 12:52

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Add low-level interface to persist blocks by block root and sidecars
by block root + index. Also support deleting junk that's been finalized
and deletion of all data associated with a block root.

Can pass `db.getQuarantineDB()` into the quarantine to expose the new DB
tables while hiding APIs that deal with trusted data, and then use the
new `beacon_chain_db_quarantine.nim` APIs to store / fetch / delete.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Partially verified

This commit is signed with the committer’s verified signature.
cheatfate’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov
Fix stack usage compilation warnings using `closureScope`.

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov
… it.

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov
Add tests.

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov

Verified

This commit was signed with the committer’s verified signature.
cheatfate Eugene Kabanov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants