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

Add use_delta_sync option #17573

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

felixbrucker
Copy link
Contributor

Purpose:

Improve wallet sync speed

Current Behavior:

Currently there are a few places where subscriptions for puzzle hash and coin updates are requested from 0 every time, instead of the fork height for this sync. This is slow.

New Behavior:

If the use_delta_sync option in the wallet config section of the config.yaml is set to true the fork height of the sync is used instead and wallet syncing is fast(er).

Note:

Generating new puzzle hashes will always sync them from 0, the same is true for subscribing to interested coin ids and puzzle hashes. IMHO the config option could be removed and a delta sync being used always, let me know if you have reservations with that, for now its just a config option which is not enabled by default.

@felixbrucker felixbrucker requested a review from a team as a code owner February 16, 2024 17:30
Copy link

coveralls-official bot commented Feb 16, 2024

Pull Request Test Coverage Report for Build 8189794206

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 30 of 30 (100.0%) changed or added relevant lines in 3 files are covered.
  • 1432 unchanged lines in 52 files lost coverage.
  • Overall coverage increased (+0.06%) to 91.022%

Files with Coverage Reduction New Missed Lines %
chia/rpc/rpc_client.py 1 98.91%
chia/consensus/block_record.py 1 95.24%
tests/wallet/cat_wallet/test_cat_wallet.py 1 99.84%
tests/fee_estimation/test_fee_estimation_unit_tests.py 1 99.06%
chia/daemon/client.py 1 85.0%
chia/util/json_util.py 1 88.46%
tests/util/test_priority_mutex.py 1 99.66%
chia/rpc/rpc_server.py 1 88.4%
tests/util/full_sync.py 1 88.89%
tests/core/full_node/stores/test_block_store.py 1 99.19%
Totals Coverage Status
Change from base Build 7941879901: 0.06%
Covered Lines: 97555
Relevant Lines: 107146

💛 - Coveralls

Rigidity
Rigidity previously approved these changes Mar 5, 2024
Copy link
Contributor

@Rigidity Rigidity left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I'm not @Chia-Network/required-reviewers, so will need another pair of eyes on it.

@Rigidity Rigidity added the Added Required label for PR that categorizes merge commit message as "Added" for changelog label Mar 5, 2024
chia/wallet/wallet_node.py Show resolved Hide resolved
tests/wallet/sync/test_wallet_sync.py Outdated Show resolved Hide resolved
chia/util/initial-config.yaml Show resolved Hide resolved
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Mar 14, 2024
@Rigidity
Copy link
Contributor

@felixbrucker looks like there's now a conflict presumably due to #17209

A comment/warning in the default config would also be nice to have, explaining that if you use this option you run the risk (even if it's not likely to be an issue) of missing coin state due to reorgs.

Other than that I don't personally have any issue with landing this.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Mar 15, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Rigidity Rigidity requested review from Rigidity, arvidn and a team March 15, 2024 12:48
Copy link
Contributor

@Rigidity Rigidity left a comment

Choose a reason for hiding this comment

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

Looks good to me. However, I don't have permission to merge, so will need more review(s).

@pmaslana pmaslana merged commit acaf7e0 into Chia-Network:main Mar 15, 2024
279 checks passed
@felixbrucker felixbrucker deleted the add-delta-sync-option branch March 15, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added Required label for PR that categorizes merge commit message as "Added" for changelog enhancement New feature or request Wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants