Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Move and rename ledger services from core to ledger #33947

Merged
merged 8 commits into from
Nov 8, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Nov 3, 2023

Problem

These services currently live in core/; however, they operate on the ledger. Mores so, these two services operate on the blockstore only, and not necessarily the entire ledger. So, it makes sense to move these services out of core and into ledger. We've recently been doing similar changes with breaking things out into individual crates in order to reduce the scope of core.

Summary of Changes

  • Move services from core to ledger
  • Rename services from ledger_* to blockstore_*

This PR deletes two tests:

  • The one from local cluster exercises behavior that can be tested in isolation on a "single" node, and is already done so in blockstore_purge.rs
  • The one from core/tests/ was previously used as a way to benchmark shred insertion speed. However, with the introduction of merkle shreds, the method of just updating slots in the shreds in no longer valid. The result is that shreds need to be regenerated on the fly; shred regeneration on the fly is the limiter in this bench so the whole thing is somewhat useless. If we wish to bench this, the best approach would be to have a full blockstore and stream shreds from one to another, new blockstore. This approach is fundamentally different enough that I think rewriting the test from scratch would be better than trying to adapt the old test. The old test has been around long enough that is has been band-aided several times, and I have previously discussed just removing it with YH

Steven Czabaniuk added 8 commits November 7, 2023 10:56
The functionality of blockstore getting cleaned up can be tested on a
"single node"; no need to spin up a local cluster for it. Additionally,
this test has a 60 second sleep which is wasteful for CI.
This test had previously been used as a benchmark for looking at
insertion speed of shreds into Blockstore. However, the test is no
longer able to hit the limits given a fundamental change to shreds.
Namely, the move to Merkle shreds means that shreds must be generated
instead of pre-computed + getting a slot modified. So, remove this test
altogether.

If we wish to recreate something similar in the future, it would best be
accomplished by having a blockstore full of blocks and then streaming
those blocks from the full blockstore to a new one. This is
fundamentally different enough that I think a fresh start would be
easier than trying to adapt the current test.
@steviez steviez force-pushed the mv_bstore_services branch from be99390 to a44ad6d Compare November 7, 2023 18:37
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #33947 (a44ad6d) into master (b8115b4) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 85.1%.

@@           Coverage Diff           @@
##           master   #33947   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         811      811           
  Lines      219354   219354           
=======================================
+ Hits       179699   179724   +25     
+ Misses      39655    39630   -25     

@steviez steviez marked this pull request as ready for review November 7, 2023 22:26
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

This works for me.
Incidentally, I wonder if solana-ledger itself could use some slimming at some point. Beyond blockstore definitions and utilities, it's become the landing place for any "stuff we need in replay"

@steviez
Copy link
Contributor Author

steviez commented Nov 8, 2023

This works for me. Incidentally, I wonder if solana-ledger itself could use some slimming at some point. Beyond blockstore definitions and utilities, it's become the landing place for any "stuff we need in replay"

That's a good point; the replay stuff is only a couple files (mostly blockstore_processor.rs) so I agree that they could seemingly be broken into their own crate. Going to hold off on doing anything at the moment; I know some people are working on overhauling the scheduler. There will likely be some shuffling there, and it doesn't seem out of the realm of possibility that a new home for replay stuff would appear from that

@steviez steviez merged commit 73815ae into solana-labs:master Nov 8, 2023
17 checks passed
@steviez steviez deleted the mv_bstore_services branch November 8, 2023 17:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants