test: use `MiniWallet` in `test/functional/interface_zmq` by josibake · Pull Request #24653 · bitcoin/bitcoin

josibake · 2022-03-23T16:02:37Z

While working on #24584 , interface_zmq started failing due to coin selection not running deterministically. The test doesn't actually need the wallet, so this PR migrates it to use MiniWallet

Note for reviewers: the second commit moves large chunks of code out of an if block, so it may be helpful to review with something that ignores whitespace, e.g git diff -w master

maflcko · 2022-03-23T16:15:23Z

test/functional/interface_zmq.py

I think you can just assign to orig_txid_2 to minimize the diff (also elsewhere)

fixed! in the other cases where i reassign, i belive its because i need both the txid, wtxid, and tx body. but ill double check

okay, in 7f943aa ive got the diff down

I don't think this is fixed?

where are you still seeing an issue? I renamed orig_tx_2 -> orig_txid_2 and also added payment_txid as a variable to avoid touching lines just to rename payment_txid -> payment_tx['txid']

where are you still seeing an issue?

In GitHub.

Usually GitHub will display "outdated" if a review comment was addressed.

You can also check the files tab and use your browsers search feature to see that the orig_tx_2 still exists.

sorry, you're right! it was fixed in the first commit, but not in the second commit. it's fixed now.

test/functional/interface_zmq.py

theStack

Nice, Concept ACK, will review this one soon!

Some quick findings:
Note that there's a typo in the PR title (s/zmg/zmq/), also I'm not sure if "refactor" is really applying here. For the second commit, adding a review suggestion for ignoring whitespace (not tested, but the very popular --color-moved=dimmed-zebra should work) would be probably very helpful for reviewers.

make interfaces_zmg run deterministically. this test is for the zmg notifications, so it doesn't need the wallet compiled to run

josibake · 2022-03-24T10:14:15Z

Nice, Concept ACK, will review this one soon!

Some quick findings: Note that there's a typo in the PR title (s/zmg/zmq/), also I'm not sure if "refactor" is really applying here. For the second commit, adding a review suggestion for ignoring whitespace (not tested, but the very popular --color-moved=dimmed-zebra should work) would be probably very helpful for reviewers.

thanks for the suggestions, @theStack ! I updated the title and also added a note for reviewers. I found that git diff -w master worked best for me

DrahtBot · 2022-03-24T15:48:52Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

vincenzopalazzo

ACK bc90b8d

maflcko

Left some thoughts

maflcko · 2022-03-24T19:35:28Z

test/functional/interface_zmq.py

        # Some transactions have been happening but we aren't consuming zmq notifications yet
        # or we lost a ZMQ message somehow and want to start over
-        txids = []
+        txs = []


Unrelated: It looks like this array is unused beside the last value? Could remove it and just assign the last value to a non-array name?

agreed, was very confused by this array. ended up leaving it alone just to minimize my changes to just MiniWallet

Yes, it is good to keep unrelated changes for separate commits and/or follow-ups.

maflcko · 2022-03-24T19:36:56Z

test/functional/interface_zmq.py

-        self.nodes[0].bumpfee(txids[-1])
+            txs.append(self.wallet.send_self_transfer(from_node=self.nodes[0]))
+        txs[-1]['tx'].vout[0].nValue -= 1000
+        self.nodes[0].sendrawtransaction(txs[-1]['tx'].serialize().hex())


I recall that you asked on IRC whether it makes sense to introduce a bumpfee method for the wallet. On a second thought, I think it could make sense.

Currently it looks like send_self_transfer will put an utxo in the wallet which will never exist if the tx is bumped?

So a helper could drop the utxo(s?) and even add the new ones?

yep, I think it would be a useful method to have. I'll open a follow-up PR for adding it as a method to MiniWallet

…face_zmq` bc90b8d [move only] remove `is_wallet_compiled` checks (josibake) 0bfbf7f test: use MiniWallet in `interfaces_zmq` (josibake) Pull request description: While working on bitcoin#24584 , `interface_zmq` started failing due to coin selection not running deterministically. The test doesn't actually need the wallet, so this PR migrates it to use MiniWallet _Note for reviewers:_ the second commit moves large chunks of code out of an if block, so it may be helpful to review with something that ignores whitespace, e.g `git diff -w master` ACKs for top commit: vincenzopalazzo: ACK bitcoin@bc90b8d Tree-SHA512: c618e23d00635d72dafdef28e68cbc88b9cc2030d4898fc5b7eac926fd621684c1958c075ed167192716b18308da5a0c1f1393396e31b99d0d3bde78b78fefc5

fanquake added the Tests label Mar 23, 2022

josibake force-pushed the josibake-make-interfaces-zmq-test-use-miniwallet branch from ead059e to a156633 Compare March 23, 2022 16:11

maflcko reviewed Mar 23, 2022

View reviewed changes

josibake force-pushed the josibake-make-interfaces-zmq-test-use-miniwallet branch 3 times, most recently from e27ec9a to 44481fb Compare March 23, 2022 17:45

theStack reviewed Mar 23, 2022

View reviewed changes

josibake changed the title ~~test, refactor: use MiniWallet in test/functional/interface_zmg~~ test: use MiniWallet in test/functional/interface_zmq Mar 24, 2022

josibake force-pushed the josibake-make-interfaces-zmq-test-use-miniwallet branch from 44481fb to ae40931 Compare March 24, 2022 09:48

josibake added 2 commits March 24, 2022 10:57

test: use MiniWallet in interfaces_zmq

0bfbf7f

make interfaces_zmg run deterministically. this test is for the zmg notifications, so it doesn't need the wallet compiled to run

[move only] remove is_wallet_compiled checks

bc90b8d

maflcko approved these changes Mar 24, 2022

View reviewed changes

josibake force-pushed the josibake-make-interfaces-zmq-test-use-miniwallet branch from ae40931 to bc90b8d Compare March 24, 2022 10:05

vincenzopalazzo approved these changes Mar 24, 2022

View reviewed changes

maflcko merged commit a697a3f into bitcoin:master Mar 24, 2022

maflcko reviewed Mar 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Mar 25, 2022

tests: Use test framework utils where possible #23127

Closed

bitcoin locked and limited conversation to collaborators Mar 25, 2023

josibake deleted the josibake-make-interfaces-zmq-test-use-miniwallet branch January 26, 2024 10:51

Conversation

josibake commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

josibake commented Mar 24, 2022

Uh oh!

DrahtBot commented Mar 24, 2022

Conflicts

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

josibake commented Mar 23, 2022 •

edited

Loading