test: use MiniWallet in test/functional/interface_zmq
#24653
Conversation
ead059e to
a156633
Compare
test/functional/interface_zmq.py
Outdated
| assert_equal(self.nodes[0].submitblock(block.serialize().hex()), None) | ||
| tip = self.nodes[0].getbestblockhash() | ||
| assert_equal(int(tip, 16), block.sha256) | ||
| orig_tx_2 = self.wallet.send_self_transfer(from_node=self.nodes[0]) |
There was a problem hiding this comment.
I think you can just assign to orig_txid_2 to minimize the diff (also elsewhere)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
okay, in 7f943aa ive got the diff down
There was a problem hiding this comment.
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']
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sorry, you're right! it was fixed in the first commit, but not in the second commit. it's fixed now.
e27ec9a to
44481fb
Compare
There was a problem hiding this comment.
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.
MiniWallet in test/functional/interface_zmgMiniWallet in test/functional/interface_zmq
44481fb to
ae40931
Compare
make interfaces_zmg run deterministically. this test is for the zmg notifications, so it doesn't need the wallet compiled to run
ae40931 to
bc90b8d
Compare
thanks for the suggestions, @theStack ! I updated the title and also added a note for reviewers. I found that |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| @@ -455,10 +452,10 @@ def test_mempool_sync(self): | |||
|
|
|||
| # 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 = [] | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
agreed, was very confused by this array. ended up leaving it alone just to minimize my changes to just MiniWallet
There was a problem hiding this comment.
Yes, it is good to keep unrelated changes for separate commits and/or follow-ups.
| 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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
While working on #24584 ,
interface_zmqstarted failing due to coin selection not running deterministically. The test doesn't actually need the wallet, so this PR migrates it to use MiniWalletNote 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