Deneb DB methods by terencechain · Pull Request #12379 · OffchainLabs/prysm

terencechain · 2023-05-09T23:36:46Z

Adding deneb db methods

New

BlobSidecarsByRoot retrieve blobs by a given root
BlobSidecarsBySlot retrieve blobs by a given slot
SaveBlobSidecar save blobs
DeleteBlobSidecar delete blots

Note: we don't have caches built in for blob as we want to avoid premature optimization

Modified

Block with deneb support
State with deneb support

Misc

New config for max blobs per slot
New config for min epochs to service blobs sidecar
New test helpers on hydration methods for block and require pkg

prestonvanloon

Partial review with @saolyn. I'll review more later today.

beacon-chain/db/kv/blob.go

prestonvanloon · 2023-05-10T14:39:28Z

beacon-chain/db/kv/blob.go

+	if len(scs) == 0 {
+		return errors.New("nil or empty blob sidecars")
+	}
+	slot := scs[0].Slot


Do you need to validate that all of the blobs in the argument scs have the same slot and block root?

Do you also need to validate that the sidecars are sorted by their index and in the correct position?

Good questions. I asked them before on Slack and the response from @kasey was that we don't want to validate it in the db package. I would say otherwise, because we want to make sure the db doesn't get corrupted in any way.

added validations 8315171

I was more opposed to the idea of verifying that the number of sidecars was less than the current value of MAX_BLOBS_PER_BLOCK than checking the consistency between slot and root. But yeah generally I do think that the db code checking the semantics within a value being stored is going a little too far and mixing concerns, especially since we're behind an abstract data storage interface here (kv). As opposed to for instance checking the integrity between values that refer to each other across atomic writes, which would definitely be the db's job, to ensure referential integrity.

context: @rkapka is referring to my response to this comment: I think we should add some more validation though, like checking that the number of saved sidecars is not more than MaxBlobsPerBlock and that all of them have the same slot etc.

prestonvanloon · 2023-05-10T14:42:28Z

beacon-chain/db/kv/blob.go

+		// If there is no element stored at blob.slot % MAX_SLOTS_TO_PERSIST_BLOBS, then we simply
+		// store the blob by key and exit early.
+		if len(replacingKey) == 0 {
+			return bkt.Put(newKey, encodedBlobSidecar)
+		}
+
+		if err := bkt.Delete(replacingKey); err != nil {
+			log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)
+		}
+		return bkt.Put(newKey, encodedBlobSidecar)


Suggested change

// If there is no element stored at blob.slot % MAX_SLOTS_TO_PERSIST_BLOBS, then we simply

// store the blob by key and exit early.

if len(replacingKey) == 0 {

return bkt.Put(newKey, encodedBlobSidecar)

}

if err := bkt.Delete(replacingKey); err != nil {

log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)

}

return bkt.Put(newKey, encodedBlobSidecar)

if len(replacingKey) != 0 {

if err := bkt.Delete(replacingKey); err != nil {

log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)

}

}

return bkt.Put(newKey, encodedBlobSidecar)

This removes the duplicate line of code bkt.Put(newKey, encodedBlobSidecar)

prestonvanloon · 2023-05-10T14:42:37Z

beacon-chain/db/kv/blob.go

+		// If there is no element stored at blob.slot % MAX_SLOTS_TO_PERSIST_BLOBS, then we simply
+		// store the blob by key and exit early.
+		if len(replacingKey) == 0 {
+			return bkt.Put(newKey, encodedBlobSidecar)
+		}
+
+		if err := bkt.Delete(replacingKey); err != nil {
+			log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)
+		}
+		return bkt.Put(newKey, encodedBlobSidecar)


Suggested change

// If there is no element stored at blob.slot % MAX_SLOTS_TO_PERSIST_BLOBS, then we simply

// store the blob by key and exit early.

if len(replacingKey) == 0 {

return bkt.Put(newKey, encodedBlobSidecar)

}

if err := bkt.Delete(replacingKey); err != nil {

log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)

}

return bkt.Put(newKey, encodedBlobSidecar)

if len(replacingKey) != 0 {

if err := bkt.Delete(replacingKey); err != nil {

log.WithError(err).Warnf("Could not delete blob with key %#x", replacingKey)

}

}

return bkt.Put(newKey, encodedBlobSidecar)

This removes the duplicate line of code bkt.Put(newKey, encodedBlobSidecar)

prestonvanloon · 2023-05-10T14:49:33Z

beacon-chain/db/kv/blob.go

+		for k, v := c.First(); k != nil; k, v = c.Next() {
+			if bytes.HasSuffix(k, root[:]) {
+				enc = v
+				break
+			}
+		}


