test: Run feature_bip68_sequence.py with MiniWallet #26657
Conversation
df1c56d to
1852787
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the revie 8000 w process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
1852787 to
192a00c
Compare
192a00c to
9129265
Compare
|
Force-pushed with taking suggestion #26657 (comment) and remove formatting fixes. |
|
interface_usdt_utxocache.py is failing due to test: Ignore spent utxos in MiniWallet rescan_utxos, I'm working on it. It seems Or, to ensure that the mempool is cleared, could I replace diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py
index 379d04a1f..6030e7138 100755
--- a/test/functional/feature_bip68_sequence.py
+++ b/test/functional/feature_bip68_sequence.py
@@ -212,7 +212,7 @@ class BIP68Test(BitcoinTestFramework):
else:
# This raw transaction should be accepted
self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=rawtx)
- self.wallet.rescan_utxos()
+ self.generate(self.wallet, 1)
utxos = self.wallet.get_utxos(include_immature_coinbase=False)
# Test that sequence locks on unconfirmed inputs must have nSequence |
| self.nodes[0].sendrawtransaction(rawtx) | ||
| utxos = self.nodes[0].listunspent() | ||
| self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=rawtx) | ||
| self.wallet.rescan_utxos() |
There was a problem hiding this comment.
Why is this needed? wallet.sendrawtransaction should already account and scan the tx
There was a problem hiding this comment.
utxos = self.wallet.get_utxos() implies mark_as_spent=True, which means self._utxos is cleared.
wallet.sendrawtransaction only appends the tx to self._utxos, so we need self.wallet.rescan_utxos() to rescan unspent utxos.
If I remove utxos = self.wallet.get_utxos(), the test throw error.
2022-12-11T05:34:00.235000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/bitcoin/test/functional/test_framework/test_framework.py", line 134, in main
self.run_test()
File "/bitcoin/./test/functional/feature_bip68_sequence.py", line 73, in run_test
self.test_sequence_lock_confirmed_inputs()
File "/bitcoin/./test/functional/feature_bip68_sequence.py", line 202, in test_sequence_lock_confirmed_inputs
tx.vin.append(CTxIn(COutPoint(int(utxos[j]["txid"], 16), utxos[j]["vout"]), nSequence=sequence_value))
IndexError: list index out of range
There was a problem hiding this comment.
Ah, ok. So alternatively the wallet.sendrawtransaction could be dropped/reverted to just sendrawtransaction, because it is not needed to scan the tx.
9129265 to
8d2a0f5
Compare
|
Force-pushed with squash commits and replace test: Ignore spent utxos in MiniWallet rescan_utxos with test: Add "include mempool" flag to MiniWallet rescan_utxos diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
index 46b628ee3..52bfb0a2e 100644
--- a/test/functional/test_framework/wallet.py
+++ b/test/functional/test_framework/wallet.py
@@ -118,14 +119,13 @@ class MiniWallet:
def get_balance(self):
return sum(u['value'] for u in self._utxos)
- def rescan_utxos(self):
+ def rescan_utxos(self, *, include_mempool=True):
"""Drop all utxos and rescan the utxo set"""
self._utxos = []
res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
assert_equal(True, res['success'])
for utxo in res['unspents']:
- # Ignore utxo inside mempool
- if not self._test_node.gettxout(txid=utxo["txid"], n=utxo["vout"]):
+ if not include_mempool and not self._test_node.gettxout(txid=utxo["txid"], n=utxo["vout"]):
continue
self._utxos.append(
self._create_utxo(txid=utxo["txid"], |
8d2a0f5 to
69f0f6b
Compare
|
Force-pushed with taking suggestion #26657 (comment) |
69f0f6b to
cdda956
Compare
|
Force-pushed with taking suggestions #26657 (comment) and #26657 (comment) |
cdda956 to
572a445
Compare
|
Force-pushed with taking suggestions #26657 (comment) and #26657 (comment) |
572a445 to
2512e1a
Compare
|
Force-pushed with taking suggestions #26657 (comment) and #26657 (comment) and #26657 (comment) |
|
Force-pushed with taking suggestion discussion_r1062820833 |
2512e1a to
4159ccd
Compare
|
ACK 4159ccd |
There was a problem hiding this comment.
Very nice. Thanks!
review ACK 4159ccd 🤸
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 4159ccd03142899028019a7cb44ee4120e68505a 🤸
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUisvgv/YsGnHt+lU5rGQdwFN6eIUrKwK/0pjUAIaDIeOp/61fuwDrJc3MhNE+/j
3cAiUyvHTtwo3eWkvV9FdQ9V+gVNOoiBWG1yEJtZNoM/syt5uaMi4zUvs2CisODn
tlr0jDcMSBw0E0Dc/YEKkwWytysPER5kPQiCleuzUbWqYD/QVX3xUOXGf+a0rttX
DpuzTcUu5ojaZh4YMEBkgprStIADIdxE2WPb/0eXd1pmwD+TpvnjVA/uRICQAft5
pBcePzrFLuGqJCOdsg5Fw72JmXJh1RjuBnCiOGiFL/xN02QH4R5MY401RNaXZtYO
B8USAMS8mO8lJB45t7njTpp1KSgrOJN0RJyEbja6BTROzyi6JttFJVRx4vie2hea
jICFDu8hNAWrkvXDQXWf/KaUSLOaWITDtYLRdK4bfirNy1PxiFsUjBPkrn0KJgaG
xI2AaYpNQqV7Krprdm9sEJpO8lev89aL+TEwZv7i2DjZONJArSOEE/Rtmm0nbQEI
2t90dylY
=isq1
-----END PGP SIGNATURE-----
| self.nodes[0].sendrawtransaction(rawtx) | ||
| utxos = self.nodes[0].listunspent() | ||
| self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=rawtx) | ||
| self.wallet.rescan_utxos() |
There was a problem hiding this comment.
Ah, ok. So alternatively the wallet.sendrawtransaction could be dropped/reverted to just sendrawtransaction, because it is not needed to scan the tx.
| tx = tx_from_hex(rawtxfund) | ||
| mini_wallet = MiniWallet(self.nodes[1]) | ||
| mini_wallet.rescan_utxos() | ||
| tx = mini_wallet.create_self_transfer()["tx"] |
There was a problem hiding this comment.
nit in the last commit (can be fixed in a follow-up):
- MiniWallet shouldn't care about the node, so a single instance should suffice per test.
- Maybe the send_self_transfer function can accept a
versionoption?
There was a problem hiding this comment.
May I create another PR to refactor those nits?
There was a problem hiding this comment.
Yes, if you agree with them and if the changes are possible (haven't checked myself)
This PR enables one more of the non-wallet functional tests (feature_bip68_sequence.py) to be run even when no wallet is compiled in by using the MiniWallet instead, as proposed in #20078.