Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

theStack
Copy link
Contributor

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.

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.
@theStack theStack force-pushed the 202201-test-MiniWallet-use_default_node_send_create_txs branch from cee2bd4 to 2da3326 Compare January 10, 2022 22:01
@DrahtBot DrahtBot added the Tests label Jan 10, 2022
@maflcko
Copy link
Member

maflcko commented Jan 11, 2022

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.

@theStack
Copy link
Contributor Author

theStack commented Jan 11, 2022

@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.

@maflcko
Copy link
Member

maflcko commented Jan 11, 2022

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 (#20362). But overall we should be moving into that direction, given that racy tests will inevitably lead to bugs (#22437 (comment)).

less controversial

Yeah, should be uncontroversial

@theStack
Copy link
Contributor Author

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 fb26175).

@theStack theStack closed this Jan 11, 2022
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 13, 2022
aa8a65e test: use MiniWallet for mempool_accept.py (Sebastian Falbesoner)
b24f6c6 test: MiniWallet: support default `from_node` for creating txs (Sebastian Falbesoner)
f30041c test: create txs with current `nVersion` (2) by default (Sebastian Falbesoner)
2f79786 test: refactor: add constant for sequence number `SEQUENCE_FINAL` (Sebastian Falbesoner)

Pull request description:

  This PR enables one more of the non-wallet functional tests (mempool_accept.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in bitcoin#20078.

  It also includes some other minor changes that came up while working on the replacement:
  * [commit 1/4] replace magic number 0xffffffff for a tx's nSequence with a new constant `SEQUENCE_FINAL`
  * [commit 2/4] create `CTransaction` instances with the current nVersion=2 by default, in order to use BIP68 for tests
  * [commit 3/4] support default `from_node` parameter for creating txs (this is a stripped down version of PR bitcoin#24025)

ACKs for top commit:
  MarcoFalke:
    re-ACK aa8a65e 📊

Tree-SHA512: 34cd085ea4147ad5bd3f3321c84204064ceb95f382664c7fe29062c1bbc79d9d9465c5e46d35e11c416f2f3cd46030c90a09b518c829c73ae40d060be5e4c9cb
@theStack theStack deleted the 202201-test-MiniWallet-use_default_node_send_create_txs branch June 21, 2022 11:09
@kouloumos
Copy link
Contributor

kouloumos commented Jun 21, 2022

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.

@maflcko
Copy link
Member

maflcko commented Jun 21, 2022

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

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

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

@kouloumos
Copy link
Contributor

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

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

@bitcoin bitcoin locked and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants