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.

Sign up for GitHub

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

Jump to bottom

blockstore: make merkle root Optional in MerkleRootMeta column #34091

Merged
merged 1 commit into from
Nov 15, 2023
Merged

blockstore: make merkle root Optional in MerkleRootMeta column #34091

merged 1 commit into from
Nov 15, 2023

Conversation

AshwinSekar
Copy link
Contributor

Problem

After discussion, it seems like legacy shreds should be supported for a while longer. To avoid having to use unwrap_or_default() and mess around with Hash::default(), we change the type of the yet to be used column to optional.

Summary of Changes

Change type. This will be backported to the same branches as #33979
Contributes to #33644

behzadnouri
behzadnouri approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

please make sure this is backported along with the original commit.

@AshwinSekar AshwinSekar merged commit 693d576 into solana-labs:master Nov 15, 2023
27 of 28 checks passed
@AshwinSekar AshwinSekar deleted the merkle-root-meta-option branch November 15, 2023 18:09
@steviez
Copy link
Contributor

steviez commented Nov 15, 2023

We good with leaving the Option<_> in there forever, or think you'll want to rip it out after legacy shreds are fully gone ?

AshwinSekar added a commit that referenced this pull request Nov 15, 2023
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
aaa877e
AshwinSekar added a commit that referenced this pull request Nov 15, 2023
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
9ac39f6
@AshwinSekar
Copy link
Contributor Author

We good with leaving the Option<_> in there forever, or think you'll want to rip it out after legacy shreds are fully gone ?

We can probably rip it out after legacy shreds are fully gone, depending on how much of a PITA it is to migrate the column.

This was referenced Nov 15, 2023
Closed
Merged
@steviez
Copy link
Contributor

steviez commented Nov 20, 2023

We good with leaving the Option<_> in there forever, or think you'll want to rip it out after legacy shreds are fully gone ?

We can probably rip it out after legacy shreds are fully gone, depending on how much of a PITA it is to migrate the column.

Gotcha - my question was aimed at whether we should try to be proactive and put something like an enum in place to allow dual-support. But, the enum would be as much (if not more) hassle in matching enum type than always dealing with the Option<_>, so I think we just leave this for now and disregard my question 😅

@AshwinSekar AshwinSekar self-assigned this Nov 29, 2023
@AshwinSekar AshwinSekar requested a review from t-nelson November 30, 2023 23:29
AshwinSekar added a commit that referenced this pull request Dec 1, 2023
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
fecebc3
AshwinSekar added a commit that referenced this pull request Dec 1, 2023
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
7a2ec68
AshwinSekar added a commit that referenced this pull request Dec 1, 2023
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
6c8f344
mergify bot added a commit that referenced this pull request Dec 1, 2023
@mergify @AshwinSekar
v1.17: add merkle root meta column to blockstore (backport of #33979) (…
da2dadd 
…#34028)

* add merkle root meta column to blockstore (#33979)

* add merkle root meta column to blockstore

* pr feedback: remove write/reads to column

* pr feedback: u64 -> u32 + revert

* pr feedback: fec_set_index u32, use Self::Index

* pr feedback: key size 16 -> 12

(cherry picked from commit e457c02)

* blockstore: make merkle root Optional in MerkleRootMeta column (#34091)

---------

Co-authored-by: Ashwin Sekar <ashwin@solana.com>
steviez pushed a commit that referenced this pull request Jan 5, 2024
@AshwinSekar
blockstore: make merkle root Optional in MerkleRootMeta column (#34091)
a7b4ee5
steviez pushed a commit that referenced this pull request Jan 5, 2024
@AshwinSekar
v1.16: add merkle root meta column to blockstore (backport of #33979) (…
835183a 
…#34665)

* add merkle root meta column to blockstore (#33979)

* add merkle root meta column to blockstore

* pr feedback: remove write/reads to column

* pr feedback: u64 -> u32 + revert

* pr feedback: fec_set_index u32, use Self::Index

* pr feedback: key size 16 -> 12

(cherry picked from commit e457c02)

# Conflicts:
#	ledger/src/blockstore.rs

* fix conflicts

* blockstore: make merkle root Optional in MerkleRootMeta column (#34091)

---------

Co-authored-by: Ashwin Sekar <ashwin@solana.com>
@willhickey willhickey mentioned this pull request Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behzadnouri behzadnouri behzadnouri approved these changes

@steviez steviez Awaiting requested review from steviez

@t-nelson t-nelson Awaiting requested review from t-nelson

Assignees

@AshwinSekar AshwinSekar

Labels
None yet
Projects
Duplicate block prevention and resolution
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AshwinSekar @steviez @behzadnouri