-
Notifications
You must be signed in to change notification settings - Fork 663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix/memoize-block-commit-descendancy #3048
Conversation
…tition_id is the sortition ID of the parent block commit. Use this field to *quickly* search the ancestor block-commits in SortitionHandleTx::descended_from() to see if a block-commit descends from an anchor block (the MARF is no longer queried)
Codecov Report
@@ Coverage Diff @@
## develop #3048 +/- ##
===========================================
+ Coverage 83.60% 83.69% +0.09%
===========================================
Files 248 248
Lines 196992 197466 +474
===========================================
+ Hits 164701 165276 +575
+ Misses 32291 32190 -101
Continue to review full report at Codecov.
|
Feedback: don't change the chainstate schema in an incompatible way; instead, re-calculate |
…ortiiton IDs for block-commits
… request times out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, just had a few superficial comments. Also, please update the CHANGELOG.md
…y, since we open the sortition DB often
reduce DB contention between relayer and chains coordinator threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's a win! Thanks. Two questions about the PR description:
- Is this really true "This PR addresses [chainstate] memoize block-commit descendancy information for
SortitionHandleTx::descended_from
#3045 by adding a new field parent_sortition_id to the block_commits table in the sortition DB"? Seems like you are adding a new tableblock_commit_parents
. - Would be nice if we could quantify "Using this field is much, much faster", but can skip this if it's a huge hassle for some reason.
@@ -2398,8 +2435,8 @@ impl SortitionDB { | |||
pub fn is_db_version_supported_in_epoch(epoch: StacksEpochId, version: &str) -> bool { | |||
match epoch { | |||
StacksEpochId::Epoch10 => false, | |||
StacksEpochId::Epoch20 => (version == "1" || version == "2"), | |||
StacksEpochId::Epoch2_05 => version == "2", | |||
StacksEpochId::Epoch20 => (version == "1" || version == "2" || version == "3"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarification: does epoch20 support version 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The node passes through epoch 2.0 when it's booting. So, if you stop and restart a node while it's still in epoch 2.0, this check will be run and it should pass (since the sortition DB will have the latest schema).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but this should have an entry in the changelog that at least mentions that the update will apply a schema migrations (with a rough time estimate), so that users can expect the delay when they restart their nodes.
This is no longer true, per feedback given here: #3048 (comment). In order to keep the sortition DB compatible with existing sortition DBs, a new table was added instead.
Processing a sortition is about 10x faster now, when considering later sortitions (where the sortition DB MARF is bigger). It takes about 0.3s to process a sortition with this PR, and it took about 3s without it. |
:)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcnelson please update the PR description to match the change!
CHANGELOG.md
Outdated
- Sortition processing performance has been improved by about an order of | ||
magnitude, by avoiding a slew of expensive database reads (#3048). | ||
magnitude, by avoiding a slew of expensive database reads (#3045). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should mention (and warn) about the schema migration. Restarting a node, using the an existing chainstate directory, with this change will require more time than other updates have in the past, so users should get some kind of warning in the changelog (either in this entry, or at the top level content for the release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
I noticed a couple bugs in the implementation when testing this on a live migration:
|
Don't bother doing a migration; just fall back to the old code if the |
…ead, use it opportunistically, and if it doesn't yield any data, do the expensive MARF read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR addresses #3045 by adding a new field
parent_sortition_id
to theblock_commits
table in the sortition DB, which shall be equal to the sortition ID of the parent block-commit. This field is then used inSortitionHandleTx::descended_from
to scan the list of block-commit ancestors to see if the winning block at a given height descends from a PoX anchor block.Using this field is much, much faster than querying the sortition DB MARF for each block height in a reward cycle. In fact, the former behavior is the limiting factor in booting up a node with #3044 applied.
NOTE: this changes the sortition DB schema in an incompatible way. The release that incorporates this PR as-is will need a chainstate version number increment.