Backfill Blobs #13595

kasey · 2024-02-07T19:22:50Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR adds the ability to sync blobs while backfilling blocks. This is a requirement for prysm nodes started from checkpoint sync to fulfill the promise of data availability. It's also puts us back in compliance with the p2p rpc specs which requires us to serve blocks up to MIN_EPOCHS_FOR_BLOCK_REQUESTS, and blobs up to MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS. Being unable to fulfill these responsibilities could hurt the peer score of prysm nodes and degrade network performance.

Which issues(s) does this PR fix?

Fixes #13029

terencechain · 2024-02-12T16:16:41Z

beacon-chain/sync/backfill/batch.go

+func (b batch) blobRequest() *eth.BlobSidecarsByRangeRequest {
+	return &eth.BlobSidecarsByRangeRequest{
+		StartSlot: b.begin,
+		Count:     uint64(b.end - b.begin),


Is b.end > b.begin requirement verified somewhere?

This should be implicitly done in the before method of the batcher

terencechain · 2024-02-12T16:27:08Z

beacon-chain/sync/backfill/blobs.go

+	return len(bs.expected) - bs.next
+}
+
+func (bs *blobSync) validateNext(rb blocks.ROBlob) error {


Does this function need any unit test?

This will mostly piggy back on the tests of the verification package but I could add some additional coverage.

terencechain · 2024-02-12T16:29:06Z

beacon-chain/sync/backfill/blobs.go

+	return len(bs.expected) - bs.next
+}
+
+func (bs *blobSync) validateNext(rb blocks.ROBlob) error {


Where do we verify the blob's slot?

We implicitly know that they match by comparing the block roots. The base request method also adds the following validations:

https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/sync/rpc_send_request.go#L296-L304

https://github.com/prysmaticlabs/prysm/blob/develop/beacon-chain/sync/rpc_send_request.go#L221-L265

james-prysm · 2024-02-12T16:29:37Z

beacon-chain/sync/backfill/verify.go

@@ -27,7 +58,7 @@ type verifier struct {
 }

 // TODO: rewrite this to use ROBlock.


is this TODO comment still needed?

yeah it's referring to the input parameter.

james-prysm · 2024-02-12T16:47:21Z

beacon-chain/sync/backfill/worker.go

+		backfillBlobsApproximateBytes.Add(float64(sz))
+		log.WithFields(b.logFields()).WithField("dlbytes", sz).Debug("backfill batch blob bytes downloaded")
+	}
+	if b.blobsNeeded() > 0 {


could this panic if b.bs is empty?

not possible the way it's currently written but good idea to check it for future proofing.

The only way to get into this func is if state is batchBlobSync, and I've changed the code so the only way to get into that state and set the value for batchh.results is to also set the batch.bs via withResults. I think that makes calling a func that assumes batch.bs is not nil safe. This can be improved with some more refactoring after I get the clock initialization overhaul done.

beacon-chain/sync/backfill/batch.go

beacon-chain/sync/backfill/blobs.go

prestonvanloon · 2024-02-12T16:28:19Z

beacon-chain/sync/backfill/blobs.go

+		if err != nil {
+			return nil, err
+		}
+		if bytesutil.ToBytes48(vb.KzgCommitment) != bytesutil.ToBytes48(c[i]) {


q: is this better than !bytes.Equal? I think ToBytes48 allocates memory while bytes.Equal casts to string to compare in place

maybe we can also just do [48]byte(vb.KzgCommitment) != [48]byte(c[i]) .

will do. i'll use bytes.Equal because I'm scared to cast to an array (ToBytes48 does padding for safety so it's not totally equivalent to casting to the underlying array).

beacon-chain/sync/backfill/metrics.go

nisdas · 2024-02-13T06:03:35Z

beacon-chain/node/node.go

@@ -251,6 +243,16 @@ func New(cliCtx *cli.Context, cancel context.CancelFunc, opts ...Option) (*Beaco
 	beacon.verifyInitWaiter = verification.NewInitializerWaiter(
 		beacon.clockWaiter, forkchoice.NewROForkChoice(beacon.forkChoicer), beacon.stateGen)

+	pa := peers.NewAssigner(beacon.fetchP2P().Peers(), beacon.forkChoicer)


What was the reason to change the ordering here ?

Because backfill service now depends on initwaiter (ie blob validation), which depends on stategen, which depends on backfill db. stategen depends on the backfill database (to check if a given slot is available), but not on the rest of backfill. So first we init the backfill db, then stategen and blob verification, then the backfill service. This is why the backfill db coverage package and db init is separate, to avoid circularity.

nisdas · 2024-02-13T06:12:44Z

beacon-chain/sync/backfill/batch.go

+func (b batch) blobRequest() *eth.BlobSidecarsByRangeRequest {
+	return &eth.BlobSidecarsByRangeRequest{
+		StartSlot: b.begin,
+		Count:     uint64(b.end - b.begin),


This should be implicitly done in the before method of the batcher

nisdas · 2024-02-13T06:22:06Z

beacon-chain/sync/backfill/blobs.go

+		if err != nil {
+			return nil, err
+		}
+		if bytesutil.ToBytes48(vb.KzgCommitment) != bytesutil.ToBytes48(c[i]) {


maybe we can also just do [48]byte(vb.KzgCommitment) != [48]byte(c[i]) .

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

terencechain

lgtm

kasey marked this pull request as ready for review February 9, 2024 15:44

kasey requested a review from a team as a code owner February 9, 2024 15:44

kasey requested review from nalepae, potuz and nisdas February 9, 2024 15:44

kasey changed the title ~~WIP: Backfill Blobs~~ Backfill Blobs Feb 9, 2024

kasey force-pushed the backfill-blobs branch 2 times, most recently from 6155179 to 5279dd1 Compare February 10, 2024 21:03

terencechain reviewed Feb 12, 2024

View reviewed changes

james-prysm reviewed Feb 12, 2024

View reviewed changes

prestonvanloon reviewed Feb 12, 2024

View reviewed changes

nisdas reviewed Feb 13, 2024

View reviewed changes

kasey force-pushed the backfill-blobs branch 4 times, most recently from c346815 to 45ec5ae Compare February 14, 2024 05:07

nisdas previously approved these changes Feb 14, 2024

View reviewed changes

kasey and others added 9 commits February 14, 2024 14:38

retrieve and save blobs during backfill

a99b3bb

Update beacon-chain/sync/backfill/batch.go

7d83d37

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

Update beacon-chain/sync/backfill/blobs.go

b1bab28

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

Update beacon-chain/sync/backfill/metrics.go

976702c

Co-authored-by: Preston Van Loon <pvanloon@offchainlabs.com>

make blobSync initialization path a little safer

43d1b7e

use bytes.Equal to avoid extra allocation

bcc4c22

stop using log.Fatal and other little cleanups

3670d64

wrap up post blob sync actions in batch mutator

b1980b6

unit test coverage on verifier

bbf2391

kasey dismissed nisdas’s stale review via bbf2391 February 14, 2024 20:38

kasey force-pushed the backfill-blobs branch from 45ec5ae to bbf2391 Compare February 14, 2024 20:38

terencechain approved these changes Feb 14, 2024

View reviewed changes

prestonvanloon approved these changes Feb 14, 2024

View reviewed changes

kasey enabled auto-merge February 14, 2024 20:44

kasey added this pull request to the merge queue Feb 14, 2024

Merged via the queue into develop with commit bb66201 Feb 14, 2024
17 checks passed

kasey deleted the backfill-blobs branch February 14, 2024 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backfill Blobs #13595

Backfill Blobs #13595

kasey commented Feb 7, 2024 •

edited

Loading

terencechain Feb 12, 2024

nisdas Feb 13, 2024

terencechain Feb 12, 2024

kasey Feb 13, 2024

terencechain Feb 12, 2024

kasey Feb 13, 2024

james-prysm Feb 12, 2024

kasey Feb 13, 2024

james-prysm Feb 12, 2024

kasey Feb 13, 2024

kasey Feb 13, 2024 •

edited

Loading

prestonvanloon Feb 12, 2024

nisdas Feb 13, 2024

kasey Feb 13, 2024

kasey Feb 13, 2024

nisdas Feb 13, 2024

kasey Feb 13, 2024 •

edited

Loading

nisdas Feb 13, 2024

nisdas Feb 13, 2024

terencechain left a comment

		@@ -27,7 +58,7 @@ type verifier struct {
		}

		// TODO: rewrite this to use ROBlock.

Backfill Blobs #13595

Backfill Blobs #13595

Conversation

kasey commented Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasey Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasey Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

kasey commented Feb 7, 2024 •

edited

Loading

kasey Feb 13, 2024 •

edited

Loading

kasey Feb 13, 2024 •

edited

Loading