test: Remove from_node from create_self_transfer* MiniWallet helpers #25435

1. 
   MarcoFalke commented at 10:03 am on June 21, 2022: member

   MiniWallet is capable to create a transaction without a node, so don’t pass it in where not needed.
2. test: Remove from_node from create_self_transfer* MiniWallet helpers

   The from_node argument is no longer used as of commit
   a55606c3bdbfdf660b093bc2a618d537ffae7f26

   fa8421bc5b
3. fanquake added the label Tests on Jun 21, 2022
4. 
   kouloumos commented at 10:39 am on June 21, 2022: member

   Could this be an opportunity to further reassess the use of the from_node argument?

   Out of the 81 times that is passed as an argument to send_self_transfer, send_to, send_self_transfer_multi and sendrawtransaction only at 9 occurances it uses a node other than the MiniWallet test_node.

   As that argument is passed down to sendrawtransaction, maybe it could be further simplified by changing from_node=None at the rest of the methods and add from_node = from_node or self._test_node in sendrawtransaction

5. 
   MarcoFalke commented at 10:48 am on June 21, 2022: member

   Sure, but let’s create a separate discussion thread for this, see also https://github.com/bitcoin/bitcoin/pull/24025
6. 
   theStack commented at 11:23 am on June 21, 2022: member

   Concept ACK 🔎

   The from_node argument was used for testing mempool validity (via the testmempoolaccept RPC), which was removed in #25356 and is hence not needed anymore.

7. 
   kouloumos commented at 1:02 pm on June 21, 2022: member

   ACK fa8421bc5bcd483a9501257073db17ff2e76eb46

   In order to remove from_node from create_self_transfer*, this PR also changes the method signature of send_self_transfer* to explicitly accept the from_node argument as it cannot access it anymore through kwargs.

   Sure, but let’s create a separate discussion thread for this, see also #24025

   Thanks for the input, I was not aware that this issue was already raised as I initially though of it as less controversial.

8. 
   MarcoFalke commented at 1:19 pm on June 21, 2022: member

   I think it can be done by explicitly passing a default_send_node to MiniWallet, with the disclaimer that it should only be used where no ambiguity exists. For example, for tests that only have one node?
9. 
   kouloumos commented at 3:12 pm on June 21, 2022: member

   I think it can be done by explicitly passing a default_send_node to MiniWallet, with the disclaimer that it should only be used where no ambiguity exists. For example, for tests that only have one node?

   Per your suggestion, I replied on the PR you mentioned, as a more suitable discussion thread.

10. theStack approved
11. 
    theStack commented at 11:34 pm on June 21, 2022: member

    ACK fa8421bc5bcd483a9501257073db17ff2e76eb46

    I think it can be done by explicitly passing a default_send_node to MiniWallet, with the disclaimer that it should only be used where no ambiguity exists. For example, for tests that only have one node?

    Sounds like a good idea to me. If anyone opens a follow-up PR implementing this or any other way to reduce the need to pass explicit from_node arguments (now discussed in #24025), feel free to ping me as reviewer.

12. MarcoFalke merged this on Jun 22, 2022
13. MarcoFalke closed this on Jun 22, 2022

    ---

14. MarcoFalke deleted the branch on Jun 22, 2022
15. sidhujag referenced this in commit 317d05d58a on Jun 22, 2022
16. theStack referenced this in commit f665c6ecda on Jun 27, 2022
17. MarcoFalke referenced this in commit 78957e71e8 on Jun 28, 2022
18. sidhujag referenced this in commit 0a8855a30e on Jun 28, 2022
19. DrahtBot locked this on Jun 22, 2023

Contributors
MarcoFalke kouloumos theStack

Labels
Tests