Revert “Add to spends only transcations from me” #24083

1. 
   S3RK commented at 7:56 am on January 17, 2022: member

   This reverts commit d04566415e16ae685af066384f346dff522c068f from #22929.

   This commit was based on invalid assumption that mapTxSpends should contain only outgoing txs and broke wallet conflicts feature.

2. Revert "Add to spends only transcations from me"

   This reverts commit d04566415e16ae685af066384f346dff522c068f.

   3b98bf9c43
3. 
   S3RK commented at 7:56 am on January 17, 2022: member

   cc @luke-jr @achow101
4. fanquake added the label Wallet on Jan 17, 2022
5. 
   achow101 commented at 6:06 pm on January 17, 2022: member

   Can we get a test for this behavior?
6. 
   DrahtBot commented at 6:25 pm on January 17, 2022: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #22693 (RPC/Wallet: Add “use_txids” to output of getaddressinfo by luke-jr)

   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.

7. 
   S3RK commented at 8:55 am on January 18, 2022: member

   Can we get a test for this behavior?

   Added a test, that fails before the revert and passes after. Not sure if there are other things to test.

   But on the second thought, I should’ve just added some new assertions in an existing test rather than a create a new test file. I’ll find a new home for it, maybe in wallet_abandonconflict.py. Wdyt?

8. test: add more wallet conflicts assertions

   verify wallet conflicts from the receiver's side

   3ee6d0788e
9. S3RK force-pushed on Jan 20, 2022
10. 
    S3RK commented at 9:15 am on January 20, 2022: member

    Moved tests to the existing file. This is ready for review
11. 
    achow101 commented at 9:54 pm on January 20, 2022: member

    The rename in the test seems unnecessary. It adds a lot of noise to the commit.
12. 
    S3RK commented at 10:04 am on January 21, 2022: member

    The rename in the test seems unnecessary. It adds a lot of noise to the commit.

    We need two wallets on the same node to verify the intended behavior. That means we need to specify wallet each time we call an RPC.

    One way we can workaround that is to unload the second wallet. But I’m not convinced this is better as it makes the test more fragile. Do you have other ideas how to avoid renames?

13. 
    achow101 commented at 6:58 pm on January 21, 2022: member

    We need two wallets on the same node to verify the intended behavior. That means we need to specify wallet each time we call an RPC.

    Ah, it looked like you were just replacing self.nodes[0] and self.nodes[1] with alice, and bob, but looking more closely, it is actually different.

14. 
    achow101 commented at 6:59 pm on January 21, 2022: member

    ACK 3ee6d0788ec1b90f7c39c9644dba4011f7cf5db4
15. 
    S3RK commented at 6:58 am on January 27, 2022: member

    Do we want to have it in 23.0 milestone?
16. MarcoFalke added this to the milestone 23.0 on Jan 27, 2022
17. luke-jr approved
18. 
    luke-jr commented at 3:42 am on January 29, 2022: member

    Confirmed:

    • included test fails on master (for expected reason) and passes on PR
    • revert is a clean and accurate revert
    • code changes look reasonable

    I think it’d be a better test if the wallets aren’t in the same node, but I guess this is fine.

    tACK

19. achow101 merged this on Feb 1, 2022
20. achow101 closed this on Feb 1, 2022

    ---

21. sidhujag referenced this in commit f2c3e4f8ec on Feb 1, 2022
22. DrahtBot locked this on Feb 1, 2023

Contributors
S3RK achow101 DrahtBot luke-jr

Labels
Wallet

Milestone
23.0