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

[CHIA-683] Drop unknown tables when resetting wallet sync DB #18222

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

Quexington
Copy link
Contributor

@Quexington Quexington commented Jun 20, 2024

In the wallet node, there is a function called reset_sync_db the purpose of which is to delete relevant information from specific wallet tables that gives you “sync state” so that you can try again if the wallet gets into a bad state.

There’s a problem with this function, which is that it specifies a list of all existing tables and throws an error if any exist that are not mentioned in that list. When you add a table, you have to add it to that list in order for the function to work but if you downgrade then that addition is no longer in the code, but it the table remains in the DB and the function throws an error about an unexpected table.

The way I see it, there’s a few potential solutions, some more robust than others:

  1. Delete the check about unknown tables - The purpose of this is dubious at best. I can’t personally think of a reason it exists, but it must have been place in there for a reason. Maybe to force new tables to think about how to reset themselves? If so, the test checking that that list is complete should be enough.
  2. Automatically delete unknown tables - If we have downgraded, we don’t necessarily have need of these tables so maybe we can just delete them instead of erroring
  3. Store instructions for how to reset each table in the DB somehow - This is probably the most robust, although most complicated, solution. This removes the reset logic from versions, it simply lives in the DB and the code to reset remains static even as the DB schema changes

This PR takes the second approach to avoid the complexity of the third approach and provide more consistent behavior than the first approach likely provides.

@Quexington Quexington added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jun 20, 2024
@Quexington Quexington changed the title Drop unknown tables when resetting wallet sync DB [CHIA-683] Drop unknown tables when resetting wallet sync DB Jun 20, 2024
@Quexington Quexington mentioned this pull request Jun 20, 2024
5 tasks
@Quexington Quexington marked this pull request as ready for review June 20, 2024 21:51
@Quexington Quexington requested a review from a team as a code owner June 20, 2024 21:51
@arvidn
Copy link
Contributor

arvidn commented Jun 21, 2024

my understanding is that there are two kinds of tables in the wallet DB:

  1. Tables that are built by syncing with the chain (and can be re-created by re-syncing the wallet)
  2. Tables that contain information that cannot be derived from the chain (and your private key)

This function is supposed to only delete the first kind, to be better than deleting the whole DB. I imagine a long term improvement would be to separate (1) and (2) into separate database files. With your change, is there a risk you delete tables of type 2?

Did you dig through git and github history to try to understand the rationale for the error? I agree that it does sound a bit unintuitive at first glance.

@arvidn
Copy link
Contributor

arvidn commented Jun 21, 2024

it seems reasonable to leave unknown tables in the database. The case where we downgrade and resync, we're syncing with the old version anyway, the unknown tables won't interfere with that. If we ever upgrade again, those tables may be useful if they are of type (2).

@Quexington
Copy link
Contributor Author

my understanding is that there are two kinds of tables in the wallet DB:

  1. Tables that are built by syncing with the chain (and can be re-created by re-syncing the wallet)
  2. Tables that contain information that cannot be derived from the chain (and your private key)

This function is supposed to only delete the first kind, to be better than deleting the whole DB. I imagine a long-term improvement would be to separate (1) and (2) into separate database files. With your change, is there a risk you delete tables of type 2?

Did you dig through git and github history to try to understand the rationale for the error? I agree that it does sound a bit unintuitive at first glance.

We talked about this in a standup and did identify that both types of data exist in the database but none of it is critical, more for display. The practice of blowing away the wallet DB has been widespread for quite some time and this function as I remember it being presented at the time was just as a way to make that process more ergonomic and faster. I would agree in the future to separate out the two types of data and would assume it would be a dependency of any work that intended on adding some data of category (2) to the wallet.

Like I said in the PR description, the approach here is taken for the purposes of having consistent behavior. When you use this function on a downgraded version, you get set to a known state for that version rather than leaving around some side effects whose form can become corrupted in any number of ways when switching from version to version. If you re-upgrade, that state is not likely to be very helpful and may actually be confusing because it contains state for a certain range of the blockchain and is missing some state for a range that we synced on a lower version. It's a much easier cognitive burden when trying to troubleshoot to say that when you re-upgrade, it's as if you've done it for the first time.

@Quexington Quexington closed this Jun 24, 2024
@Quexington Quexington reopened this Jun 24, 2024
@Quexington Quexington closed this Jul 8, 2024
@Quexington Quexington reopened this Jul 8, 2024
Copy link

Pull Request Test Coverage Report for Build 9846380723

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

  • 12 of 12 (100.0%) changed or added relevant lines in 2 files are covered.
  • 31 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.009%) to 90.973%

Files with Coverage Reduction New Missed Lines %
chia/_tests/simulation/test_simulation.py 1 96.5%
chia/full_node/full_node.py 1 85.49%
chia/daemon/client.py 1 73.94%
chia/server/node_discovery.py 2 80.67%
chia/wallet/wallet_node.py 4 88.82%
chia/server/server.py 4 82.58%
chia/server/address_manager.py 7 90.48%
chia/timelord/timelord.py 11 72.49%
Totals Coverage Status
Change from base Build 9846325110: -0.009%
Covered Lines: 101604
Relevant Lines: 111638

💛 - Coveralls

@Quexington Quexington added the ready_to_merge Submitter and reviewers think this is ready label Jul 9, 2024
@Starttoaster Starttoaster merged commit 6d03354 into main Jul 9, 2024
1032 of 1050 checks passed
@Starttoaster Starttoaster deleted the quex.reset_sync_db_forwards_compat branch July 9, 2024 16:53
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 ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants