blockstore: use erasure meta index field to find conflicting shreds by AshwinSekar · Pull Request #1151 · anza-xyz/agave

AshwinSekar · 2024-05-02T17:31:18Z

Problem

We perform a costly scan in order to find the original coding shred when an erasure config mismatch is detected.

Summary of Changes

Instead use the new field on erasure meta introduced in #961

Revert to the scan if the field is 0 in order to support old blockstores. This can be removed when we no longer support < v1.18.12

codecov-commenter · 2024-05-02T19:16:23Z

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (4ae2ca1) to head (ded4d5b).
⚠️ Report is 6603 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1151   +/-   ##
=======================================
  Coverage    82.1%    82.1%           
=======================================
  Files         886      886           
  Lines      236439   236490   +51     
=======================================
+ Hits       194252   194306   +54     
+ Misses      42187    42184    -3

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

behzadnouri · 2024-05-03T17:53:31Z

+            if !self.has_duplicate_shreds_in_slot(slot) {
+                let conflicting_shred = self
+                    .find_conflicting_coding_shred(&shred, slot, erasure_meta, just_received_shreds)
+                    .expect("Shred indicated by erasure meta must exist")


This expect here made me wonder what happens in the scenarios that a shred is purge/pruned/dumped from blockstore between the time erasure_meta is obtained until here the shred is fetched from blockstore?

when replay decides to dump a block it uses clear_unconfirmed_slot which shares a lock with shred insertion precisely to avoid the situation you mentioned:

agave/ledger/src/blockstore.rs

Lines 1290 to 1291 in bf1b765

pub fn clear_unconfirmed_slot(&self, slot: Slot) {

let _lock = self.insert_shreds_lock.lock().unwrap();

i'm not an expert on the blockstore automatic cleanup (cc @steviez). while it doesn't share this lock, it looks at blockstore.max_root() in order to determine which slots to cleanup. We also compare against blockstore.max_root() earlier

agave/ledger/src/blockstore.rs

Line 1386 in bf1b765

if !Blockstore::should_insert_coding_shred(&shred, self.max_root()) {

but this seems unsafe as replay_stage could have rooted and triggered an automatic cleanup in between.

hmm, will recheck these blockstore expects under the assumption that cleanup could have happened https://discord.com/channels/428295358100013066/1235788902888116274/1235942333334290442

You're correct that the BlockstoreCleanupService doesn't share the insert_shreds_lock. And right, the service fetches the latest root, and potentially clean data that is strictly older than the root. We never clean data newer than the latest root.

Practically speaking, the value for --limit-ledger-size is required to be >= 50M. Assuming 1k shreds per slot, the oldest data is ~50k slots older than the newest data. That being said, I think there is merit to an unwrap() and expect() audit like y'all are suggesting

hmm, will recheck these blockstore expects under the assumption that cleanup could have happened https://discord.com/channels/428295358100013066/1235788902888116274/1235942333334290442

Given that mainnet is upgrading to v1.18, and we have not have much of soak time, probably better to demote all (or most of) those expects to an error log or metric. We can always add them back in if we are confident we are not missing an edge case.

I combed through the expect/unwraps we introduced and demoted the affected ones here #1259 .

I believe these remaining checks to be safe and have not demoted them:

agave/ledger/src/blockstore.rs

Line 1134 in fd8b075

.expect("Erasure meta was just created, initial shred must exist");

agave/ledger/src/blockstore.rs

Line 1163 in fd8b075

.expect("Merkle root meta was just created, initial shred must exist");

behzadnouri · 2024-05-03T17:54:20Z

+                    .store_duplicate_slot(slot, conflicting_shred.clone(), shred.payload().clone())
+                    .is_err()
+                {
+                    warn!("bad duplicate store..");


this was old code but nonetheless we need to log the actual err than just dropping it.

behzadnouri · 2024-05-08T22:56:57Z

                    ));
                }
-            } else {
-                datapoint_info!("bad-conflict-shred", ("slot", slot, i64));


maybe keep this line (or an error log or something similar), for when find_conflicting_coding_shred returns None.

We have a couple warn!()'s under the // ToDo: ... line, do think those are sufficient ? Namely, I think self.has_duplicate_shreds_in_slot(slot) still being false would indicate that we did not find a conflicting coding shred

hmm! the warn! logs will be there regardless of this datapoint.
but this bad-conflict-shred would specifically indicate that we have a mismatching erasure-meta but can't find the shred which initialized that erasure-meta.

anyways, I am inclined to keep this datapoint just in case we observe these anomalies, but don't feel strongly about that. so either way is fine with me.

I added it back as an error log.

steviez · 2024-05-09T05:11:01Z

+                    if let Err(e) = self.store_duplicate_slot(
+                        slot,
+                        conflicting_shred.clone(),
+                        shred.payload().clone(),


Semi-related to the PR (but certainly not blockers):

Wondering why we didn't use write_batch for this function to store the udpates, not that the updates to this column need to occur atomically with any other column

Wondering if we could store the proof without cloning the two Vec's; we'd want to serialize with borrowed data but deserialize with owned data. A quick glance since bincode supports Cow

yeah we could definitely be smarter about this:

write batch to store duplicate proof updates

cache a map so we don't have to read blockstore every time we check if there's already a duplicate proof

avoid the clones

I guess duplicate's happen so infrequently that this isn't a major problem, but can definitely look into cleaning this up in a follow up.

Wondering why we didn't use write_batch for this function to store the udpate

I figured this out and thought I edited my comment, but I guess I did not:

store_duplicate_slot() and has_duplicate_shreds_in_slot() both read from the DB

Items written to the write_batch are not visible to the DB until the batch is committed

We call has_duplicate_shreds_in_slot() in the same function, potentially after calling store_duplicate_slot() ... if we used the write_batch, the read wouldn't see the write immediately before it. And, if insert_shreds_handle_duplicate() was called with 100 shreds that we're iterating through, a duplicate proof written for the 5th shred would not be visible if we read for the next 95 shreds

This same gotcha is why we have the get_shred_from_just_inserted_or_db() function in combination with the HashMap for getting shreds that are in the same "insert batch"

steviez · 2024-05-09T05:44:03Z

                    ));
                }
