Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store Blinded Beacon Blocks by Default for New Prysm Databases #11591

Merged
merged 32 commits into from
Mar 1, 2023

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Oct 27, 2022

This PR sets up new Prysm beacon node databases to store blocks in blinded format, which is far better for long-term storage requirements of Prysm. There is no need to store full execution payloads when these can be retrieved from the execution client at anytime. This PR aims to be minimally disruptive to existing Prysm users and does not require a database migration.

We estimate savings of at least 150Gb from using this feature with geth nodes

A design document for this PR along with trade-offs is linked here

Here are the invariants we enforce, and each is covered by a unit test.

Existing Databases

  1. If an existing DB is storing full beacon blocks, it will continue storing full beacon blocks
  2. If an existing DB is storing blinded beacon blocks, and a user enables the --save-full-execution-payloads flag, we throw an error and fail to start the node. We point the user to resync if desired and recommend checkpoint sync

New Databases

  1. If the --save-full-execution-payloads flag is enabled, we store full beacon blocks
  2. If default settings are used, we store blinded beacon blocks

@rauljordan rauljordan requested a review from a team as a code owner October 27, 2022 19:29
@rauljordan rauljordan added Enhancement New feature or request Ready For Review A pull request ready for code review labels Oct 27, 2022
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been run in our infrastructure? Has it been run in prod?

Can you add the design doc link to the description, if you have one?

beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/kv.go Outdated Show resolved Hide resolved

var migrationBlindedBeaconBlocksKey = []byte("blinded-beacon-blocks-enabled")

func migrateBlindedBeaconBlocksEnabled(ctx context.Context, db *bolt.DB) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do users opt-in to this feature with an old database without a migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is far too error prone to create a full migration logic, as it is expensive and slow. Instead, we are only enabling this for new databases. Thankfully, checkpoint sync exists and helps us quickly resync if users want to leverage this feature and reduce their disk use

var encodedBlock []byte
var err error
blockToSave := blk
if features.Get().EnableOnlyBlindedBeaconBlocks {
if saveBlindedBlocks {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set this if branch to always be true and no tests failed. Are you sure you have enough test coverage on marshalBlock with this new functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After refactoring, confirmed we now have have coverage in both branches. It has been split into two functions and we now have them covered

@rauljordan rauljordan added the Blocked Blocked by research or external factors label Feb 17, 2023
@rauljordan
Copy link
Contributor Author

Blocking until we can test in Capella devnets / current testnets / and mainnet as it changes default behavior for Prysm

@rauljordan
Copy link
Contributor Author

PTAL again @prestonvanloon if you have some time

beacon-chain/db/kv/kv.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/blocks.go Outdated Show resolved Hide resolved
beacon-chain/db/kv/kv.go Show resolved Hide resolved
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nisdas nisdas added OK to merge and removed Blocked Blocked by research or external factors labels Feb 28, 2023
@prylabs-bulldozer prylabs-bulldozer bot merged commit 11b90e1 into develop Mar 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the default-blinded branch March 1, 2023 00:07
@henridf
Copy link

henridf commented Mar 7, 2023

A design document for this PR along with trade-offs is linked here

This doc is inaccessible to people outside your Notion org. Would it be possible to open it up or somehow share the contents?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants