Skip to content
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

update the StateChangeSummary to not keep full NPCResult objects #16793

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Nov 8, 2023

Purpose:

This is a simplification of the state the Blockchain object returns in its StateChangeSummary class.
Currently, it includes the NPCResult object for the most recent block (and possibly all blocks along a reorg-chain).

The NPCResult is the conditions and created coins from each block. In the case of a reorg, we currently have to re-run all the blocks to produce this state, to be able to return it.

The only things we actually need out of the NPCResult object are additions and removals (and hints). This patch simplifies the StateChangeSummary class to only include the information we need.

The consumers of this information are the mempool (to determine which mempool items are no longer valid) and the hint store, to index hints and update subscriptions.

This is a step towards not having to re-run the blocks in the case of a reorg.

The main change is the members of StateChangeSummary, here: https://github.com/Chia-Network/chia-blockchain/pull/16793/files#diff-40409ea13958d9e8cec2be388c12e68da7c3179de55f01879713c5abdb1fed38R78-R81

Current Behavior:

StateChangeSummary contains information that's expensive to compute.

New Behavior:

StateChangeSummary contains information that's cheaper to compute.

Profile

Profile of the cost of computing the NPCResult in _reconsider_peak():

reorg-node-1-and-3

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Nov 8, 2023
@arvidn arvidn mentioned this pull request Nov 8, 2023
@arvidn arvidn force-pushed the simplify-state-update-summary branch from e4a7ed6 to db1d954 Compare November 8, 2023 13:37
@arvidn arvidn marked this pull request as ready for review November 8, 2023 18:47
@arvidn arvidn requested a review from a team as a code owner November 8, 2023 18:47
@arvidn arvidn changed the title update the StateUpdateSummary to not keep full NPCResult objects update the 'StateChangeSummary` to not keep full 'NPCResult' objects Nov 8, 2023
@arvidn arvidn changed the title update the 'StateChangeSummary` to not keep full 'NPCResult' objects update the 'StateChangeSummary' to not keep full 'NPCResult' objects Nov 8, 2023
@arvidn arvidn changed the title update the 'StateChangeSummary' to not keep full 'NPCResult' objects update the StateChangeSummary to not keep full NPCResult objects Nov 8, 2023
@arvidn arvidn requested a review from fchirica November 8, 2023 18:53
fchirica
fchirica previously approved these changes Nov 9, 2023
Copy link

Pull Request Test Coverage Report for Build 6880536962

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 35 of 35 (100.0%) changed or added relevant lines in 4 files are covered.
  • 70 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-0.05%) to 90.181%

Files with Coverage Reduction New Missed Lines %
chia/data_layer/dl_wallet_store.py 1 95.74%
chia/wallet/util/wallet_sync_utils.py 1 77.21%
chia/data_layer/data_layer_wallet.py 2 91.09%
chia/full_node/full_node.py 3 84.67%
chia/rpc/rpc_server.py 3 87.33%
chia/full_node/weight_proof.py 4 90.73%
chia/introducer/introducer.py 5 79.12%
chia/wallet/wallet_node.py 11 86.54%
chia/timelord/timelord.py 13 72.25%
tests/core/util/test_lockfile.py 27 77.73%
Totals Coverage Status
Change from base Build 6880378616: -0.05%
Covered Lines: 93345
Relevant Lines: 103447

💛 - Coveralls

chia/consensus/blockchain.py Show resolved Hide resolved
chia/full_node/full_node.py Outdated Show resolved Hide resolved
chia/full_node/hint_management.py Show resolved Hide resolved
@arvidn arvidn requested a review from emlowe November 21, 2023 20:20
@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready Cleanup Code cleanup labels Nov 21, 2023
@cmmarslender cmmarslender merged commit 5318514 into main Nov 21, 2023
255 checks passed
@cmmarslender cmmarslender deleted the simplify-state-update-summary branch November 21, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants