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.

Sign up for GitHub

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

Jump to bottom

test: Remove from_node from create_self_transfer* MiniWallet helpers #25435

Merged
merged 1 commit into from
Jun 22, 2022
Merged

test: Remove from_node from create_self_transfer* MiniWallet helpers #25435

merged 1 commit into from
Jun 22, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 21, 2022

MiniWallet is capable to create a transaction without a node, so don't pass it in where not needed.

test: Remove from_node from create_self_transfer* MiniWallet helpers
fa8421b 
The from_node argument is no longer used as of commit
a55606c
@fanquake fanquake added the Tests label Jun 21, 2022
@kouloumos
Copy link
Contributor

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

@maflcko
Copy link
Member Author

maflcko commented Jun 21, 2022

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

theStack
theStack reviewed Jun 21, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kouloumos
kouloumos reviewed Jun 21, 2022
View reviewed changes
Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa8421b

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.

@maflcko
Copy link
Member Author

maflcko commented Jun 21, 2022

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?

@kouloumos kouloumos mentioned this pull request Jun 21, 2022
Closed
@kouloumos
Copy link
Contributor

kouloumos commented Jun 21, 2022 •
edited
Loading

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.

theStack
theStack approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa8421b

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.

@maflcko maflcko merged commit 1b71c76 into bitcoin:master Jun 22, 2022
@maflcko maflcko deleted the 2206-test-miniwallet-no-node-🎳 branch June 22, 2022 05:35
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2022
Merge bitcoin#25435: test: Remove from_node from create_self_transfer…
317d05d 
…* MiniW…
theStack added a commit to theStack/bitcoin that referenced this pull request Jun 27, 2022
@theStack
test: fix failing test interface_usdt_utxocache.py
f665c6e 
The `from_node` argument doesn't exist anymore for
`MiniWallet.create_self_transfer` since PR bitcoin#25435 (commit
fa8421b).
@theStack theStack mentioned this pull request Jun 27, 2022
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 28, 2022
Merge bitcoin#25486: test: fix failing test `interface_usdt_utxocache…
78957e7 
….py`

f665c6e test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner)

Pull request description:

  The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR bitcoin#25435 (commit fa8421b), leading to an error on master:

  ```
  $ sudo ./test/functional/interface_usdt_utxocache.py
  2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1
  2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API
  2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing
  Traceback (most recent call last):
    File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
      self.run_test()
    File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test
      self.test_uncache()
    File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache
      invalid_tx = self.wallet.create_self_transfer(
  TypeError: create_self_transfer() got an unexpected keyword argument 'from_node'
  2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes
  [...]
  ```
  Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.)

ACKs for top commit:
  MarcoFalke:
    cr ACK f665c6e

Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 28, 2022
Merge bitcoin#25486: test: fix failing test `interface_usdt_utxocache…
0a8855a 
….py`

f665c6e test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner)

Pull request description:

  The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR bitcoin#25435 (commit fa8421b), leading to an error on master:

  ```
  $ sudo ./test/functional/interface_usdt_utxocache.py
  2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1
  2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API
  2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing
  Traceback (most recent call last):
    File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
      self.run_test()
    File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test
      self.test_uncache()
    File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache
      invalid_tx = self.wallet.create_self_transfer(
  TypeError: create_self_transfer() got an unexpected keyword argument 'from_node'
  2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes
  [...]
  ```
  Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.)

ACKs for top commit:
  MarcoFalke:
    cr ACK f665c6e

Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6
@bitcoin bitcoin locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kouloumos kouloumos kouloumos left review comments

@theStack theStack theStack approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @kouloumos @theStack @fanquake