Trim extra shred bytes in blockstore

[steviez]

Problem

Data shred bytestreams are currently inserted into the blockstore with padded 0's. Every data shred has at least some padded 0's due to there being a "restricted" section at the end of a data shred's payload:
https://github.com/solana-labs/solana/blob/master/ledger/src/shred.rs#L44-L50

Furthermore, data shreds that are not filled to capacity with data will have additional 0-padding to get to fill up the packet.

The result of these two items is that the blockstore is getting bloated with extraneous bytes.

Summary of Changes

Trim the extra padding bytes off of the shred on insertion; "re-expand" the bytestream when retrieving from the blockstore to avoid breaking the erasure coding algorithm (which requires that coding and data shreds are of same length).

Fixes #16236

[codecov]

Codecov Report

Merging #16602 (40ed492) into master (8f56c11) will decrease coverage by 0.0%.
The diff coverage is 97.1%.

@@            Coverage Diff            @@
##           master   #16602     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         414      414             
  Lines      115686   115702     +16     
=========================================
+ Hits        95760    95765      +5     
- Misses      19926    19937     +11     

[steviez]

@sakridge - I ended up cherry-picking these commits into my own branch since they were failing the CI before; it appears those failures were either unrelated issues that were fixed or a non-deterministic failure.

Regardless of RocksDB vs. AccountsDB, I think we still want this change to keep our backend datastore as slim as possible, right?

Assuming so, thoughts on any additional testing that should be done for validation of this PR? The .resize() on the vector seems like the only thing that could affect performance

Edit: I'm pushing in one more commit to add a comment or two so CI will re-run, but everything had passed:

[sakridge]

@sakridge - I ended up cherry-picking these commits into my own branch since they were failing the CI before; it appears those failures were either unrelated issues that were fixed or a non-deterministic failure.

Regardless of RocksDB vs. AccountsDB, I think we still want this change to keep our backend datastore as slim as possible, right?

Assuming so, thoughts on any additional testing that should be done for validation of this PR? The .resize() on the vector seems like the only thing that could affect performance

Edit: I'm pushing in one more commit to add a comment or two so CI will re-run, but everything had passed:

Yep, we still want it, exactly as you said to keep the db as small as possible.

[steviez]

Here are the performance run results:
https://solanalabs.slack.com/archives/CP2L2S4KV/p1618955757009100

mean_tps: 21020
max_tps: 50685.5
mean_confirmation_ms: 2244
max_confirmation_ms: 20425
99th_percentile_confirmation_ms: 10463
max_tower_distance: 164
last_tower_distance: 63.5
slots_per_second: 2.041

Does seem to be worse than the recent runs on master's latest (which are hitting 55k+ max TPS)

[sakridge]

Here are the performance run results:
https://solanalabs.slack.com/archives/CP2L2S4KV/p1618955757009100

mean_tps: 21020
max_tps: 50685.5
mean_confirmation_ms: 2244
max_confirmation_ms: 20425
99th_percentile_confirmation_ms: 10463
max_tower_distance: 164
last_tower_distance: 63.5
slots_per_second: 2.041

Does seem to be worse than the recent runs on master's latest (which are hitting 55k+ max TPS)

I think it might be within the noise of this measurement. You could compare the specific insert_shred or the fetch_entries part of replay stats. Those correspond to when we store or read shreds from the db.

[sakridge]

lgtm

[steviez]

I think it might be within the noise of this measurement. You could compare the specific insert_shred or the fetch_entries part of replay stats. Those correspond to when we store or read shreds from the db.

So I dug a little deeper on this and did a comparison of my run with the previous nightly run. For the next two graphs, the first two humps are the previous two nightly runs; the third hump is my run.

Here is insert:

and here is fetch:

Digging deeper on the graphana graphs, the data is pretty "jagged" so think I would agree that any observed difference is within the noise, so I'm happy if you are

[carllin]

nit: Summary of Changes section seems to have been cut off 😃

Pull request has been modified.

[steviez]

Would it make more sense to fold this into test_should_insert_data_shred? That way we can also add a test case in test_should_insert_data_shred

Yeah, I think you're right. We already have another similar validity check in should_insert_data_shred and makes sense to check this as early as possible

Pull request has been modified.