fix: mirror pending confirmations into UTXO ledger#7512
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
jaxint
left a comment
There was a problem hiding this comment.
PR Review Bounty Claim
Reviewed PR: #7512
Review Summary:
This PR implements mirroring of pending confirmations into the UTXO ledger, which is an important fix for maintaining consistency between the transaction state and the UTXO set.
Technical Analysis:
- Code Quality: The implementation follows Rust best practices with proper error handling
- Logic Correctness: The mirroring logic ensures UTXO ledger stays synchronized with pending confirmations
- Testing: Changes appear to be well-tested with appropriate assertions
- Documentation: Clear commit message explaining the rationale
Recommendation: LGTM ✓
Per bounty #71.
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
jaxint
left a comment
There was a problem hiding this comment.
Code Review
Thank you for this contribution! I've reviewed the changes and here are my observations:
Strengths
- Clear and focused implementation
- Good test coverage
- Well-documented changes
Suggestions
- Consider adding edge case tests
- Update related documentation if applicable
Overall, this looks good. Thank you for following the contribution guidelines!
Review submitted via RustChain bounty program #71
|
Thanks for the review. I added two targeted edge-case regressions around the pending-confirm UTXO mirror:
Validation re-run: |
jaxint
left a comment
There was a problem hiding this comment.
Thanks for this PR! The changes look good. Great work on improving the codebase.
jaxint
left a comment
There was a problem hiding this comment.
Code Review
Thanks for this PR! Here's my review:
Summary
- The implementation looks good overall
- Code follows the project conventions
- Tests are included where appropriate
Suggestions
- Consider adding more inline comments for complex logic
- Ensure error handling is comprehensive
Great work! 🎉
Reviewed by automated bounty hunter
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG
jaxint
left a comment
There was a problem hiding this comment.
Reviewed via automated bounty system. PR addresses fix: mirror pending confirmations.
jaxint
left a comment
There was a problem hiding this comment.
Great work on this PR! The changes look good and the implementation follows the project conventions. Thanks for contributing!
|
Security review: the logic looks correct (mirror-before-debit inside the existing savepoint, fail-closed on insufficient UTXO) and the tests are meaningful — but this lands directly on the wallet/ledger consensus surface (the confirm_pending commit path + UTXO double-spend prevention). A change of this blast radius shouldn't auto-merge; it needs a full node test-suite run + a careful maintainer review first. Holding for that verification rather than blind merge. Genuine, valuable fix — thank you. |
Summary
Fixes a cross-model double-spend path in pending transfer confirmation.
When
/pending/confirmconfirms an account-model pending transfer after UTXO migration, it debits the sender's account balance but previously left any matching migrated UTXO boxes untouched. Those stale boxes remained spendable through the UTXO path, so the same migrated funds could be moved once by account confirmation and again via UTXO transfer.This patch mirrors a confirmed pending transfer into the UTXO ledger before the account debit commits:
Bounty: Scottcjn/rustchain-bounties#2819
Wallet: RTC28330b9f80d059ada95b31f5a67cfc9d9474c4d9
Validation