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-2224] Add some extra safety into create_message_spend #19153

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

Quexington
Copy link
Contributor

Related to an issue #19152 where recovery information was accidentally cleared during a routine message spend. There's no reliable reproduction for this issue so it's tough to say if this will fix it, but my best guess is that the self.did_info was not fully in sync before the user was allowed to submit a transaction that uses that information. This is a big architectural problem with the DID wallet but without a complete overhaul, I'm not sure how we can fix it. This PR is a small step that should add some extra safety by ensuring that the inner puzzle we think we have does actually match what is in the coin and then using the information directly from that inner puzzle to create the new one. It should be impossible with these changes to do a message spend where any recovery or otherwise metadata can change during the creation of the spend.

@Quexington Quexington added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Jan 15, 2025
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.

Other than these (which should fix the CI error?), looks good

chia/wallet/did_wallet/did_wallet.py Outdated Show resolved Hide resolved
chia/wallet/did_wallet/did_wallet.py Outdated Show resolved Hide resolved
@Quexington Quexington marked this pull request as ready for review January 16, 2025 15:39
@Quexington Quexington requested a review from a team as a code owner January 16, 2025 15:39
@Quexington Quexington closed this Jan 16, 2025
@Quexington Quexington reopened this Jan 16, 2025
Copy link
Contributor

File Coverage Missing Lines
chia/wallet/did_wallet/did_wallet_puzzles.py 75.0% lines 90, 92
Total Missing Coverage
13 lines 2 lines 84%

Copy link
Contributor

@markelrod markelrod left a comment

Choose a reason for hiding this comment

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

coverage diff ok

@Quexington Quexington added the ready_to_merge Submitter and reviewers think this is ready label Jan 24, 2025
@pmaslana pmaslana merged commit 7c01b87 into main Jan 27, 2025
712 of 717 checks passed
@pmaslana pmaslana deleted the quex.did_message_spend_safety_check branch January 27, 2025 16:00
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 ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants