feat: update backfill related db repositories #8085
Conversation
|
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| hasBlobs: ssz.Boolean || null, | ||
| columnIndices: new ListUintNum64Type(128) || null, |
There was a problem hiding this comment.
You cannot do union types like this with our ssz library. They will need to be handled a bit differently. Here are a couple of suggestions but you can also come up with another method if you prefer.
- During deserialization. Have a non-nullable data type
EpochBackfillStateSerialized(or called something similar likeEpochBackfillStateWrapper) that lives in the DB and then on deserialization check the fork for the requested epoch and nullify as necessary and returnEpochBackfillState - Creating additional fields for the presence of a data type like
needsBlobsandneedsColumnsor perhaps storing theforkNameon the type so it's interpreted correctly by the consumer
| async _get(epoch: Epoch): Promise<EpochBackfillState | null> { | ||
| const wrappedValue = await super.get(epoch); | ||
| if (!wrappedValue) return null; | ||
| return this.unwrapEpochBackfillState(wrappedValue, epoch); | ||
| } | ||
|
|
||
| async _put(epoch: Epoch, value: EpochBackfillState): Promise<void> { | ||
| const wrappedValue = this.wrapEpochBackfillState(value); | ||
| await super.put(epoch, wrappedValue); | ||
| } |
There was a problem hiding this comment.
Can't override get and put functions as the function signatures aren't matching with the methods in parent class due to different types: EpochBackfillState and EpochBackfillStateWrapper. So, temporarily changed name to _get and _put.
This doesn't seem to be a good practice. What are your thoughts? What else can be done?
There was a problem hiding this comment.
I'm wondering if @nflaig has any ideas on how to do this. Like perhaps massaging the schema a bit.
There was a problem hiding this comment.
Maybe we can sort something so that we dont need to change the type to EpochBackfillState. So not needing blobs will just get interpreted correctly by the consumer depending on the epoch.... @wemeetagain might also have a suggestion
There was a problem hiding this comment.
Another idea would be to add a field for DAType and that will be a
export const enum DAType = {
PreData = "PreData",
Blobs = "Blobs",
Columns = "Columns",
};We have this exported already by our BlockInput types so can refactor that to a better place in the code for reuse.
There was a problem hiding this comment.
what's the reason we have EpochBackfillState type which deviates from repository type?
There was a problem hiding this comment.
In the DB, we can't store EpochBackfillState with nullable fields as it is not compatible with ssz types. We need null in certain situations, for example, to differentiate between blobs' availability and when they are not even present i.e. pre-Deneb (hasBlobs: boolean | null).
So thought to store EpochBackfillStateWrapper in DB and convert it to EpochBackfillState while putting and getting as needed.
There was a problem hiding this comment.
I think we shouldn't even implement backfill for pre-fulu and only consider data columns, this might simplify things
Edit: even if we don't support pre-fulu we might still need to handle the case if we backfill outside of the DA period (~18 days) after which we only need to backfill sync blocks
There was a problem hiding this comment.
you can use Optional ssz type which allows the value to be nullable
There was a problem hiding this comment.
Thanks. Added OptionalType, modified the unit tests, they're successfully passing.
Please check once.
| columnIndices: number[] | null; | ||
| }; | ||
|
|
||
| export class BackfillState extends Repository<Epoch, EpochBackfillStateWrapper> { |
There was a problem hiding this comment.
using the term "state" here could be misleading, I'd rather call this BackfillTracker or BackfillProgress
There was a problem hiding this comment.
@matthewkeil please confirm
| async _get(epoch: Epoch): Promise<EpochBackfillState | null> { | ||
| const wrappedValue = await super.get(epoch); | ||
| if (!wrappedValue) return null; | ||
| return this.unwrapEpochBackfillState(wrappedValue, epoch); | ||
| } | ||
|
|
||
| async _put(epoch: Epoch, value: EpochBackfillState): Promise<void> { | ||
| const wrappedValue = this.wrapEpochBackfillState(value); | ||
| await super.put(epoch, wrappedValue); | ||
| } |
There was a problem hiding this comment.
what's the reason we have EpochBackfillState type which deviates from repository type?
4 participants
Motivation
This goal of this pr is to include db related changes for backfill.
Backfill issue: #7753
Description
WIP