-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
test: MiniWallet: support default from_node
for sending/creating txs
#24025
Conversation
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.
cee2bd4
to
2da3326
Compare
Not sure if this is useful. Otherwise you could also create a "default from node" for 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. |
@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, As a possibly less controversial suggestion, would it at least make sense to introduce the default parameter for |
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)).
Yeah, should be uncontroversial |
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
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.
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 |
I don't think this is true. Those will remain:
|
Yes, I'm not sure what I was thinking😅 |
In most functional tests using MiniWallet, the node that is used for the sending/creation of transactions (parameter
from_node
to the methodssend_to
,create_self_transfer
,send_self_transfer
andsend_to
) is identical to the one passed to the MiniWallet instance (parametertest_node
). This especially applies for tests with only one nodeself.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. passingfrom_node
explicitely only remains if it differs from the wallettest_node
or if it is important for readability (e.g. inp2p_feefilter.py
, two different nodes are used intermittently).The instances to tackle were identified via
git grep from_node= ./test/functional/*.py
.