[v2] Blob Metadata Store #818

ian-shim · 2024-10-18T06:06:49Z

Why are these changes needed?

Set up v2 blob metadata store. Only implements few basic methods for now.

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.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

core/data.go

pschork · 2024-10-18T07:23:47Z

disperser/common/v2/blobstore/dynamo_store.go

+	return &dynamodb.CreateTableInput{
+		AttributeDefinitions: []types.AttributeDefinition{
+			{
+				AttributeName: aws.String("PK"),


Could you document the tables that are abbreviations? It's not obvious to me what PK and SK stand for.

pschork · 2024-10-18T07:41:50Z

disperser/common/v2/blobstore/dynamo_store.go

+
+	blobKeyPrefix     = "BlobKey#"
+	dispersalIDPrefix = "DispersalID#"
+	blobMetadataSK    = "BlobMetadata"


So we're not using immutable design where blob metadata updates create a new unique sortable item via compound SK like BlobMetadata#UUID7 or BlobMetadata#EpochSeconds?

yeah as discussed offline, some fields (BlobStatus) will be updated

cody-littley · 2024-10-18T14:55:12Z

disperser/common/v2/blob.go

+type BlobMetadata struct {
+	core.BlobHeaderV2 `json:"blob_header"`
+
+	BlobKey    core.BlobKey `json:"blob_key"`
+	BlobStatus BlobStatus   `json:"blob_status"`
+	// Expiry is Unix timestamp of the blob expiry in seconds from epoch
+	Expiry      uint64 `json:"expiry"`
+	NumRetries  uint   `json:"num_retries"`
+	BlobSize    uint64 `json:"blob_size"`
+	RequestedAt uint64 `json:"requested_at"`
+}


Since this is a high level struct that will get reused in a bunch of places, would it be worth while to add godocs? (This request is non-blocking, feel free to disregard it if you don't think they are worthwile.)

cody-littley · 2024-10-18T14:55:31Z

disperser/common/v2/blobstore/blob_metadata_store.go

+type BlobMetadataStore interface {
+	PutBlobMetadata(ctx context.Context, metadata *v2.BlobMetadata) error
+	GetBlobMetadata(ctx context.Context, blobKey core.BlobKey) (*v2.BlobMetadata, error)
+	GetBlobMetadataByStatus(ctx context.Context, status v2.BlobStatus) ([]*v2.BlobMetadata, error)
+	GetBlobMetadataCountByStatus(ctx context.Context, status v2.BlobStatus) (int32, error)
+}


Please add godocs if you think it's worth it. (This request is non-blocking, feel free to disregard it if you don't think they are worthwile.)

cody-littley · 2024-10-18T14:59:15Z

disperser/common/v2/blobstore/dynamo_store.go

+	return &dynamodb.CreateTableInput{
+		AttributeDefinitions: []types.AttributeDefinition{
+			{
+				AttributeName: aws.String("PK"),


Could you document the tables that are abbreviations? It's not obvious to me what PK and SK stand for.

cody-littley · 2024-10-18T15:01:03Z

disperser/common/v2/blob.go

+type BlobMetadata struct {
+	core.BlobHeaderV2 `json:"blob_header"`
+
+	BlobKey    core.BlobKey `json:"blob_key"`
+	BlobStatus BlobStatus   `json:"blob_status"`
+	// Expiry is Unix timestamp of the blob expiry in seconds from epoch
+	Expiry      uint64 `json:"expiry"`
+	NumRetries  uint   `json:"num_retries"`
+	BlobSize    uint64 `json:"blob_size"`
+	RequestedAt uint64 `json:"requested_at"`
+}


Will this eventually contain information such as blob header and/or blob cert?

It already includes blob header. It represents an disperser internal record tracked in DB. Blob cert will be a separate type and we can probably reuse what's defined in core/v2

ian-shim · 2024-10-24T00:14:04Z

core/data.go

-	data = append(data, paymentBytes...)
-
-	return crypto.Keccak256(data)
+func (pm *PaymentMetadata) Hash() ([32]byte, error) {


updated this hash function to make it compatible with solidity
cc @hopeyen

mooselumph

looks good!

mooselumph · 2024-10-24T22:55:57Z

disperser/common/v2/blobstore/dynamo_store.go

+	OperatorResponseIndexName  = "OperatorResponseIndex"
+
+	blobKeyPrefix     = "BlobKey#"
+	dispersalIDPrefix = "DispersalID#"


Is this used?

It's used in a follow up PR, removed for now

mooselumph · 2024-10-24T23:00:00Z

disperser/common/v2/blobstore/dynamo_store.go

+
+// GetBlobMetadataByStatus returns all the metadata with the given status
+// Because this function scans the entire index, it should only be used for status with a limited number of items.
+func (s *blobMetadataStore) GetBlobMetadataByStatus(ctx context.Context, status v2.BlobStatus) ([]*v2.BlobMetadata, error) {


Would there ever be a need for a batched version of this?

What do you mean by a batched version?

mooselumph · 2024-10-24T23:02:35Z

core/data.go

-	// Existing fields
-	AccountID string
+	// AccountID is the ETH account address for the payer
+	AccountID string `json:"account_id"`


Where is the json serialization used?

It's used when we marshal/unmarshal the struct to/from json, i.e. in dataapi.
I've removed these for now

mooselumph · 2024-10-24T23:07:20Z

core/data.go

+	return hash, nil
+}
+
+func (pm *PaymentMetadata) MarshalDynamoDBAttributeValue() (types.AttributeValue, error) {


Does this belong in core? Is there any way to have this code in the blobstore?

They need to belong in the same package as where the type is defined. We can wrap and clone these types in blobstore but not sure if that's worth it

mooselumph · 2024-10-24T23:07:58Z

core/v2/types.go

@@ -59,7 +81,153 @@ func (b *BlobHeader) GetEncodingParams() (encoding.EncodingParams, error) {
 		NumChunks:   uint64(params.NumChunks),
 		ChunkLength: uint64(length),
 	}, nil
+}
+
+func (b *BlobHeader) BlobKey() (BlobKey, error) {


ian-shim requested review from pschork, mooselumph and cody-littley October 18, 2024 06:06

ian-shim commented Oct 18, 2024

View reviewed changes

core/data.go Outdated Show resolved Hide resolved

ian-shim marked this pull request as ready for review October 18, 2024 06:13

pschork reviewed Oct 18, 2024

View reviewed changes

cody-littley approved these changes Oct 18, 2024

View reviewed changes

ian-shim added 2 commits October 23, 2024 10:07

v2 blob metadata

cc05eed

v2 blob metadata store

839dffe

ian-shim force-pushed the v2-blob-metadata branch from 75c03ab to a4b23f5 Compare October 24, 2024 00:12

ian-shim requested review from cody-littley and hopeyen October 24, 2024 00:13

ian-shim commented Oct 24, 2024

View reviewed changes

ian-shim force-pushed the v2-blob-metadata branch from a4b23f5 to 2837f2f Compare October 24, 2024 00:40

mooselumph reviewed Oct 24, 2024

View reviewed changes

ian-shim force-pushed the v2-blob-metadata branch 2 times, most recently from 0dbb4c9 to 1b63aee Compare October 24, 2024 23:38

incorporate core/v2

3b4a2eb

ian-shim force-pushed the v2-blob-metadata branch from 1b63aee to 3b4a2eb Compare October 25, 2024 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] Blob Metadata Store #818

[v2] Blob Metadata Store #818

ian-shim commented Oct 18, 2024

pschork Oct 18, 2024

cody-littley Oct 18, 2024

pschork Oct 18, 2024 •

edited

Loading

ian-shim Oct 24, 2024

cody-littley Oct 18, 2024

ian-shim Oct 18, 2024

cody-littley Oct 18, 2024

cody-littley Oct 18, 2024

cody-littley Oct 18, 2024

ian-shim Oct 21, 2024

ian-shim Oct 24, 2024

mooselumph left a comment

mooselumph Oct 24, 2024

ian-shim Oct 24, 2024

mooselumph Oct 24, 2024

ian-shim Oct 24, 2024

mooselumph Oct 24, 2024

ian-shim Oct 24, 2024

mooselumph Oct 24, 2024

ian-shim Oct 24, 2024

mooselumph Oct 24, 2024

[v2] Blob Metadata Store #818

Are you sure you want to change the base?

[v2] Blob Metadata Store #818

Conversation

ian-shim commented Oct 18, 2024

Why are these changes needed?

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschork Oct 18, 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

mooselumph 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschork Oct 18, 2024 •

edited

Loading