test: MiniWallet: support default from_node for sending/creating txs #24025

1. 
   theStack commented at 9:58 pm on January 10, 2022: member

   In most functional tests using MiniWallet, the node that is used for the sending/creation of transactions (parameter from_node to the methods send_to, create_self_transfer, send_self_transfer and send_to) is identical to the one passed to the MiniWallet instance (parameter test_node). This especially applies for tests with only one node self.nodes[0].

   This PR changes those methods to support a default from_node and applies it to all functional tests where it makes sense, i.e. passing from_node explicitely only remains if it differs from the wallet test_node or if it is important for readability (e.g. in p2p_feefilter.py, two different nodes are used intermittently).

   The instances to tackle were identified via git grep from_node= ./test/functional/*.py.

2. test: MiniWallet: support default `from_node` for sending/creating txs

   If no `from_node` parameter is passed explicitely to the
   {send,create}... methods, the test node passed in the course
   of creating the MiniWallet instance is used.  This seems to
   be the main use-case in most of the current functional
   tests, i.e. in many instances the calls can be shortened.

   f52b77d25b
3. test: refactor: only specify `from_node` if necessary 2da33266aa
4. theStack force-pushed on Jan 10, 2022
5. DrahtBot added the label Tests on Jan 10, 2022
6. 
   MarcoFalke commented at 8:55 am on January 11, 2022: member

   Not sure if this is useful. Otherwise you could also create a “default from node” for sendrawtransaction or other RPCs. However, this will make tests harder to read and understand, as it is unclear which node the tx was sent from.

   The MiniWallet acts as a shared wallet between all nodes spun up in the test (as opposed to a wallet that is attached and managed by a specific node).

   I think the only place where this makes sense is for tests that only spin up one node.

7. 
   theStack commented at 12:06 pm on January 11, 2022: member

   @MarcoFalke: Fair points, though I would counter the general “will make tests harder to read and understand” argument with the fact that for the vast majority of tests the origin of a tx is not relevant at all, as we just want to fill the mempools of all nodes (in the rare cases where different mempool- or policy-related settings are used, or the nodes are not all connected, from_node could still be passed explicitly). In a personal experience it feels odd to already need to pass a specific node on the MiniWallet instance but then still having to pass from_node again and again for trivial tests.

   As a possibly less controversial suggestion, would it at least make sense to introduce the default parameter for create_self_transfer? The node parameter is only used for the testmempoolaccept RPC and the tx is not sent yet, i.e. using self._test_node as default for that purpose seems to be fine. send_self_transfer and send_to would of course still call create_self_transfer with the still mandatory from_node argument.

8. 
   MarcoFalke commented at 12:32 pm on January 11, 2022: member

   want to fill the mempools of all nodes

   Ok, unrelated, but I think we should make that explicit. Not sure how successful such a patch would be, considering that #23300 took two years (https://github.com/bitcoin/bitcoin/pull/20362). But overall we should be moving into that direction, given that racy tests will inevitably lead to bugs (https://github.com/bitcoin/bitcoin/pull/22437#discussion_r779593489).

   less controversial

   Yeah, should be uncontroversial

9. 
   theStack commented at 3:06 pm on January 11, 2022: member

   Closing this now, as there are good arguments against it and it’s very unlikely to get merged; #24035 includes a stripped-down variant of this PR that only changes create_self_transfer (commit https://github.com/bitcoin/bitcoin/pull/24035/commits/fb26175b27cf5c873d196642b757271b8e670a70).
10. theStack closed this on Jan 11, 2022

    ---

11. MarcoFalke referenced this in commit 807169e10b on Jan 13, 2022
12. theStack deleted the branch on Jun 21, 2022
13. 
    kouloumos commented at 3:12 pm on June 21, 2022: member

    As I raised the same issue on #25435 (comment) and without being aware of this PR, I’ll reply here, as a more suitable discussion thread.

    In a personal experience it feels odd to already need to pass a specific node on the MiniWallet instance but then still having to pass from_node again and again for trivial tests

    After #25435 there will be no usage of test_node in the MiniWallet logic, so one approach that will also make clear that “The MiniWallet acts as a shared wallet between all nodes spun up in the test” could be to remove the test_node argument. Therefore having the shared wallet and from_node indicating the node that each tx is send. EDIT: NOT TRUE

    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? (src)

    That would make sense. I was thinking that a way to be sure that this is used where no ambiguity exists could be the introduction of a use_miniwallet set_test_param that will create an instance of the MiniWallet that will only have a default_send_node when there is only one node. This will also allow for one less import on each test file that is using the MiniWallet. However, this approach has an issue as it does not take into consideration the different modes that the MiniWallet can have.

14. 
    MarcoFalke commented at 3:30 pm on June 21, 2022: member

    After #25435 there will be no usage of test_node in the MiniWallet logic

    I don’t think this is true. Those will remain:

    0test/functional/test_framework/wallet.py:            self._scriptPubKey = bytes.fromhex(self._test_node.validateaddress(self._address)['scriptPubKey'])
    1test/functional/test_framework/wallet.py:        res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
    2test/functional/test_framework/wallet.py:        blocks = self._test_node.generatetodescriptor(num_blocks, self.get_descriptor(), **kwargs)
    3test/functional/test_framework/wallet.py:            block_info = self._test_node.getblock(blockhash=b, verbosity=2)
    

15. 
    kouloumos commented at 6:33 pm on June 21, 2022: member

    I don’t think this is true. Those will remain:

    Yes, I’m not sure what I was thinking😅

16. DrahtBot locked this on Jun 21, 2023

Contributors
theStack MarcoFalke kouloumos

Labels
Tests