wallet: Detect DuplicateAddress earlier #610

1. 
   hebasto commented at 5:25 pm on May 29, 2022: member

   This PR makes check for DuplicateAddress error easier and earlier.
2. qt, wallet: Detect `DuplicateAddress` earlier c6832ce992
3. hebasto added the label Wallet on May 29, 2022
4. 
   furszy commented at 6:29 pm on May 29, 2022: member

   Could move it even earlier in the flow and remove the code from the model (which requires the wallet to be locked):

   Inside SendCoinsDialog ::PrepareSendText, before calling WalletModel::prepareTransaction, we are looping over all the entries to load up the recipients list –> Line 257 of SendCoinsDialog.

5. 
   hebasto commented at 8:18 am on May 30, 2022: member

   which requires the wallet to be locked

   Do you mean “unlocking” using WalletModel::UnlockContext?

   Inside SendCoinsDialog ::PrepareSendText, before calling WalletModel::prepareTransaction, we are looping over all the entries to load up the recipients list –> Line 257 of SendCoinsDialog.

   Would it make logic a bit messier?

6. 
   furszy commented at 2:41 pm on May 30, 2022: member

   Do you mean “unlocking” using WalletModel::UnlockContext?

   Yeah. I skipped the first two letters.

   Would it make logic a bit messier?

   I think that the current logic is messy actually. We are already validating all the recipients’ data there. Check this out: https://github.com/furszy/bitcoin/commit/6115d09946b2fa3b25d005f748a5f9a0992c973b . I made the code flow cleaner there.

   Before creating the recipient and adding it to the temporal recipients vector, we call to SendCoinsEntry::validate, which validates the address and the amount (failing if the address is invalid, or the amount is 0 or dust). So, when we are inside WalletModel:: prepareTransaction, we already know whether the address and/or the amount are valid or not. Those checks are duplicated.

   I reworked the area a bit here (stopped after few commits to not enter in a long rabbit hole): https://github.com/furszy/bitcoin/commits/220529-dupaddr

   Feel free to cherry-pick any of those commits if you like them, all yours :). Maybe we could team up to get deeper over this rabbit hole in follow-up PRs.

7. 
   DrahtBot commented at 9:44 pm on June 12, 2022: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

8. 
   hebasto commented at 10:04 am on October 24, 2022: member

   Going to leave this PR up for grabs.

   cc @furszy

9. hebasto closed this on Oct 24, 2022

   ---

10. hebasto added the label Up for grabs on Oct 24, 2022
11. bitcoin-core locked this on Oct 24, 2023

Contributors
hebasto furszy DrahtBot

Labels
Up for grabs Wallet