Created blob verifier worker. #708

cody-littley · 2024-08-19T14:39:24Z

Why are these changes needed?

This is a fragment of a larger PR. This particular PR adds a new worker called the "blob verifier", whose job is to check on the status of a blob until it becomes confirmed/finalized by the disperser.

Checks

I've made sure the lint is passing in this PR.
I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

Signed-off-by: Cody Littley <[email protected]>

jianoaix · 2024-08-19T23:30:42Z

tools/traffic/table/blob_store.go

+
+// GetAll returns all blobs currently stored. For testing purposes only.
+func (store *BlobStore) GetAll() []*BlobMetadata {
+	store.lock.Lock()


Use a RW lock so the read-only function can use reader lock?

jianoaix · 2024-08-19T23:32:35Z

tools/traffic/workers/blob_verifier.go

+	"context"
+	"github.com/Layr-Labs/eigenda/api/clients"
+	"github.com/Layr-Labs/eigenda/api/grpc/disperser"
+	config2 "github.com/Layr-Labs/eigenda/tools/traffic/config"


This alias seems not needed again?

jianoaix · 2024-08-19T23:33:17Z

tools/traffic/workers/blob_verifier.go

+	config *config2.WorkerConfig
+
+	// A table of confirmed blobs. Blobs are added here when they are confirmed by the disperser service.
+	table *table.BlobStore


table -> confirmedBlobs? It'd be more readable

jianoaix · 2024-08-19T23:33:38Z

tools/traffic/workers/blob_verifier.go

+	table *table.BlobStore
+
+	// The disperser client used to monitor the disperser service.
+	dispenser clients.DisperserClient


typo: dispenser -> disperser

Also the comment may be more accurate to say it's used to interact rather than monitor the disperser?

jianoaix · 2024-08-19T23:35:53Z

tools/traffic/workers/blob_verifier.go

+	dispenser clients.DisperserClient
+
+	// The keys of blobs that have not yet been confirmed by the disperser service.
+	unconfirmedKeys []*UnconfirmedKey


unconfirmedBlobs? (reads well when compared with the confirmedBlobs above)

I agree. Change made.

jianoaix · 2024-08-19T23:45:11Z

tools/traffic/workers/blob_verifier.go

+
+	case disperser.BlobStatus_FAILED:
+		verifier.failedCountMetric.Increment()
+		return true


Given this is returning true, it looks what you want is isBlobStatusTerminal

It is actually hard to find an accurate name for this function given that it does lots of different things. I've broken this down into a small number of more focused methods.

jianoaix · 2024-08-19T23:56:11Z

tools/traffic/workers/blob_verifier.go

+	blobIndex := status.GetInfo().BlobVerificationProof.GetBlobIndex()
+
+	confirmationTime := time.Now()
+	confirmationLatency := confirmationTime.Sub(key.SubmissionTime)


There is some inaccuracy introduced by when the status was polled

I agree. This is one of those situations where it's nice data to have as long as you understand the limits of its precision, but the effort required to get more precise data is cost prohibitive. How do you suggest we handle this situation? Should I remove this metric, or is ok to leave it?

jianoaix · 2024-08-19T23:57:13Z

tools/traffic/workers/blob_verifier.go

+// BlobVerifier periodically polls the disperser service to verify the status of blobs that were recently written.
+// When blobs become confirmed, the status verifier updates the blob blobsToRead accordingly.
+// This is a thread safe data structure.
+type BlobVerifier struct {


Based on the purpose of this struct, it'd probably more accurate to name it something like BlobStatusTracker. It doesn't do any verification work from what I see.

jianoaix · 2024-08-19T23:59:41Z

tools/traffic/workers/blob_verifier.go

+	// Revisit this method if there are performance problems.
+
+	unconfirmedKeys := make([]*UnconfirmedKey, 0)
+	for _, key := range verifier.unconfirmedKeys {


Perf idea suggestion: this loop may have tens of thousands blobs. I think we may add an API to Disperser for querying the statuses of a batch of blobs, which will be much more efficient.

This is a good suggestion, although I think adding this API is probably a task that doesn't belong in this PR. Would you like me to take on that task and merge it prior to the traffic generator changes, or should this be something we circle back on later?

Yea just pointing out the potential issue here. We don't need to bundle all changes in a single PR.

jianoaix · 2024-08-20T00:03:55Z

tools/traffic/workers/blob_verifier.go

+	for {
+		select {
+		case <-(*verifier.ctx).Done():
+			verifier.waitGroup.Done()


Can this just return? Not sure if this wait group is needed here

I use the wait group to ensure that all goroutines have exited when stopping the traffic generator, see https://github.com/Layr-Labs/eigenda/pull/666/files#diff-f597b71e94b466f93ec0cc1f7cd21e7b9230cfe074e306569b125d035c794ce5R216

If you don't think this is a necessary precaution I can remove it.

Signed-off-by: Cody Littley <[email protected]>

ian-shim · 2024-08-27T17:49:10Z

tools/traffic/table/blob_metadata.go

@@ -10,6 +10,9 @@ type BlobMetadata struct {
 	// BlobIndex of the blob.
 	BlobIndex uint

+	// Hash of the batch header that the blob was written in.
+	BatchHeaderHash []byte


should this be [32]byte?

change made

ian-shim · 2024-08-27T18:26:25Z

tools/traffic/workers/blob_status_tracker.go

+	for _, key := range verifier.unconfirmedBlobs {
+
+		blobStatus := verifier.getBlobStatus(key)
+		if blobStatus == nil {


should probably log error as this case is unexpected

ian-shim · 2024-08-27T18:27:42Z

tools/traffic/workers/blob_status_tracker.go

+	if err != nil {
+		verifier.logger.Error("failed check blob status", "err:", err)
+		verifier.getStatusErrorCountMetric.Increment()
+		return nil


why don't we return err here?

ian-shim · 2024-08-27T18:29:58Z

tools/traffic/workers/blob_status_tracker.go

+	if requiredDownloads <= 0 {
+		// Allow unlimited downloads.
+		downloadCount = -1
+	} else if requiredDownloads == 0 {


doesn't look like this block will ever be triggered

ian-shim · 2024-08-27T18:30:41Z

tools/traffic/workers/blob_status_tracker.go

+	} else if requiredDownloads == 0 {
+		// Do not download blob.
+		return
+	} else if requiredDownloads < 1 {


same with this condition. requiredDownloads <= 0 includes this and the above conditions?

Not sure if it's fixed. If requiredDownloads < 1, it would have been handled on if requiredDownloads < 0 block above

Not sure if it's fixed. If requiredDownloads < 1, it would have been handled on if requiredDownloads < 0 block above

The requiredDownloads < 1 will trigger if the number of required downloads is between 0 and 1. It's a float64 value. Any number between 0 and 1 is interpreted as a probabilistic number of downloads.

ian-shim · 2024-08-27T18:33:15Z

tools/traffic/workers/blob_status_tracker.go

+		return true
+	case disperser.BlobStatus_INSUFFICIENT_SIGNATURES:
+		return true
+	case disperser.BlobStatus_CONFIRMED:


CONFIRMED isn't a terminal state as it can be transitioned to FINALIZED or FAILED (if reorg'd)

Correct, although the behavior is intentional in this context. Added a comment to clarify.

case disperser.BlobStatus_CONFIRMED: // Technically this isn't terminal, as confirmed blobs eventually should become finalized. // But it is terminal from the status tracker's perspective, since we stop tracking the blob // once it becomes either confirmed or finalized. return true

Signed-off-by: Cody Littley <[email protected]>

ian-shim

lgtm, one last comment

ian-shim · 2024-08-29T20:58:58Z

tools/traffic/workers/blob_status_tracker.go

+	} else if requiredDownloads == 0 {
+		// Do not download blob.
+		return
+	} else if requiredDownloads < 1 {


Not sure if it's fixed. If requiredDownloads < 1, it would have been handled on if requiredDownloads < 0 block above

jianoaix

Mostly look good. A few comments, as I think it's still in the middle of the renaming.

jianoaix · 2024-08-30T00:05:01Z

tools/traffic/workers/blob_status_tracker.go

+)
+
+// BlobStatusTracker periodically polls the disperser service to verify the status of blobs that were recently written.
+// When blobs become confirmed, the status verifier updates the blob blobsToRead accordingly.


nit: status verifier -> status tracker

jianoaix · 2024-08-30T00:06:51Z

tools/traffic/workers/blob_status_tracker.go

+}
+
+// NewBlobVerifier creates a new BlobStatusTracker instance.
+func NewBlobVerifier(


name it consistently as struct name

fixed naming

jianoaix · 2024-08-30T00:09:16Z

tools/traffic/workers/blob_status_tracker.go

+}
+
+// monitor periodically polls the disperser service to verify the status of blobs.
+func (verifier *BlobStatusTracker) monitor() {


verifier -> tracker. May be worth fixing others like VerifierInterval to PollInterval etc

I searched for the string "verifier" and replaced all instances.

Signed-off-by: Cody Littley <[email protected]>

Created blob verifier worker.

eaa5bc5

Signed-off-by: Cody Littley <[email protected]>

cody-littley requested review from jianoaix and ian-shim August 19, 2024 14:39

cody-littley self-assigned this Aug 19, 2024

jianoaix reviewed Aug 20, 2024

View reviewed changes

cody-littley added 3 commits August 22, 2024 09:53

Made suggested changes.

24c942c

Signed-off-by: Cody Littley <[email protected]>

Rename files.

97ee14c

Signed-off-by: Cody Littley <[email protected]>

Fix unit test flake.

60d5dad

Signed-off-by: Cody Littley <[email protected]>

ian-shim reviewed Aug 27, 2024

View reviewed changes

cody-littley added 2 commits August 29, 2024 14:36

Made suggested changes.

51d38ad

Signed-off-by: Cody Littley <[email protected]>

Bugfixes.

aef5d55

Signed-off-by: Cody Littley <[email protected]>

ian-shim approved these changes Aug 29, 2024

View reviewed changes

jianoaix reviewed Aug 30, 2024

View reviewed changes

Made suggested changes.

b7df5a2

Signed-off-by: Cody Littley <[email protected]>

jianoaix approved these changes Sep 3, 2024

View reviewed changes

cody-littley merged commit 57ef5f7 into Layr-Labs:master Sep 3, 2024
6 checks passed

cody-littley deleted the blob-verifier-fragment branch September 3, 2024 19:07

Created blob verifier worker. #708

Created blob verifier worker. #708

Conversation

cody-littley commented Aug 19, 2024 • edited Loading

Why are these changes needed?

Checks

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

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

cody-littley Aug 22, 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

cody-littley Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cody-littley Aug 29, 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

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianoaix left a 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

cody-littley commented Aug 19, 2024 •

edited

Loading

cody-littley Aug 22, 2024 •

edited

Loading

cody-littley Aug 29, 2024 •

edited

Loading

cody-littley Aug 29, 2024 •

edited

Loading