This is going to be an O(n) lookup because you have to search every value in the bucket. Consider indexing the block roots in another bucket or putting the block root at the start of the string. Searching by prefix ought to be O(logn) if bbolt uses binary search. I'm not totally sure about the implementation yet.

Also consider simply acknowledging this in the godoc since the bucket is bounded to a max size, it may never be much of an issue.

I considered multiple options and decided to remain the same to keep the implementation simple. I added a benchmark 9bcd36a to prove in the absolute worst case, every slot has the blob over the last 18 days (highly unlikely), to traverse through the list to get the last blob is no more than 2.3ms. This is entirely acceptable given this is not used in the hot path:

BenchmarkStore_BlobSidecarsByRoot-10 925645 1182 ns/op. // first slot BenchmarkStore_BlobSidecarsByRoot-10 496 2340958 ns/op. // last slot

It could be much faster but I'm OK with accepting it like this. Thanks for writing the benchmark and demonstrating the worst case. We can monitor these spans as well to see how much of an impact it is.

beacon-chain/db/kv/blob.go

prestonvanloon · 2023-05-10T14:56:19Z

beacon-chain/db/kv/blob.go

+	if err := s.db.View(func(tx *bolt.Tx) error {
+		c := tx.Bucket(blobsBucket).Cursor()
+		// Bucket size is bounded and bolt cursors are fast. Moreover, a thin caching layer can be added.
+		for k, v := c.First(); k != nil; k, v = c.Next() {


It may be faster to recompute the prefix bytes(slot_to_rotating_buffer(blob.slot)) and use a prefix scan in boltdb. This should be O(logn) if using binary search. Again, im not 100% sure it does use BS.

https://github.com/etcd-io/bbolt#prefix-scans

Seek uses BS
https://github.com/etcd-io/bbolt/blob/master/cursor.go#L273

This is a good optimization. I used the suggestion bytes(slot_to_rotating_buffer(blob.slot)) here:
8315171

prestonvanloon · 2023-05-10T15:20:42Z

OK I was able to confirm that seek does use binary search so that would make prefix lookups much faster. https://github.com/etcd-io/bbolt/blob/8b1ee10512ccb01d9d8744a620afc12939d26fb2/cursor.go#LL273C1-L273C1

config/params/network_config.go

beacon-chain/db/kv/blocks.go

rkapka · 2023-05-10T16:31:47Z

testing/assertions/assertions.go

Changes to this file are not Deneb-specific. I would open a separate PR targeting develop with these improvements.

I think it's OK to tag alone test utility enhancement in the same PR, and it's nicer to review them in the PR so the reviews get the complete picture. Shout out to @kasey as the original author for these assertion improvements. I'll let him decide if he wants to open them against develop

terencechain · 2023-05-11T02:32:01Z

beacon-chain/db/kv/state.go

 	return s.storeValidatorEntriesSeparately(ctx, tx, validatorsEntries)
 }

+func getPhase0PbState(rawState interface{}) (*ethpb.BeaconState, error) {


Had to do these to fix cognitive complexity 80 of func (*Store).saveStatesEfficientInternal is high (> 65)

saolyn · 2023-05-11T08:48:41Z

beacon-chain/db/kv/state.go

+				return err
+			}
+		case *ethpb.BeaconStateDeneb:
+			pbState, err := getDenebPbState(rawType)


There doesn't appear to be any unit tests for either the capella or deneb cases

saolyn · 2023-05-11T09:07:21Z

beacon-chain/state/state-native/hasher.go

 	case version.Capella:
 		fieldRoots = make([][]byte, params.BeaconConfig().BeaconStateCapellaFieldCount)
+	case version.Deneb:
+		fieldRoots = make([][]byte, params.BeaconConfig().BeaconStateCapellaFieldCount)


Is this correct? shouldn't we have a beacon state field count specific to Deneb?
I've seen we use the Capella one in quite a few places.

saolyn · 2023-05-11T09:19:13Z

config/params/config.go

@@ -104,6 +104,7 @@ type BeaconChainConfig struct {
 	MaxWithdrawalsPerPayload         uint64 `yaml:"MAX_WITHDRAWALS_PER_PAYLOAD" spec:"true"`          // MaxWithdrawalsPerPayload defines the maximum number of withdrawals in a block.
 	MaxBlsToExecutionChanges         uint64 `yaml:"MAX_BLS_TO_EXECUTION_CHANGES" spec:"true"`         // MaxBlsToExecutionChanges defines the maximum number of BLS-to-execution-change objects in a block.
 	MaxValidatorsPerWithdrawalsSweep uint64 `yaml:"MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP" spec:"true"` //MaxValidatorsPerWithdrawalsSweep bounds the size of the sweep searching for withdrawals per slot.


Suggested change

MaxValidatorsPerWithdrawalsSweep uint64 `yaml:"MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP" spec:"true"` //MaxValidatorsPerWithdrawalsSweep bounds the size of the sweep searching for withdrawals per slot.

MaxValidatorsPerWithdrawalsSweep uint64 `yaml:"MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP" spec:"true"` // MaxValidatorsPerWithdrawalsSweep bounds the size of the sweep searching for withdrawals per slot.

I know this isn't part of your modifications but it's hard to ignore

rkapka · 2023-05-11T12:03:56Z

One more validation for verifySideCars: check that each index from 0 to len(scs) - 1 appears exactly once. And please add some unit tests for this function.

terencechain · 2023-05-11T13:33:06Z

One more validation for verifySideCars: check that each index from 0 to len(scs) - 1 appears exactly once. And please add some unit tests for this function.

They have tests. See TestStore_verifySideCars

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

terencechain added the Ready For Review label May 9, 2023

terencechain self-assigned this May 9, 2023

terencechain requested a review from a team as a code owner May 9, 2023 23:36

terencechain requested review from james-prysm, potuz and rkapka and removed request for a team May 9, 2023 23:36

terencechain added the Deneb label May 10, 2023

prestonvanloon requested changes May 10, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

prestonvanloon reviewed May 10, 2023

View reviewed changes

config/params/network_config.go Show resolved Hide resolved

rkapka reviewed May 10, 2023

View reviewed changes

terencechain commented May 11, 2023

View reviewed changes

terencechain force-pushed the deneb-integration branch from 45809a8 to 9970071 Compare May 11, 2023 04:32

saolyn reviewed May 11, 2023

View reviewed changes

terencechain and others added 9 commits May 11, 2023 07:15

Add Deneb DB methods

9f8a2c2

Fix tests

b45190d

Preston's feedback

fab4597

BlobSidecarsByRoot benchmark

d1e6412

Comments and fix tests

ab62427

Check sidecar index and epochs for blob sidecar request

b36491f

suggested change on error

1ece755

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

getPbState helpers

66d81cb

Sammy's feedback

7fa1277

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

Deneb DB methods (#12379)

891666a

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

Deneb DB methods (#12379)

bfeb553

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

Deneb DB methods (#12379)

0576053

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

Deneb DB methods (#12379)

ac7f19f

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

Deneb DB methods (#12379)

92fd77e

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

Deneb DB methods (#12379)

cbc7668

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

Deneb DB methods (#12379)

8993098

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

Deneb DB methods (#12379)

5d2c212

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

Deneb DB methods (#12379)

ab64364

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

Deneb DB methods (#12379)

a845903

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

Deneb DB methods (#12379)

387fe3a

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

Deneb DB methods (#12379)

b1adeeb

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

Deneb DB methods (#12379)

2a607fb

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

Deneb DB methods (#12379)

c95a21e

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

Deneb DB methods (#12379)

2303cd3

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

Deneb DB methods (#12379)

250c5bf

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

Deneb DB methods (#12379)

17b0cdb

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

Deneb DB methods (#12379)

5510954

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

Deneb DB methods (#12379)

688a910

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

Deneb DB methods (#12379)

d2c0082

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

Deneb DB methods (#12379)

097bddc

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

Deneb DB methods (#12379)

c740396

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

Deneb DB methods (#12379)

0960db1

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

Deneb DB methods (#12379)

f85064f

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

Deneb DB methods (#12379)

37a70d2

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

Deneb DB methods (#12379)

17c4338

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

Deneb DB methods (#12379)

3222adc

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

Deneb DB methods (#12379)

1cc20f4

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

Deneb DB methods (#12379)

9523134

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

Deneb DB methods (#12379)

43e8ae4

	MaxValidatorsPerWithdrawalsSweep uint64 `yaml:"MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP" spec:"true"` //MaxValidatorsPerWithdrawalsSweep bounds the size of the sweep searching for withdrawals per slot.
	MaxValidatorsPerWithdrawalsSweep uint64 `yaml:"MAX_VALIDATORS_PER_WITHDRAWALS_SWEEP" spec:"true"` // MaxValidatorsPerWithdrawalsSweep bounds the size of the sweep searching for withdrawals per slot.

Conversation

terencechain commented May 9, 2023

New

Modified

Misc

Uh oh!

prestonvanloon left a comment

Choose a reason for hiding this comment

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!

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!

terencechain May 10, 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!

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!

This comment was marked as duplicate.

Uh oh!

This comment was marked as duplicate.

Uh oh!

prestonvanloon commented May 10, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saolyn May 11, 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 commented May 11, 2023

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

terencechain May 10, 2023 •

edited

Loading

saolyn May 11, 2023 •

edited

Loading

terencechain commented May 11, 2023 •

edited

Loading