Skip to content
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

Amend Blobs Database to Use a Rotating Buffer for Keys #3

Merged
merged 13 commits into from
Nov 4, 2022

Conversation

rauljordan
Copy link

@rauljordan rauljordan commented Oct 25, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Bolt DB does not play nicely with pruning, as evidenced by extensive pains developing slasher. Blob data can be huge, and we will be storing a lot of it in Prysm with EIP-4844. Instead of having to prune, we devise a database schema for blobs where keys are a rotating buffer that wraps around once the expiration period for blobs is reached. For example, imagine we store 2 epochs max worth of blobs. Then, once we reach epoch 2, we will be overriding the contents of epoch 0 in the database by "wrapping around" the keys using a modulo operation. This removes the need for any kind of deletion routine or frequent pruning and keeps the database size bounded.

There is some complexity involved, as we there could be multiple blob sidecars per slot, so we can't do a naive overwriting algorithm. Instead, here's how the saving of blobs works:

When we receive a blob:

  1. Convert slot using a modulo operator to [0, maxSlots] where maxSlots = MAX_BLOB_EPOCHS*SLOTS_PER_EPOCH
  2. Compute key for blob as bytes(slot_to_rotating_buffer(blob.slot)) ++ bytes(blob.slot) ++ blob.block_root
  3. Begin the save algorithm: If the incoming blob has a slot bigger than the last saved slot at the spot in the rotating keys buffer, we overwrite all elements.
firstElemKey = getFirstElement(bucket)
shouldOverwrite = blob.slot > bytes_to_slot(firstElemKey[8:16])
if shouldOverwrite:
for existingKey := seek prefix bytes(slot_to_rotating_buffer(blob.slot))
  bucket.delete(existingKey)
bucket.put(key, blob)

The unit test included shows exactly how the algorithm works.

// where slot_to_rotating_buffer(slot) = slot % MAX_SLOTS_TO_PERSIST_BLOBS.
func blobSidecarKey(blob *ethpb.BlobsSidecar) []byte {
slotsPerEpoch := params.BeaconConfig().SlotsPerEpoch
maxEpochsToPersistBlobs := params.BeaconNetworkConfig().MinEpochsForBlobsSidecarsRequest
Copy link

Choose a reason for hiding this comment

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

Should this be configurable via a flag? I can imagine users wanting to save blobs for longer.

Copy link
Author

Choose a reason for hiding this comment

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

Yep it should be done via a feature config IMO...will reserve that for a second PR

beacon-chain/db/kv/blobs.go Outdated Show resolved Hide resolved
// store the blob by key and exit early.
if len(firstElementKey) == 0 {
return bkt.Put(key, encodedBlobSidecar)
} else if len(firstElementKey) != len(key) {
Copy link

Choose a reason for hiding this comment

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

Is this even possible?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah maybe someone changes the schema. It's just a defensive check to prevent panics and instead error gracefully because otherwise we'll be accessing slice indices out of range

beacon-chain/db/kv/blobs.go Outdated Show resolved Hide resolved
if shouldOverwrite {
for k, _ := c.Seek(rotatingBufferPrefix); bytes.HasPrefix(k, rotatingBufferPrefix); k, _ = c.Next() {
if err := bkt.Delete(k); err != nil {
return err
Copy link

Choose a reason for hiding this comment

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

Aren't we possibly leaving the db in an inconsistent state?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah good point, might instead collect the errors

beacon-chain/db/kv/blobs.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blobs.go Outdated Show resolved Hide resolved
Comment on lines +115 to 117
if len(k) != blobSidecarKeyLength {
continue
}
Copy link

Choose a reason for hiding this comment

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

Is this even possible?

Copy link
Author

Choose a reason for hiding this comment

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

Same comment as above

beacon-chain/db/kv/blobs.go Outdated Show resolved Hide resolved
require.NoError(t, db.SaveBlobsSidecar(ctx, sidecar))
require.Equal(t, true, db.HasBlobsSidecar(ctx, bytesutil.ToBytes32(sidecar.BeaconBlockRoot)))

keyPrefix = append(bytesutil.SlotToBytesBigEndian(0), bytesutil.SlotToBytesBigEndian(64)...)
Copy link

Choose a reason for hiding this comment

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

You should not append 64 because the original sidecar at round buffer's 0 value had a different slot.

Copy link
Author

Choose a reason for hiding this comment

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

In this example, the key we are expecting is 0 ++ 64 ++ bytes32(foo-64). A key contains the round buffer, the slot, and the block root. The new value we are storing at round buffer 0 will be under this new key, so this is the correct value to expect

rauljordan and others added 6 commits October 25, 2022 11:39
Co-authored-by: Radosław Kapka <radek@prysmaticlabs.com>
Co-authored-by: Radosław Kapka <radek@prysmaticlabs.com>
Co-authored-by: Radosław Kapka <radek@prysmaticlabs.com>
Copy link
Owner

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Lgtm, let's review this more in detail when it goes to the upstream develop branch

Copy link

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

I still argue that TestBlobsSidecar_Overwriting does not test overwriting. The last assertion will pass even if old blobs were not overwritten. We should check the number of items under key prefix bytesutil.SlotToBytesBigEndian(0).

@terencechain terencechain merged commit aba7e73 into terencechain:eip4844 Nov 4, 2022
@terencechain
Copy link
Owner

@rkapka I'm going to move this to prysm branch, we'll make a note of your comment there. Thanks!

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.

3 participants