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
489 | + add_witness_commitment(block) 490 | + block.solve() 491 | + assert_equal(self.nodes[0].submitblock(block.serialize().hex()), None) 492 | + tip = self.nodes[0].getbestblockhash() 493 | + assert_equal(int(tip, 16), block.sha256) 494 | + orig_tx_2 = self.wallet.send_self_transfer(from_node=self.nodes[0])
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 https://github.com/bitcoin/bitcoin/pull/24653/commits/7f943aa1e0543765f499b2f50697281d5fe22adf 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.
497 | + (hash_str, label, mempool_seq) = seq.receive_sequence() 498 | + while hash_str != orig_tx['txid']: 499 | +<<<<<<< HEAD 500 | +>>>>>>> ec13426c0d (self_transfer) 501 | +======= 502 | +>>>>>>> ead059e477 ([move only] remove `is_wallet_compiled` checks)
?
whoops! fixing
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
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-zebrashould 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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
451 | @@ -455,10 +452,10 @@ def test_mempool_sync(self): 452 | 453 | # Some transactions have been happening but we aren't consuming zmq notifications yet 454 | # or we lost a ZMQ message somehow and want to start over 455 | - txids = [] 456 | + 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.
480 | @@ -484,11 +481,12 @@ def test_mempool_sync(self): 481 | # Things continue to happen in the "interim" while waiting for snapshot results 482 | # We have node 0 do all these to avoid p2p races with RBF announcements 483 | for _ in range(num_txs): 484 | - txids.append(self.nodes[0].sendtoaddress(address=self.nodes[0].getnewaddress(), amount=0.1, replaceable=True)) 485 | - self.nodes[0].bumpfee(txids[-1]) 486 | + txs.append(self.wallet.send_self_transfer(from_node=self.nodes[0])) 487 | + txs[-1]['tx'].vout[0].nValue -= 1000 488 | + 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
Left some thoughts