Add Blob Gossip by terencechain · Pull Request #12413 · OffchainLabs/prysm

terencechain · 2023-05-17T02:28:34Z

This PR

Adds blob p2p validator outlined in: https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/p2p-interface.md#blob_sidecar_index
Adds sufficient unit tests to cover 81% of validate_blobs.go
Adds blob subscriber, note this mostly is no-op, it simply adds the blobs to the seen cache but still needs to cache the blobs before passing all or nothing to blockchain pkg. (See TODO)
Similar to block, blob also has a pending cache for unknown parent

beacon-chain/blockchain/testing/mock.go

beacon-chain/p2p/pubsub_filter.go

beacon-chain/sync/validate_blob.go

rkapka · 2023-05-17T14:19:42Z

beacon-chain/p2p/pubsub_filter.go

-
+	denebForkDigest, err := forks.ForkDigestFromEpoch(params.BeaconConfig().DenebForkEpoch, s.genesisValidatorsRoot)
+	if err != nil {
+		log.WithError(err).Error("Could not determine Capella fork digest")


Suggested change

log.WithError(err).Error("Could not determine Capella fork digest")

log.WithError(err).Error("Could not determine Deneb fork digest")

rkapka · 2023-05-17T14:23:00Z

beacon-chain/sync/pending_blocks_queue.go

+		return []*eth.SignedBlobSidecar{}
+	}
+	bs, ok := value.([]*eth.SignedBlobSidecar)
+	if !ok {


Should we log something here? Something very strange is happening if we have the wrong object in the cache.

rkapka · 2023-05-17T14:24:08Z

beacon-chain/sync/pending_blocks_queue.go

+func (s *Service) addPendingBlobToCache(b *eth.SignedBlobSidecar) error {
+	blobs := s.pendingBlobsInCache(b.Message.Slot)
+
+	blobs = append(blobs, b)


Is it possible that we accidentally append a blob that has been already cached, resulting in having the same blob twice?

Looking at the way we use it, I would say yes, meaning that someone can spam the cache with one blob for an nonexistant block.

if !s.cfg.chain.HasBlock(ctx, parentRoot) { if err := s.addPendingBlobToCache(sBlob)

Nice catch! dbdf385

rkapka · 2023-05-17T14:25:38Z

beacon-chain/sync/service.go


 const rangeLimit = 1024
 const seenBlockSize = 1000
+const seenBlobSize = 4000 // Each block can have max 4 blobs. Worst case 164kB for cache.


Suggested change

const seenBlobSize = 4000 // Each block can have max 4 blobs. Worst case 164kB for cache.

`const seenBlobSize = seenBlockSize * 4` // Each block can have max 4 blobs. Worst case 164kB for cache.

rkapka · 2023-05-17T14:28:03Z

beacon-chain/sync/subscriber.go

+			s.validateBlob,   /* validator */
+			s.blobSubscriber, /* message handler */
+			digest,
+			params.BeaconConfig().MaxBlobsPerBlock,


I think we should have a BlobsSubnetCount value in the network config for symmetry with AttestationSubnetCount. The two can possibly be different in the future, I guess...

rkapka · 2023-05-17T14:35:21Z

beacon-chain/sync/validate_blob.go

+		return pubsub.ValidationIgnore, err
+	}
+
+	// [IGNORE] The blob's block's parent (defined by sidecar.block_parent_root) has been seen (via both gossip and non-gossip sources)


Suggested change

// [IGNORE] The blob's block's parent (defined by sidecar.block_parent_root) has been seen (via both gossip and non-gossip sources)

// [IGNORE] The sidecars's block's parent (defined by sidecar.block_parent_root) has been seen (via both gossip and non-gossip sources)

Nitpick :)

rkapka · 2023-05-17T14:35:49Z

beacon-chain/sync/validate_blob.go

+	parentRoot := bytesutil.ToBytes32(blob.BlockParentRoot)
+	if !s.cfg.chain.HasBlock(ctx, parentRoot) {
+		if err := s.addPendingBlobToCache(sBlob); err != nil {
+			log.WithError(err).WithFields(blobFields(blob)).Error("Failed to add blob to queue")


Suggested change

log.WithError(err).WithFields(blobFields(blob)).Error("Failed to add blob to queue")

log.WithError(err).WithFields(blobFields(blob)).Error("Failed to add blob to cache")

rkapka · 2023-05-17T14:36:51Z

beacon-chain/sync/validate_blob.go

+	}
+
+	// [REJECT] The sidecar's block's parent (defined by sidecar.block_parent_root) passes validation.
+	blk, err := s.cfg.chain.GetBlock(ctx, parentRoot)


Is this enough for "passes validation"?

It's enough. We only put validated object in the DB or initial sync cache

rkapka · 2023-05-17T14:38:15Z

beacon-chain/sync/validate_blob.go

+	// is valid with respect to the sidecar.proposer_index pubkey.
+	parentState, err := s.cfg.stateGen.StateByRoot(ctx, parentRoot)
+	if err != nil {
+		return pubsub.ValidationIgnore, err


Shouldn't we reject here? After all we are unable to verify the signature.

We are not able to get the parent state, which does not mean we cannot verify the signature. In this case, we probably shouldn't penalize the peer, but we do drop the object

rkapka · 2023-05-17T14:42:43Z

beacon-chain/sync/validate_blob.go

+	"github.com/sirupsen/logrus"
+)
+
+func (s *Service) validateBlob(ctx context.Context, pid peer.ID, msg *pubsub.Message) (pubsub.ValidationResult, error) {


I see a little inconsistency in this function with regards to logging. We usually log before returning on ignore/reject, but not always.

Good point. I'm only putting them in the relevant parts. I just fixed it and only use debug

rkapka · 2023-05-18T08:34:08Z

beacon-chain/blockchain/receive_block.go

 	ReceiveBlock(ctx context.Context, block interfaces.ReadOnlySignedBeaconBlock, blockRoot [32]byte) error
 	ReceiveBlockBatch(ctx context.Context, blocks []interfaces.ReadOnlySignedBeaconBlock, blkRoots [][32]byte) error
 	HasBlock(ctx context.Context, root [32]byte) bool
+	GetResentBlockSlot(root [32]byte) (primitives.Slot, error)


What's the purpose of using the word Resent here? Was it supposed to be Recent?

Renamed it to ResentBlockSlot :)

beacon-chain/sync/pending_blocks_queue.go

Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>

terencechain added the Deneb label May 17, 2023

terencechain self-assigned this May 17, 2023

terencechain requested a review from a team as a code owner May 17, 2023 02:28

terencechain requested review from kasey, potuz and rauljordan and removed request for a team May 17, 2023 02:28

saolyn reviewed May 17, 2023

View reviewed changes

beacon-chain/blockchain/testing/mock.go Show resolved Hide resolved

beacon-chain/p2p/pubsub_filter.go Outdated Show resolved Hide resolved

beacon-chain/sync/validate_blob.go Show resolved Hide resolved

rkapka reviewed May 17, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch from 5421f7b to 62c6fc4 Compare May 17, 2023 19:35

terencechain requested a review from prestonvanloon as a code owner May 17, 2023 19:35

rkapka reviewed May 18, 2023

View reviewed changes

terencechain force-pushed the gossip-blob branch from bf95956 to 686a042 Compare May 18, 2023 14:48

terencechain and others added 10 commits May 18, 2023 07:52

Add blob gossipnet

53747c5

Gazelle

8d9ded5

Use Reject

0c422be

Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>

Fix comment

0157759

Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>

Fix comment

4732167

Co-authored-by: Sammy Rosso <15244892+saolyn@users.noreply.github.com>

Feedback

ddeacee

Fix TestValidateBlob_AlreadySeenInCache

88f2380

Fix can subscribe test

680ba49

Use block slot from forkchoice

a369dff

Rename to RecentBlockSlot

cef6483

terencechain force-pushed the gossip-blob branch from 686a042 to cef6483 Compare May 18, 2023 14:52

rkapka approved these changes May 18, 2023

View reviewed changes

terencechain merged commit 6e9f6fd into deneb-integration May 18, 2023

terencechain deleted the gossip-blob branch May 18, 2023 16:13

terencechain added a commit that referenced this pull request May 18, 2023

Add Blob Gossip (#12413)

ce4a937

kasey pushed a commit that referenced this pull request May 19, 2023

Add Blob Gossip (#12413)

ca11e4a

kasey pushed a commit that referenced this pull request May 22, 2023

Add Blob Gossip (#12413)

220beca

terencechain added a commit that referenced this pull request May 31, 2023

Add Blob Gossip (#12413)

5a82b9b

terencechain added a commit that referenced this pull request May 31, 2023

Add Blob Gossip (#12413)

2fb675d

terencechain added a commit that referenced this pull request Jun 7, 2023

Add Blob Gossip (#12413)

ad2f47b

terencechain added a commit that referenced this pull request Jun 12, 2023

Add Blob Gossip (#12413)

a4403b9

terencechain added a commit that referenced this pull request Jun 12, 2023

Add Blob Gossip (#12413)

d541086

terencechain added a commit that referenced this pull request Jun 16, 2023

Add Blob Gossip (#12413)

a3811e6

terencechain added a commit that referenced this pull request Jun 27, 2023

Add Blob Gossip (#12413)

aae3137

terencechain added a commit that referenced this pull request Jul 7, 2023

Add Blob Gossip (#12413)

5c0e7e9

terencechain added a commit that referenced this pull request Jul 9, 2023

Add Blob Gossip (#12413)

df30ab8

terencechain added a commit that referenced this pull request Jul 10, 2023

Add Blob Gossip (#12413)

05b5d1a

kasey pushed a commit that referenced this pull request Jul 20, 2023

Add Blob Gossip (#12413)

87ee4fa

james-prysm pushed a commit that referenced this pull request Aug 4, 2023

Add Blob Gossip (#12413)

75d3a75

terencechain added a commit that referenced this pull request Aug 16, 2023

Add Blob Gossip (#12413)

9416880

kasey pushed a commit that referenced this pull request Aug 21, 2023

Add Blob Gossip (#12413)

b16d4af

kasey pushed a commit that referenced this pull request Aug 22, 2023

Add Blob Gossip (#12413)

4f54f8c

kasey pushed a commit that referenced this pull request Aug 22, 2023

Add Blob Gossip (#12413)

38f7741

kasey pushed a commit that referenced this pull request Aug 22, 2023

Add Blob Gossip (#12413)

d7accd5

kasey pushed a commit that referenced this pull request Aug 23, 2023

Add Blob Gossip (#12413)

1806054

kasey pushed a commit that referenced this pull request Aug 23, 2023

Add Blob Gossip (#12413)

ca673ac

kasey pushed a commit that referenced this pull request Aug 23, 2023

Add Blob Gossip (#12413)

d58c697

kasey pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

a3849aa

kasey pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

d322bf4

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

f9bf11f

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

263af2a

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

95b97a3

prestonvanloon pushed a commit that referenced this pull request Aug 24, 2023

Add Blob Gossip (#12413)

6ab6097

james-prysm pushed a commit that referenced this pull request Aug 25, 2023

Add Blob Gossip (#12413)

190916d

prestonvanloon pushed a commit that referenced this pull request Aug 30, 2023

Add Blob Gossip (#12413)

34efd91

prestonvanloon pushed a commit that referenced this pull request Aug 30, 2023

Add Blob Gossip (#12413)

05d7295

prestonvanloon pushed a commit that referenced this pull request Aug 31, 2023

Add Blob Gossip (#12413)

e3ac8b7

	log.WithError(err).Error("Could not determine Capella fork digest")
	log.WithError(err).Error("Could not determine Deneb fork digest")

	const seenBlobSize = 4000 // Each block can have max 4 blobs. Worst case 164kB for cache.
	`const seenBlobSize = seenBlockSize * 4` // Each block can have max 4 blobs. Worst case 164kB for cache.

	// [IGNORE] The blob's block's parent (defined by sidecar.block_parent_root) has been seen (via both gossip and non-gossip sources)
	// [IGNORE] The sidecars's block's parent (defined by sidecar.block_parent_root) has been seen (via both gossip and non-gossip sources)

	log.WithError(err).WithFields(blobFields(blob)).Error("Failed to add blob to queue")
	log.WithError(err).WithFields(blobFields(blob)).Error("Failed to add blob to cache")

Conversation

terencechain commented May 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkapka May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkapka May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rkapka May 17, 2023 •

edited

Loading

rkapka May 17, 2023 •

edited

Loading