test: use MiniWallet for mempool_accept.py #24035

pull theStack wants to merge 4 commits into bitcoin:master from theStack:202201-test-use_MiniWallet_for_mempool_accept_etc changing 16 files +88 −86
  1. theStack commented at 2:58 PM on January 11, 2022: member

    This PR enables one more of the non-wallet functional tests (mempool_accept.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078.

    It also includes some other minor changes that came up while working on the replacement:

    • [commit 1/4] replace magic number 0xffffffff for a tx's nSequence with a new constant SEQUENCE_FINAL
    • [commit 2/4] create CTransaction instances with the current nVersion=2 by default, in order to use BIP68 for tests
    • [commit 3/4] support default from_node parameter for creating txs (this is a stripped down version of PR #24025)
  2. test: refactor: add constant for sequence number `SEQUENCE_FINAL` 2f79786822
  3. test: create txs with current `nVersion` (2) by default
    This enables testing of BIP68 without the need of explicitly
setting nVersion to 2. This is e.g. useful for transactions
created with MiniWallet.
    f30041c914
  4. test: MiniWallet: support default `from_node` for creating txs
    If no `from_node` parameter is passed explicitely to the
`create_self_transfer` method, the test node passed in the course
of creating the MiniWallet instance is used.  This seems to
be the main use-case in most of the current functional
tests, i.e. in many instances the calls can be shortened.
    b24f6c6855
  5. theStack force-pushed on Jan 11, 2022
  6. in test/functional/mempool_accept.py:89 in ebc3d51bf5 outdated 
      85 | @@ -86,27 +86,24 @@ def run_test(self):
  86 |  
  87 |          self.log.info('A transaction not in the mempool')
  88 |          fee = Decimal('0.000007')
  89 | -        raw_tx_0 = node.signrawtransactionwithwallet(node.createrawtransaction(
  90 | -            inputs=[{"txid": txid_in_block, "vout": 0, "sequence": BIP125_SEQUENCE_NUMBER}],  # RBF is used later
  91 | -            outputs=[{node.getnewaddress(): Decimal('0.3') - fee}],
  92 | -        ))['hex']
  93 | -        tx = tx_from_hex(raw_tx_0)
  94 | +        self.wallet.scan_tx(node.decoderawtransaction(raw_tx_in_block))
    MarcoFalke commented at 4:35 PM on January 11, 2022:

    You can use self.wallet.sendrawtransaction(node, tx), no?

    theStack commented at 12:13 PM on January 12, 2022:

    This didn't work due too high fee caused by the missing -maxfeerate=0 parameter. Fixed that by changing the amounts now, i.e. we can use wallet.sendrawtransaction(...) and don't to call wallet.scan_tx(...) manually anymore.

    MarcoFalke commented at 12:23 PM on January 12, 2022:

    It might be cleaner to keep the amounts and change sendraw to accept and pass on **kwargs (like def generate).

    theStack commented at 3:54 PM on January 12, 2022:

    Nice idea, done!

  7. in test/functional/mempool_accept.py:189 in ebc3d51bf5 outdated 
     191 | +        tx = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_spend, sequence=SEQUENCE_FINAL)['tx']
 192 | +        tx.vout[0].nValue = int(0.05 * COIN)
 193 | +        raw_tx_reference = tx.serialize().hex()
 194 |          # Reference tx should be valid on itself
 195 |          self.check_mempool_result(
 196 |              result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.1') - Decimal('0.05')}}],
    MarcoFalke commented at 4:46 PM on January 11, 2022:

    You can remove signrawtransactionwithwallet a few lines down?

    theStack commented at 12:14 PM on January 12, 2022:

    Thanks, done. Also removed a bunch of other comments about skipping (re)-signing, since we don't sign anything here in this test in the first place.

  8. in test/functional/mempool_accept.py:183 in ebc3d51bf5 outdated 
     184 | -            inputs=[{'txid': txid_spend_both, 'vout': 0}],
 185 | -            outputs=[{node.getnewaddress(): 0.05}],
 186 | -        ))['hex']
 187 | -        tx = tx_from_hex(raw_tx_reference)
 188 | +        self.log.info('Create a "reference" tx for later use')
 189 | +        self.wallet.scan_tx(node.decoderawtransaction(raw_tx_spend_both))
    MarcoFalke commented at 4:47 PM on January 11, 2022:

    Same

  9. in test/functional/mempool_accept.py:184 in ebc3d51bf5 outdated 
     186 | -        ))['hex']
 187 | -        tx = tx_from_hex(raw_tx_reference)
 188 | +        self.log.info('Create a "reference" tx for later use')
 189 | +        self.wallet.scan_tx(node.decoderawtransaction(raw_tx_spend_both))
 190 | +        utxo_to_spend = self.wallet.get_utxo(txid=txid_spend_both)
 191 | +        tx = self.wallet.create_self_transfer(utxo_to_spend=utxo_to_spend, sequence=SEQUENCE_FINAL)['tx']
    MarcoFalke commented at 4:49 PM on January 11, 2022:

    Why set FINAL when it wasn't set previously?

    theStack commented at 12:19 PM on January 12, 2022:

    Per default createrawtransaction sets the input's nSequence fields to 0xffffffff i.e. it actually was already final before. This is needed because of the following line below:

    https://github.com/bitcoin/bitcoin/blob/318c79e8096ce477561ef099835954b95a687685/test/functional/mempool_accept.py#L326

    Probably it would make sense in a follow-up to also create CTxIn instances with nSequence=SEQUENCE_FINAL by default in the test framework, to match the behaviour in the bitcoind codebase?

    https://github.com/bitcoin/bitcoin/blob/318c79e8096ce477561ef099835954b95a687685/src/primitives/transaction.h#L105-L106

    MarcoFalke commented at 12:29 PM on January 12, 2022:

    ok, fair enough

  10. DrahtBot added the label Tests on Jan 11, 2022
  11. MarcoFalke approved
  12. MarcoFalke commented at 4:49 PM on January 11, 2022: member

    ACK ebc3d51bf50fa88042bb2620c37206f0528a70b0 🕉

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK ebc3d51bf50fa88042bb2620c37206f0528a70b0 🕉
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiyQgv/RaF3J1PaDgEOYs6oJ32aX+c/14xaYW5lzUWPzXT7qa/UzlZI1ZgiHpPM
/2aQ0cbjYwRHdDBNS0oE/V/N3DI28oxxOHzTVaCbQwqVeVcMLDsZhb43831NQcz1
otfqSomZZJineWtjZRaKcOB8b20qwy0uGvyVvKgvNmeFDMaZC8K1JKNnar4+UtAU
oJQoZBPbWzY5GHsj3v9B7HpJN+K3iIYHlbaBs+x9jOlLZHnJT896cKBycw4JrDXL
bw45hBwK4EdnK4+KJZWzC3l9OeZEZ4aubNFtX2MlANp5l+CUsaZVgnwi4UnJebUn
/fTef1W+Vg6B7pOvqa3iiMaIN1wBVEs3BLG+PjBvnVTDz9CMQryzHpZB13gk42O7
BO1gsOrB346P6zpGR5smobMWabyqZhbDH2I7/8qbCdQxKQ3VSUGlGh7r3y2BoW+j
3SrVsPFifDRMjqHZy697eFxFpjj/6SDwUrLEYfHrdBz1U8P/xReuJFh9eyjpHV5w
XnSV1rOS
=ULDi
-----END PGP SIGNATURE-----

    </details>

  13. theStack force-pushed on Jan 12, 2022
  14. theStack commented at 12:24 PM on January 12, 2022: member

    Thanks for the review @MarcoFalke, tried to address all of your comments (getting rid of wallet.scan_tx() calls, removing comments about skipping (re-)signing that don't apply anymore).

  15. MarcoFalke commented at 12:27 PM on January 12, 2022: member

    re-ACK ebd3b552686a3333e06cb2e151ef5c99af327c03 🐘

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK ebd3b552686a3333e06cb2e151ef5c99af327c03  🐘
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj1/Qv9H86e10i7hwNLo1kyCzTSTysj1Zv24M5ZpSF+L+A62IoPFsn0aDDM0DHY
ksIUKe2s32PHhM+/CPS4LYr1q+B3NuEEir1MTrnucfdEwpEyb05cWHGxFANr275o
Ztj2Nxvb4Qgdt6YS4lkxfmP0aKG8PTKzAPaIUbpjIbtfxE/i2piMIu0W5a8MxA7D
BKSC04U2L0te8k+aFBhKKcZUHBoj+ETk4te66xOrO30WTPyq5AmsuFufyplnAV7S
rd94vZ4her2/8WtRdkAAhyPEdfrS9n909PwPQkC8Twh0zJ2PxBhW9sByWPgDHpWC
ZqCU+ziyZXl1KENKaddQCaQKlJ9udCqVGDFGhxlZrjbS1EtqHAy2AgJKSdYOUiiO
Grk2Bd0FiRkuq6akZt3qyIEpq9RChFdhzUy9yhybZfF3RakPKtzv6g2kNLz4OhE8
7v8Jf3JQTbYBZ+Z6AlzxGSnd8zlb6yfOjHaSmg7zSed/djfsXtUGeYo+5gttWF12
n9YAaOLW
=gJ7h
-----END PGP SIGNATURE-----

    </details>

  16. in test/functional/mempool_accept.py:105 in ebd3b55268 outdated 
     105 | -        coin = coins.pop()  # Pick a random coin(base) to spend
 106 |          output_amount = Decimal('0.025')
 107 | -        raw_tx_final = node.signrawtransactionwithwallet(node.createrawtransaction(
 108 | -            inputs=[{'txid': coin['txid'], 'vout': coin['vout'], "sequence": 0xffffffff}],  # SEQUENCE_FINAL
 109 | -            outputs=[{node.getnewaddress(): output_amount}],
 110 | -            locktime=node.getblockcount() + 2000,  # Can be anything
    MarcoFalke commented at 1:04 PM on January 12, 2022:

    nit: any reason to remove this comment?

    theStack commented at 3:54 PM on January 12, 2022:

    No reason, this was not intended. Put it back in the latest force-push.

    MarcoFalke commented at 5:51 PM on January 12, 2022:

    nit: any reason to remove the trailing comma? :sweat_smile:

    theStack commented at 5:35 PM on January 13, 2022:

    D'oh, for some reason I thought that's only allowed for import lists and not for function calls :smirk: Done!

  17. theStack force-pushed on Jan 12, 2022
  18. theStack commented at 3:57 PM on January 12, 2022: member

    Force-pushed again with two suggestions tackled (https://github.com/bitcoin/bitcoin/pull/24035#discussion_r783026046 and #24035 (review)). Thanks for your patience!

  19. MarcoFalke approved
  20. MarcoFalke commented at 5:53 PM on January 12, 2022: member

    re-ACK 0dd28a752f15df117c810123810729703c7c575c 🍢

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 0dd28a752f15df117c810123810729703c7c575c 🍢
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUirgAv/Zcb5audGGnebIIxRH85LrU+UjCbmnBPH4iENA6AFzCUPRGkbigiGM1dq
5JqciQbgKy7ooOooat0gGntexJDQ7A42HVcCoXkEo9GtGUC4VjF8o0rF+J3CoSzQ
9IIeggEEgUWsC0Fuj4jtf45AA1YWxr2RlF1ztTCuIWNsa/AdDhSr+raGmwCsj8uR
vnrzNuCU16gBghLTS/ClU030mX2mjr4AcNkbEy1UIN/khyxsLN/XKEKrfMCpuWdB
mC/FDbbAFJcLu8ERe+V0PYjxsOqWC2lvM712iE5RwuBPaMAAms4rzc8AL28zUFQ0
AUokMsm71IbOHzGRFXV8sNLvcKACj23l8HS+0/6QdX7VThefClUS27T2ATi5NDgO
jbCnHT7Ea/zdzkNlOyOmdUmrJ8QiatwgsJ50ZIDqStMYjXP5mgABDASuLyZeCA7S
vGUjYZaTPY3giAgx3NV9TX8EOJVWflDYEXpx5y6+4Bi+5u7fCA7uuAYqQM8fER0Q
OzDTk3jT
=S1cB
-----END PGP SIGNATURE-----

    </details>

  21. test: use MiniWallet for mempool_accept.py
    This test can now be run even with the Bitcoin Core wallet disabled.
    aa8a65e4a8
  22. theStack force-pushed on Jan 13, 2022
  23. MarcoFalke commented at 5:39 PM on January 13, 2022: member

    re-ACK aa8a65e4a88bfbd83ac756a87bfb8faf52fb675d 📊

    <details><summary>Show signature</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK aa8a65e4a88bfbd83ac756a87bfb8faf52fb675d 📊
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi3mgv/fsG1sHFrfTMhZoVxBbD6+vaKEU/xFzPRQZWnF1k+GN2EzwX5NdzpgTqu
+OPUKrlj61y4bHAJVtp2EccDdqMCvSgsBwKB5Td1tGFLTk81LZYFPUb1cvaO9y0N
lORWGlIiyePaUOOBTA1ahiHAiQvr7UfuU64kmfjSCOgtpQ+LgSoJgwoUMi6KY47a
I998bcENP130pfAX8LkUF2vNz86Vp7S/ej1rPSU8OHhImkjy8Ie0lY1pi+ZcHZ73
1yufp27fvleFRRd5WbcFQz/m0M53VM/VP+ZpeqH2Ka5cSHyZV7niLnXOMf/BLtWv
5MQP/08ZRx3Bt0xITD0bIolbf68be7mQKvBb8LrTCsT5isV7koTSPafQJ5f9lsae
ZfgiJkjv7DoHUzvia37r1AESOCVkmFcP5LBp9HTesaqa7pwhoZi5oXzEscmISyyl
A+RZIYazGUk4ZL8iK7jiM0uaZETvxeYWkYVXbSuz0GFahPncvbhQOwV2+5Zgmn23
UTiuj0qJ
=ZFWj
-----END PGP SIGNATURE-----

    </details>

  24. MarcoFalke merged this on Jan 13, 2022
  25. MarcoFalke closed this on Jan 13, 2022
  26. theStack deleted the branch on Jan 13, 2022
  27. sidhujag referenced this in commit ee6e8d51f7 on Jan 14, 2022
  28. DrahtBot locked this on Jan 13, 2023
Contributors
theStackMarcoFalke
Labels
Tests
Linked (view graph)
#20078 test: Convert non-wallet tests to use our python MiniWallet#24025 test: MiniWallet: support default `from_node` for sending/creating txs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-28 03:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me