wallet: Migrate entire address book entries to watchonly and solvables too#28610
wallet: Migrate entire address book entries to watchonly and solvables too#28610fanquake merged 1 commit intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
achow101
force-pushed
the
migrate-avoidreuse
branch
from
8f191ad to
4fcd3ef
Compare
achow101
force-pushed
the
migrate-avoidreuse
branch
from
4fcd3ef to
a39eff2
Compare
achow101
force-pushed
the
migrate-avoidreuse
branch
2 times, most recently
from
067e09e to
0266501
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 02665016a5870c9fcba049b849a4086b14d08255. Nice cleanup and test!
achow101
force-pushed
the
migrate-avoidreuse
branch
from
0266501 to
406b71a
Compare
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Code review ACK 406b71a
With two non-blocking points:
Firstly, made a test to verify that the transaction's extra information is preserved during migration: c1aabd1. This includes the preservation of bump fee info ('replaces_txid' and 'replaced_by_txid') and the user's custom comments ('comment' and 'comment_to'). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.
Secondly, shilling mode; with #26836, which now is part of #28574, this could have been implemented with even less code.
| wallet = self.create_legacy_wallet("avoidreuse") | ||
| wallet.setwalletflag("avoid_reuse", True) |
There was a problem hiding this comment.
tiny nit: could provide the avoid_reuse flag to createwallet() instead of calling setwalletflag separately.
| self.test_conflict_txs() | ||
| self.test_hybrid_pubkey() | ||
| self.test_failed_migration_cleanup() | ||
| self.test_avoidreuse() |
There was a problem hiding this comment.
Would be good to unload the wallets at the end of the test case.
016cc80 test: wallet migration, add coverage for tx extra data (furszy) Pull request description: Quick follow-up to #28610, coming from #28610 (review). Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration, as well as the extra tx comments. ACKs for top commit: jamesob: Nice, ACK 016cc80 achow101: ACK 016cc80 pablomartin4btc: ACK 016cc80 BrandonOdiwuor: lgtm ACK 016cc80 Tree-SHA512: 697cabece730cbe5c5947bf98455e80a8877c0352fbe2a66362ce5ea530b67882b0bec561a67d48fee200cdad717cd62c57fd809e2a94ff83c3fad30021e1d9e
Github-Pull: bitcoin#28610 Rebased-From: 406b71a
6 participants
Not all of the data in an address book entry was being copied to the watchonly and solvables wallets. This includes information such as whether the address was previously spent, and any receive requests that may exist. A test has been added to check that the previously spent information is copied, although it passes without the changes in this PR since this information is also regenerated when a transaction is loaded/added into a wallet.