-            } else {
-                datapoint_info!("bad-conflict-shred", ("slot", slot, i64));


We have a couple warn!()'s under the // ToDo: ... line, do think those are sufficient ? Namely, I think self.has_duplicate_shreds_in_slot(slot) still being false would indicate that we did not find a conflicting coding shred

steviez

Change looks good to me, but I think it'd be good to confirm that Behzad's concern about the removed datapoint is adequately covered by the comments further down the function I mentioned

AshwinSekar requested review from behzadnouri and steviez May 2, 2024 20:22

AshwinSekar marked this pull request as ready for review May 2, 2024 20:22

behzadnouri reviewed May 3, 2024

View reviewed changes

AshwinSekar added 2 commits May 7, 2024 21:00

blockstore: use erasure meta index field to find conflicting shreds

756e39b

pr feedback: error msg, let Some in case of cleanup

c3b08ae

AshwinSekar force-pushed the conflicting-coding-shred branch from 2dd7027 to c3b08ae Compare May 7, 2024 21:00

behzadnouri previously approved these changes May 8, 2024

View reviewed changes

AshwinSekar mentioned this pull request May 9, 2024

blockstore: account for blockstore cleanup during shred insertion #1259

Merged

steviez reviewed May 9, 2024

View reviewed changes

steviez previously approved these changes May 9, 2024

View reviewed changes

pr feedback: add error if conflicting shred is not found

ded4d5b

AshwinSekar dismissed stale reviews from steviez and behzadnouri via ded4d5b May 9, 2024 21:33

steviez approved these changes May 9, 2024

View reviewed changes

behzadnouri approved these changes May 10, 2024

View reviewed changes

AshwinSekar merged commit 6cd0128 into anza-xyz:master May 10, 2024

AshwinSekar deleted the conflicting-coding-shred branch May 10, 2024 01:12

mergify Bot mentioned this pull request May 10, 2024

v1.18: blockstore: account for blockstore cleanup during shred insertion (backport of #1259) #1279

Merged

	pub fn clear_unconfirmed_slot(&self, slot: Slot) {
	let _lock = self.insert_shreds_lock.lock().unwrap();

Conversation

AshwinSekar commented May 2, 2024

Problem

Summary of Changes

Uh oh!

codecov-commenter commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

steviez May 9, 2024 • 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!

steviez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 2, 2024 •

edited

Loading

steviez May 9, 2024 •

edited

Loading