add merkle root meta column to blockstore #33979

AshwinSekar · 2023-11-07T21:03:58Z

Problem

In order to detect duplicate blocks with conflicting merkle roots we need to be able to access the merkle root for each FEC set. We would like to store the merkle root and relevant meta information for easy access in blockstore.

Summary of Changes

Add new merkle root meta column

Contributes to #33644

codecov · 2023-11-07T23:37:58Z

Codecov Report

Merging #33979 (f17b5d2) into master (04e4efd) will increase coverage by 0.0%.
The diff coverage is 57.1%.

@@           Coverage Diff           @@
##           master   #33979   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         811      811           
  Lines      219424   219452   +28     
=======================================
+ Hits       179761   179797   +36     
+ Misses      39663    39655    -8

behzadnouri · 2023-11-08T19:57:23Z

Should we separate out the new blockstore column addition into its own pr?
I think this commit correctly identifies merkle root mismatch when fec_set_index are the same. But another scenario for duplicate blocks is that you receive a coding shred which indicates data shreds with indices [10..20] belong to the same FEC set with fec_set_index = 10, but then you have a data shred with index 17 (i.e. within that range) but different fec_set_index. We are not catching this case anywhere right?
Another case is that we have 2 coding shreds which indicate overlapping FEC sets.

AshwinSekar · 2023-11-08T20:05:23Z

Should we separate out the new blockstore column addition into its own pr?

This PR only adds the new column, and writes to it. The next change will actually utilize the column

I think this commit correctly identifies merkle root mismatch when fec_set_index are the same. But another scenario for duplicate blocks is that you receive a coding shred which indicates data shreds with indices [10..20] belong to the same FEC set with fec_set_index = 10, but then you have a data shred with index 17 (i.e. within that range) but different fec_set_index. We are not catching this case anywhere right?

Another case is that we have 2 coding shreds which indicate overlapping FEC sets.

Correct, this behavior is not currently caught, but it is next on my list! #33037

ledger/src/blockstore.rs

behzadnouri · 2023-11-09T16:13:00Z

This PR only adds the new column, and writes to it. The next change will actually utilize the column

don't we need to backport the new column to v1.17, v1.16 for forward compatibility?
In that case we probably need to keep the writing part out as well.

steviez · 2023-11-09T16:31:32Z

don't we need to backport the new column to v1.17, v1.16 for forward compatibility?
In that case we probably need to keep the writing part out as well.

Yes, the idea would be to backport some minimal PR to the older versions. Since we're adding a new column, the old branches only need to know about the existence of the columns (Rocksdb has a requirement that all present columns must be opened in order to open the DB).

I think we could considerably slim down what we backport from this PR. Namely, we don't need any code that interacts with the column, like the stuff in do_insert_shreds().

We would definitely want the BP for v1.17, probably v1.16 as well but we'll likely have to do some convincing given where v1.16 is at in its' release cycle

ledger/src/blockstore_db.rs

ledger/src/blockstore_meta.rs

behzadnouri · 2023-11-09T16:57:45Z

ledger/src/shred.rs

+    pub fn merkle_root(&self) -> Option<Hash> {
+        layout::get_merkle_root(self.payload())
+    }


Instead of using raw layout, this probably should dispatch to merkle_root methods:
https://github.com/solana-labs/solana/blob/a9509f56b/ledger/src/shred/merkle.rs#L157
https://github.com/solana-labs/solana/blob/a9509f56b/ledger/src/shred/merkle.rs#L269

noted, will do this in the following PR

AshwinSekar · 2023-11-09T17:18:45Z

Yes, the idea would be to backport some minimal PR to the older versions. Since we're adding a new column, the old branches only need to know about the existence of the columns (Rocksdb has a requirement that all present columns must be opened in order to open the DB).

good to know, I was unaware of the rocksdb requirement. I've slimmed down this PR to just the addition of the new column.

ledger/src/blockstore.rs

