store non canonical blob sidecars

[mehdi-aouadi]

PR Description

Add non canonical blob sidecars storage:

• Activated based on --data-storage-non-canonical-blocks-enabled option
• Non canonical blob sidecars are stored in a separate column NON_CANONICAL_BLOB_SIDECAR_BY_SLOT_AND_BLOCK_ROOT_AND_BLOB_INDEX
• Non canonical blob sidecars are pruned by the BlobSidecarPruner: Same logic as blob sidecars, the non canonical ones prior to the blob sidecars availability window are deleted

Fixed Issue(s)

#7229

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[mehdi-aouadi]

PS: I set the option to true in the database migrator

[mehdi-aouadi]

Doc change PR: Consensys/doc.teku#459

[rolfyone]

We can't imply this by the fact non-canonical-blocks-enabled=true?

[mehdi-aouadi]

We can't imply this by the fact non-canonical-blocks-enabled=true?

We thought it would be better to separate them. Users might only need one or the other depending on their needs.

[rolfyone]

When will we ever not use 'non-canonical-block' storage but set this enabled?
Are we doing this to follow the non canonical storage model?
Are we keeping non-canonical blobs forever like we keep non canonical blocks?

If there's a way in which it differs from non canonical block storage, then I kind of get it, but if its storing non canonical blobs forever like we store non canonical blocks, then I'm struggling to see the use-case

[tbenr]

I thought separating them because blocks and blobs have actually very different use cases (at least in theory).
You might be an L2 operator that is interested in non canonical blobs in particular and not being interested in non canonical blocks. On the other hand, a service could be interested in tracking orphaned blocks, without being interested in having the corresponding blobs (they are just meaningless data for them).

I don't have a super-strong opinion if there is real need of separating the options.

About forever storing, I think both non canonical blocks and blobs should respect the storage mode in place.

In particular:

• For blocks, we are currently missing pruning of non canonical when the MINIMAL is enabled. I think we should fix this.

• For blobs, we should prune non canonical respecting the same data availability window applicable for canonical blobs.
  in addition, as per make MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS overridable #6395 if someone wants to run an altruistic node serving more blobs (wider DA window), the modified pruning window should effects the non canonical blobs too.

[rolfyone]

As long as we're pretty sure people actually want to store sidecars and not non canonical blocks specifically... I would be happy to make them just store both initially...

I just don't want to end up in a position where because someone has used this flag and not that flag that they've got useless data they cant access anyway, or we have to do weird lookups because the block isn't present... it'll just be a big time sink for little value and no current call to do this permutation.

[rolfyone]

Can we make sure we don't follow the non canonical block storage style? I don't want another special table that has to be completely separately handled unless we really have to do that...

[mehdi-aouadi]

As long as we're pretty sure people actually want to store sidecars and not non canonical blocks specifically... I would be happy to make them just store both initially...

I just don't want to end up in a position where because someone has used this flag and not that flag that they've got useless data they cant access anyway, or we have to do weird lookups because the block isn't present... it'll just be a big time sink for little value and no current call to do this permutation.

I will keep the option as it is and update the description and the doc then

[mehdi-aouadi]

Can we make sure we don't follow the non canonical block storage style? I don't want another special table that has to be completely separately handled unless we really have to do that...

I'm implementing it the same way as non canonical blocks actually: A separate column where I move blob sidecars from the blob sidecars table to the non canonical blob sidecars one (when we update the finalized data if the feature is enabled). I understand this might imply heavy db operations (I batched them: 100 blocks at a time -> Max 100 blocks * 6 blobs) but I thought this would keep things simple and separate. In this case, there will be no impact when the feature is off. The current non canonical blocks storage is not ideal because we never delete them but that could be fixed and avoided right from the start with non canonical blob sidecars (if that's what's bothering you).

Otherwise we were thinking of other approaches but none of them is really convincing:

1. We could add a flag in the column key (SlotAndBlockRootAndBlobIndex -> SlotAndBlockRootAndBlobIndexCanonical) but this would mean adding complexity and possibly impacting performance even if the feature is not active.
2. Another idea @tbenr came up with: extend the DB schema and implement some kind of a flag entry which could live next to each entry (after each blob sidecar entry we could have have an entry which could flag it as non conical) but that requires implementing a new level of abstraction and seeing how it goes. Not sure if this use case is worth the effort.

Do you think it's worth implementing one of these? Or do we keep it the same way (separate table) and make sure we do the required cleanups?

[rolfyone]

Separate block columns get complicated and slow things down, i think we want an understanding of why we need separate columns initially...
Also whether these are even needed - to my mind they're good for debugging but worthless if they're non canonical for any L2... This may be a PO question...
To me I'd be tempted to dump them to file, I still think that's better than database storage for disk, but I realise that's not how the current set is done.
I just find it hard to see why we'd use this at all other than development debug while the testnets are going. If that's the only time then adding columns is a whole lot of work to help out debugging something.
Also remember that blocks can start out canonical and become non canonical by finalization. this means moving all of this data between tables which becomes a bunch of reads and writes. this is one reason while we'll probably end up re-visiting block storage, and if we've followed the same pattern 'because thats how its done', then we've just made more of a mess to clean up potentially...

To me this seems like it's either best off being a part of non canonical storage flag, or a development flag initially at best, and we need to be super careful about the database structure if we do end up storing them. Unless there's an actual identified need for it for L2's or something, more than just something we guess they might like...

[tbenr]

I agree that this is more a PO question. We are doing this not for debug purpose, it is for aligning the non-canonical feature with new deneb blobs. I assume there are real use-cases, even if not very popular. But if we are unsure, let's park it for now.

Thinking about the need of having a separate column for non-canonical: it is for easely accessing canonical without the need of getting canonical blocks first to filter non canonical blobs out.

This mainly impacts the getBlobsByRange RPC. If canonical and non canonical where stored in the same table we have to filter them out, even for very old finalized blobs, which requires extra DB access. And this have to be done even if the non-canonical flag is currently off (you could have enabled it for a while and still have non-canonical data in DB). So there will be a constant impact for all users just for supporting a very niche use case.
With a separate column, there will be no impact for normal users (no filtering needed) the new column will be forever empty. For the ones who want to enable the non-canonical feature, they'll probably accept to pay the DB overhead price.

From API perspective, now there is teku specific getAllBlocks which takes in consideration the non canonical. I see there will be the corresponding getAllBlobs.

[mehdi-aouadi]

I removed the non canonical blob sidecars storage option (I use the existing one related to the non canonical blocks) and added the persistence logic.
I will park this PR for now until we reach consensus on how we could implement this if we need this this feature at all.

[tbenr]

I think there is something to fix.

I played a bit with the test, while trying to understand it better: tbenr@4d5e30d

[tbenr]

I also suspect that we will need nonCanonicalBlobsidecarsAtSlotfor the api. We may end up not using the current method. No we will actually use it in the standard get BlobSidecars when when blockId is a block root (when we lookup non canonical knowing the block root)

[mehdi-aouadi]

I also suspect that we will need nonCanonicalBlobsidecarsAtSlotfor the api. We may end up not using the current method. No we will actually use it in the standard get BlobSidecars when when blockId is a block root (when we lookup non canonical knowing the block root)

I think it will be better to update/add the APIs in a separate PR once this one is merged

[tbenr]

We are vwry close. A couple of things still

[tbenr]

LGTM. I'd like to have a feedback from @zilm13 on the earliestBlobSidecarSlot in thepruneBlobSidecars method