Skip to content

Store changes to persist data columns#6073

Merged
mergify[bot] merged 8 commits into
sigp:unstablefrom
jimmygchen:data-columns-store
Aug 2, 2024
Merged

Store changes to persist data columns#6073
mergify[bot] merged 8 commits into
sigp:unstablefrom
jimmygchen:data-columns-store

Conversation

@jimmygchen

@jimmygchen jimmygchen commented Jul 11, 2024 •
edited by dapplion
Loading

Copy link
Copy Markdown
Member

Issue Addressed

Part of #6072.

This PR contains only the Store changes to persist data columns.

DataColumns are stored individually by index. Sampling peers will request single columns or arbitrary slots. Storing bundled like blobs would be inefficient.

Upstream PRs:

@jimmygchen @dapplion
Store changes to persist data columns.
80b416f 
Co-authored-by: dapplion <35266934+dapplion@users.noreply.github.com>
@jimmygchen jimmygchen added ready-for-review The code is ready for review das Data Availability Sampling labels Jul 11, 2024
@dapplion dapplion mentioned this pull request Jul 11, 2024
Closed
@jimmygchen
Merge branch 'unstable' into data-columns-store
3746f31 
# Conflicts:
#	beacon_node/store/src/lib.rs
#	consensus/types/src/chain_spec.rs
@jimmygchen
Minor refactor.
c9390d4
@jimmygchen jimmygchen mentioned this pull request Jul 29, 2024
Merged
realbigsean
realbigsean reviewed Jul 31, 2024
View reviewed changes
Comment thread beacon_node/store/src/lib.rs Outdated
Comment thread beacon_node/store/src/hot_cold_store.rs Outdated
@jimmygchen jimmygchen requested a review from realbigsean August 1, 2024 07:14
michaelsproul
michaelsproul approved these changes Aug 2, 2024
View reviewed changes

@michaelsproul michaelsproul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Although this introduces new columns on disk I think it is OK to merge this without a schema version increment as nothing will be stored in those columns until the fork epoch is set.

Ideally we would want to block downgrading to a column-oblivious schema once peer DAS is activated, but it's kind of a moot point because nodes won't be able to follow the chain without these changes (and the rest of the DAS logic).

We will also be bumping the schema version (and disallowing downgrades) with this PR:

Comment thread beacon_node/store/src/hot_cold_store.rs Outdated
Comment thread beacon_node/store/src/hot_cold_store.rs Outdated
@michaelsproul

michaelsproul commented Aug 2, 2024

Copy link
Copy Markdown
Member

@mergify queue

@mergify

mergify Bot commented Aug 2, 2024

Copy link
Copy Markdown

queue

🛑 The pull request has been removed from the queue default

Details

The pull request #6073 has been manually updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 2, 2024
@michaelsproul

michaelsproul commented Aug 2, 2024

Copy link
Copy Markdown
Member

@mergify refresh

@mergify

mergify Bot commented Aug 2, 2024

Copy link
Copy Markdown

refresh

✅ Pull request refreshed

@michaelsproul

michaelsproul commented Aug 2, 2024

Copy link
Copy Markdown
Member

@mergify queue

@mergify

mergify Bot commented Aug 2, 2024 •
edited
Loading

Copy link
Copy Markdown

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 0e96d4f

@mergify mergify Bot merged commit 0e96d4f into sigp:unstable Aug 2, 2024
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
@jimmygchen @chong-he
Store changes to persist data columns (sigp#6073)
0d7ea99 
* Store changes to persist data columns.

Co-authored-by: dapplion <35266934+dapplion@users.noreply.github.com>

* Update to use `eip7594_fork_epoch` for data column slot in Store.

* Fix formatting.

* Merge branch 'unstable' into data-columns-store

# Conflicts:
#	beacon_node/store/src/lib.rs
#	consensus/types/src/chain_spec.rs

* Minor refactor.

* Merge branch 'unstable' into data-columns-store

# Conflicts:
#	beacon_node/store/src/metrics.rs

* Init data colum info at PeerDAS epoch instead of Deneb fork epoch. Address review comments.

* Remove Deneb-related comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

@realbigsean realbigsean Awaiting requested review from realbigsean

Assignees

No one assigned

Labels

das Data Availability Sampling das-unstable-merge ready-for-merge This PR is ready to merge.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jimmygchen @michaelsproul @realbigsean