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

Fix DID resync #15675

Merged
merged 9 commits into from
Oct 5, 2023
Merged

Fix DID resync #15675

merged 9 commits into from
Oct 5, 2023

Conversation

ytx1991
Copy link
Contributor

@ytx1991 ytx1991 commented Jul 1, 2023

Purpose:

During the resync, it may create DID wallets that don't belong to the current key. These wallets will not be removed after fully synced. It confuses users.

Current Behavior:

New Behavior:

Only create DID wallet when we see the DID coin the first time, and it is unspent.

Testing Notes:

@ytx1991 ytx1991 added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Jul 1, 2023
@ytx1991 ytx1991 requested a review from a team as a code owner July 1, 2023 16:52
Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

There is a print which should be dropped. And do you understand why this even happens during a sync because actually the coin states should be sorted so that unspent coin states come before spent ones? I know there are cases where we due to batching/task processing of the coin states might still process them out of order but here this should probably not be this case.. ? After quick check this feels like this might become flaky because you not really force the coin states to be out of order in the added test.

chia/wallet/wallet_state_manager.py Outdated Show resolved Hide resolved
@ytx1991
Copy link
Contributor Author

ytx1991 commented Jul 6, 2023

I have added a check while fixing Clawback resync. It will log something if the coin state in add_coin_states is unspent but in the latest block the coin state is spent. I resynced several times but never see that log printed so I assume during the resync, the wallet will not receive an unspent coin state for a spent coin. I thought there should be two coin states for a spent coin, but seems like it only has one coin state.

@ytx1991 ytx1991 requested a review from xdustinface July 6, 2023 18:22
Copy link
Contributor

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

so I assume during the resync, the wallet will not receive an unspent coin state for a spent coin.

Okay yeah thats right. I digged more into this and its indeed because we alter the states in the full node and of course then only send the spent states after.

I think it would be good to add the the proposed renaming for the test, it makes it easier to read/understand it.

Thinking about this test maybe you should just resync both wallets and also assert at the end that the DID wallets (not) exists for the particular wallet instead of only checking the tx record and unspent coin count?

I also think, now you only test the resync after the coin was spent but not if the coin is still unspent. Do you think we should test all cases?

tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
@ytx1991 ytx1991 requested a review from xdustinface July 25, 2023 18:35
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Show resolved Hide resolved
tests/wallet/did_wallet/test_did.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Oct 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

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

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

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

wjblanke
wjblanke previously approved these changes Oct 5, 2023
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok thanks kyle

@cmmarslender cmmarslender merged commit ad267b1 into main Oct 5, 2023
239 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants