-
Notifications
You must be signed in to change notification settings - Fork 276
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
base: unstable
Are you sure you want to change the base?
Conversation
850a228
to
5572f3a
Compare
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. |
template tableName(sidecar: typedesc[ForkyDataSidecar]): string = | ||
when sidecar is deneb.BlobSidecar: | ||
"electra_sidecars_quarantine" | ||
else: |
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.
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)) |
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.
Maybe comment explicitly in some places that this means there's no compatibility guarantees, forward or backward, around this schema.
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.
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()) |
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.
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, |
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.
- 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 |
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.
decodeSZSSZ() is generic
d581663
to
079d0fa
Compare
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.
Fix stack usage compilation warnings using `closureScope`.
Add tests.
4cb3423
to
3d7c3af
Compare
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 newbeacon_chain_db_quarantine.nim
APIs to store / fetch / delete.