ledger/src/blockstore/blockstore_purge.rs

ledger/src/blockstore_db.rs

ledger/src/blockstore_meta.rs

ledger/src/blockstore.rs

behzadnouri · 2023-11-10T14:28:23Z

ledger/src/blockstore_db.rs

+    }
+
+    fn key((slot, fec_set_index): Self::Index) -> Vec<u8> {
+        let mut key = vec![0; 16];


This is not 16 bytes anymore.
It is 8 + 4 = 12 bytes.

Can we add a test for this part? The mismatch isn't caught by any test sounds scary.

unable to create a test that would fail.
assert_eq!(MerkleRootMeta::index(&MerkleRootMeta::key((slot, fec_set_index)), (slot, fec_set_index)) will pass with both 12 and 16.

only way would be to check the length of MerkleRootMeta::key which requires hardcoding the 16 or 12 in the unit test. this approach would not have caught the oversight.

a round-trip test might still be useful to assert that what is written is read back the same.
asserting manually in the test that the key size is 12 is also fine (agreed that it wouldn't have caught this though, but nonetheless)

Never hit submit but was chatting with Ashwin and agreed with others that a unit test would have been unlikely to catch this.

There is a key_size() field on the Column trait that I thought about suggesting; however, this method is flawed in that it calls std::mem::size_of on Self::Index. The result is that the calculated key size from this call will include any alignment bytes that would be present in Self::Index. But, we manually pack the keys end-to-end, so the actual keys do not contain alignment bytes. Have a PR to rip that out / maybe re-implement on each column individually.

Will think on this one a little more to see if there is some way to make this a little more full-proof.

behzadnouri · 2023-11-13T14:22:13Z

I think we'll definitely want this BP'd for v1.17; I think it is worth bringing up in #releng as well to discuss v1.16 BP. A Blockstore that is opened with this commit will create the new, empty column. Without a v1.16 BP, v1.16 would be unable to open a Blockstore that has been opened with tip-of-master

We always want to be + 1 version compatible, so the storage created/written to by v1.17 should work with v1.16 as well; and so if we are backporting to v1.17 then we need to backport to v1.16 as well.

Nonetheless, my preference would be to backport to v1.16 anyways. It would be a pita if we need to do some debugging on mainnet and then we hit this incompatibility issue.

* 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

mergify · 2023-11-29T19:04:03Z

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

mergify · 2023-11-29T19:04:04Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify · 2023-11-30T23:29:31Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

* 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

* 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)

…#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>

mergify · 2024-01-05T17:14:51Z

Backports to the stable branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule.

* 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

…#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>

AshwinSekar force-pushed the merkle-root-blockstore branch 3 times, most recently from 5b62f50 to 01f3c9f Compare November 7, 2023 21:54

AshwinSekar changed the title ~~add merkle root column in blockstore~~ add merkle root meta column to blockstore Nov 7, 2023

AshwinSekar force-pushed the merkle-root-blockstore branch from 01f3c9f to 29fae8d Compare November 7, 2023 22:14

AshwinSekar marked this pull request as ready for review November 7, 2023 23:16

AshwinSekar requested review from steviez and behzadnouri November 7, 2023 23:16

AshwinSekar mentioned this pull request Nov 7, 2023

blockstore: add merkle root to ErasureMeta column family #33848

Closed

AshwinSekar commented Nov 8, 2023

View reviewed changes

ledger/src/blockstore.rs Outdated Show resolved Hide resolved

behzadnouri reviewed Nov 9, 2023

View reviewed changes

ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved

ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved

ledger/src/blockstore_db.rs Outdated Show resolved Hide resolved

behzadnouri reviewed Nov 9, 2023

View reviewed changes

AshwinSekar force-pushed the merkle-root-blockstore branch from 29fae8d to eba1089 Compare November 9, 2023 17:12

AshwinSekar requested a review from behzadnouri November 9, 2023 17:19

behzadnouri reviewed Nov 9, 2023

View reviewed changes

AshwinSekar force-pushed the merkle-root-blockstore branch from eba1089 to 1fdfed7 Compare November 9, 2023 21:15

AshwinSekar requested a review from behzadnouri November 9, 2023 21:26

AshwinSekar added the v1.17 PRs that should be backported to v1.17 label Nov 9, 2023

steviez reviewed Nov 10, 2023

View reviewed changes

ledger/src/blockstore.rs Show resolved Hide resolved

behzadnouri reviewed Nov 10, 2023

View reviewed changes

AshwinSekar force-pushed the merkle-root-blockstore branch from 1fdfed7 to a5dd204 Compare November 10, 2023 16:39

AshwinSekar added 3 commits November 11, 2023 01:26

add merkle root meta column to blockstore

ac7b562

pr feedback: remove write/reads to column

45f504a

pr feedback: u64 -> u32 + revert

fe0e45e

AshwinSekar added the v1.16 PRs that should be backported to v1.16 label Nov 13, 2023

mergify bot mentioned this pull request Nov 13, 2023

v1.16: add merkle root meta column to blockstore (backport of #33979) #34046

Closed

AshwinSekar mentioned this pull request Nov 15, 2023

blockstore: make merkle root Optional in MerkleRootMeta column #34091

Merged

AshwinSekar deleted the merkle-root-blockstore branch November 15, 2023 20:16

behzadnouri mentioned this pull request Nov 16, 2023

gossip: process duplicate proofs for merkle root conflicts #34066

Merged

This was referenced Nov 17, 2023

Remove key_size() method from Column trait #34021

Merged

Allow Blockstore to open unknown columns #34174

Merged

AshwinSekar self-assigned this Nov 29, 2023

AshwinSekar added v1.17 PRs that should be backported to v1.17 and removed v1.16 PRs that should be backported to v1.16 v1.17 PRs that should be backported to v1.17 labels Nov 29, 2023

AshwinSekar requested a review from t-nelson November 30, 2023 23:29

steviez added the v1.16 PRs that should be backported to v1.16 label Jan 5, 2024

steviez removed the v1.16 PRs that should be backported to v1.16 label Jan 5, 2024

steviez mentioned this pull request Jan 5, 2024

v1.16: add merkle root meta column to blockstore (backport of #33979) #34665

Merged

HaoranYi mentioned this pull request Apr 8, 2024

pr634 1.17 new filter anza-xyz/agave#657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add merkle root meta column to blockstore #33979

add merkle root meta column to blockstore #33979

AshwinSekar commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

behzadnouri commented Nov 8, 2023

AshwinSekar commented Nov 8, 2023

behzadnouri commented Nov 9, 2023

steviez commented Nov 9, 2023

behzadnouri Nov 9, 2023

AshwinSekar Nov 9, 2023

AshwinSekar commented Nov 9, 2023

behzadnouri Nov 10, 2023

AshwinSekar Nov 10, 2023

yhchiang-sol Nov 10, 2023

AshwinSekar Nov 11, 2023

behzadnouri Nov 12, 2023

steviez Nov 13, 2023

behzadnouri commented Nov 13, 2023

mergify bot commented Nov 29, 2023

mergify bot commented Nov 29, 2023

mergify bot commented Nov 30, 2023

mergify bot commented Jan 5, 2024

add merkle root meta column to blockstore #33979

add merkle root meta column to blockstore #33979

Conversation

AshwinSekar commented Nov 7, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

behzadnouri commented Nov 8, 2023

AshwinSekar commented Nov 8, 2023

behzadnouri commented Nov 9, 2023

steviez commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar commented Nov 9, 2023

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

behzadnouri commented Nov 13, 2023

mergify bot commented Nov 29, 2023

mergify bot commented Nov 29, 2023

mergify bot commented Nov 30, 2023

mergify bot commented Jan 5, 2024

AshwinSekar commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading