feat: improve block selection in stacks-inspect
#6735
Conversation
Improves the UX by merging `validate-naka-block` and `validate-block,` improves the lookup time by optimizing the queries, and improves the output for a better experience. Adds `--early-exit` flag to exit on first error instead of completing all blocks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6735 +/- ##
===========================================
+ Coverage 70.05% 71.95% +1.89%
===========================================
Files 578 579 +1
Lines 358373 360160 +1787
===========================================
+ Hits 251064 259140 +8076
+ Misses 107309 101020 -6289
... and 390 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
contrib/stacks-inspect/src/lib.rs
Outdated
| panic!("Failed to open staging blocks DB at {staging_blocks_db_path}: {e}"); | ||
| }); | ||
| let sql = format!( | ||
| "SELECT index_block_hash, consensus_hash, anchored_block_hash, height FROM staging_blocks {clause}" |
There was a problem hiding this comment.
nit: since we are only consuming index_block_hash, maybe we can simplity the query as SELECT index_block_hash FROM staging_blocks {clause}
There was a problem hiding this comment.
Yeah, I had that there originally because I needed other fields when I was also reusing the new structure in other places, but then ended up deleting that code. I'll delete these now.
contrib/stacks-inspect/src/lib.rs
Outdated
| for (i, index_block_hash) in index_block_hashes.iter().enumerate() { | ||
| if i % 100 == 0 { | ||
| println!("Checked {i}..."); | ||
| let sql = format!("SELECT index_block_hash, height FROM nakamoto_staging_blocks {clause}"); |
There was a problem hiding this comment.
nit: since we are only consuming index_block_hash, maybe we can simplity the query as SELECT index_block_hash FROM nakamoto_staging_blocks {clause}
contrib/stacks-inspect/src/lib.rs
Outdated
| let mut seen = HashSet::new(); | ||
| let mut entries = Vec::new(); |
There was a problem hiding this comment.
suuuuper nit: These two variables hold over 5 million index_block_hash values each when the function is called with BlockSelection::All, stored as 64 bytes hex strings. ~320 MB for each variable (plus additional overhead from the data structures). It's not a concern right now, but we could reduce memory usage by about half if we move the
let index_block_hash = StacksBlockId::from_hex(&index_block_hash_hex).unwrap();processing inside this function and avoid keeping the hex strings around.
There was a problem hiding this comment.
as always, feel free to skip the changes for the nits
There was a problem hiding this comment.
This is a good point. Also, a good reason to just remove the seen tracking altogether.
Remove the `seen` tracking since it doesn't really gain us much and uses a lot of space when processing many blocks.
| next_staging_block.commit_burn, | ||
| next_staging_block.sortition_burn, | ||
| ); | ||
| Ok(()) |
There was a problem hiding this comment.
I assume you meant to change replay_block to return a Result<,> and then return that result here?
Returning an unconditional Ok(()) regardless of what happens in replay_block (if it even ever returns -- it has some process::exits in there) doesn't seem like it's the intent.
There was a problem hiding this comment.
The way it is now, replay_block only returns if it is successful. The error cases panic or process::exit, so it seems okay to do this here. You're probably right that it would be better to change that to return an error and pass that up.
There was a problem hiding this comment.
So these would all be considered success cases?
stacks-core/contrib/stacks-inspect/src/lib.rs
Lines 874 to 875 in 9203886
stacks-core/contrib/stacks-inspect/src/lib.rs
Lines 887 to 888 in 9203886
stacks-core/contrib/stacks-inspect/src/lib.rs
Lines 913 to 921 in 9203886
(This is an honest question -- I'm not familiar enough with all this, especially the pre-naka stuff, to judge this. Totally possible that these are all considered "valid" results in this context.)
There was a problem hiding this comment.
These would represent errors in the chainstate, so technically not what is intended to be tested here, but it would probably be nice to actually make these errors more noticeable, rather than just printing to stdout and returning. It wouldn't be the worst thing to refactor this to return an error.
This option adds complexity to the block selection across pre-Nakamoto / Nakamoto blocks and it doesn't seem useful, so remove it. Also, make `range` exclusive of the `end` height.
benjamin-stacks
left a comment
benjamin-stacks
left a comment
There was a problem hiding this comment.
Left another comment about a question I'm not sure about, but in either case this change is an improvement on the status quo, so I'm happy to approve as is.
|
I think @wileyj should try this patch out on a block-replay before merging, because the corresponding block validation script will need to be updated to use this new CLI. That said, I'm kinda iffy on whether or not it was a good idea to put |
|
it's a block validation party today - will start once #6754 is complete. |
|
updated the validation script, and a few things are odd so far (not sure if it's bad or not, but it is behaving differently than stacks-inspect in 3.3.0.0.3 for example). for comparison, in 3.3.0.0.3 - each partition took roughly the same amount of time when using the (deprecated in this PR) function i'm running a complete validation now, will share more details once it's complete and the logs are full. |
|
attaching output from validating all blocks using as noted on the call, since hte blocks are processed serially - it makes more sense why the earlier blocks are validated much faster than the later blocks, and why some partitions take substantially longer than with version of stacks-inspect in develop branch (which doesn't process the blocks serially). overall, a full run with from this branch took roughly 13 hours whereas from develop it was around 11 hours. i will also introduce a block failure and re-run to see how the binary change logs validation failures |
|
@brice-stacks for readability - this is a little awkward. would be it possible to put the failure on a separate line? note that this is a synthetic failure i introduced for epoch 2.5 to see what the output looks like since the use case here is to not stop once a failure is discovered, it's helpful to have each one printed on their own line - that may be tricky with the change to a % based output though 🤔 |
|
I'm going to put the |
5 participants
Improves the UX by merging
validate-naka-blockandvalidate-block,improves the lookup time by optimizing the queries, and improves the output for a better experience. Adds--early-exitflag to exit on first error instead of completing all blocks.