test: use MiniWallet for simple doublespend sub-test in feature_rbf.py #22330

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202106-test-feature_rbf_use_miniwallet_for_doublespend changing 1 files +19 −30
  1. theStack commented at 3:58 PM on June 23, 2021: member

    This PR's goal is to prepare the functional test feature_rbf.py for more MiniWallet usage. It first gets rid of unused initialization code (I guess that was needed at times when the nodes were still in IBD at the start of tests?), then makes the MiniWallet instance introduced in #22210 available for all sub-tests, and finally, uses that instance in the first sub-test test_simple_doublespend.

    Note that the same idea of replacing the make_utxo calls with MiniWallet can be also applied to other sub-tests too; this just serves as a first proof-of-concept.

  2. DrahtBot added the label Tests on Jun 23, 2021
  3. brunoerg commented at 1:07 PM on June 26, 2021: member

    Concept ACK

  4. mjdietzx commented at 9:54 PM on July 14, 2021: contributor

    ACK c582546366496f25b452a516d7890389af3e0729

  5. MarcoFalke commented at 6:19 PM on July 15, 2021: member

    Needs rebase

  6. test: remove unneeded initialization code in feature_rbf.py 84c874794c
  7. theStack force-pushed on Jul 15, 2021
  8. theStack commented at 8:21 PM on July 15, 2021: member

    Needs rebase

    Done. Took the new sub-test test_replacement_relay_fee() into account for the second commit.

  9. in test/functional/feature_rbf.py:89 in 9f2a53d0f4 outdated 
      84 | @@ -84,10 +85,8 @@ def skip_test_if_missing_module(self):
  85 |          self.skip_if_no_wallet()
  86 |  
  87 |      def run_test(self):
  88 | -        make_utxo(self.nodes[0], 1 * COIN)
  89 | -
  90 | -        # Ensure nodes are synced
  91 | -        self.sync_all()
  92 | +        self.wallet = MiniWallet(self.nodes[0])
  93 | +        self.wallet.scan_blocks(start=76, num=3)
    ShubhamPalriwala commented at 5:36 PM on July 21, 2021:

    Previously, We were scanning block 76 for test_no_inherited_signaling(self) and block 77 for test_replacement_relay_fee(self)

    However in the PR, We are scanning blocks 76,77, and 78.

    Block 78 is not required to be scanned here and seems unnecesary, hence, we can rather have this as self.wallet.scan_blocks(start=76, num=2)

    theStack commented at 4:11 PM on July 24, 2021:

    Thanks, done.

  10. ShubhamPalriwala changes_requested
  11. sriramdvt approved
  12. sriramdvt commented at 5:52 PM on July 23, 2021: contributor

    tACK 9f2a53d0f450ea83b24b1af209591db7fc5b31b9. nit: I agree with https://github.com/bitcoin/bitcoin/pull/22330/files#r674195638 that it is not necessary to scan 3 blocks

  13. theStack force-pushed on Jul 24, 2021
  14. theStack commented at 4:16 PM on July 24, 2021: member

    @ShubhamPalriwala @sriramdvt: Thanks for your reviews! You are right, in total we only need two blocks to scan -- in the second commit one UTXO in MiniWallet to start with is enough (both sub-tests test_no_inherited_signaling and test_replacement_relay_fee consume one UTXO, but also create one, due to calls to send_self_transfer), in the latest commit, we have to increase it to two, however (the sub-test test_simple_doublespend only consumes a UTXO, but doesn't create a new one).

  15. practicalswift commented at 8:17 PM on July 24, 2021: contributor

    Concept ACK

  16. in test/functional/feature_rbf.py:141 in 48e5b1f66f outdated 
     144 | +        tx1a = deepcopy(tx_template)
 145 |          tx1a.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT)]
 146 |          tx1a_hex = tx1a.serialize().hex()
 147 |          tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex, 0)
 148 |  
 149 |          self.sync_all()
    josibake commented at 12:03 PM on July 26, 2021:

    I'm not not sure why this call to self.sync_all() is here. Looks like you removed it on line 137 and it's also not used after calling sendrawtransaction on line 155. Since the miniwallet is attached to node 0 and its the only once we are referencing in the test, it seems unnecessary.

    I deleted line 141 while testing, ran the test a few times and every run passed. I'd suggest removing it.

    theStack commented at 9:21 PM on July 27, 2021:

    Thanks, excellent find! Removed the sync_all() call in the latest force-push.

  17. in test/functional/feature_rbf.py:92 in 48e5b1f66f outdated 
      84 | @@ -84,10 +85,8 @@ def skip_test_if_missing_module(self):
  85 |          self.skip_if_no_wallet()
  86 |  
  87 |      def run_test(self):
  88 | -        make_utxo(self.nodes[0], 1 * COIN)
  89 | -
  90 | -        # Ensure nodes are synced
  91 | -        self.sync_all()
  92 | +        self.wallet = MiniWallet(self.nodes[0])
  93 | +        self.wallet.scan_blocks(start=76, num=2)
    josibake commented at 12:05 PM on July 26, 2021:

    Initially, I was confused by these parameters and noted that the test would fail if I picked a start less than 70, but would run for starts greater than 76. I dug through the comments and found the explanation on a previous PR, but considering this test is meant to be a proof of concept to motivate re-writing all the tests to use miniwallet, it might be worth adding a small comment explaining why 76 and 2 are chosen.

    theStack commented at 9:23 PM on July 27, 2021:

    Good point, extended the second commit with an explanation about why we start at block 76, with reference to the test framework's method where the chain is pre-mined.

  18. josibake approved
  19. josibake commented at 12:06 PM on July 26, 2021: member

    tACK 48e5b1f

    I ran the tests of master and then off the PR to confirm everything still passes as expected. Left some optional feedback in the comments.

  20. hg333 approved
  22. ShubhamPalriwala commented at 8:05 PM on July 27, 2021: contributor

    tested and works fine on Ubuntu 21.04 https://github.com/bitcoin/bitcoin/commit/48e5b1f66f567b21ab6ce3ec1b58838ca04279ca

    However, I agree with @josibake here:

    Initially, I was confused by these parameters and noted that the test would fail if I picked a start less than 70, but would run for starts greater than 76. I dug through the comments and found the explanation on a previous PR, but considering this test is meant to be a proof of concept to motivate re-writing all the tests to use miniwallet, it might be worth adding a small comment explaining why 76 and 2 are chosen.

    Adding a comment explaining the 2 blocks would help to understand the code even further

  23. test: feature_rbf.py: make MiniWallet instance available for all sub-tests
    also document on why we start scanning blocks at height 76
    a3f6397c73
  24. theStack force-pushed on Jul 27, 2021
  25. theStack commented at 9:27 PM on July 27, 2021: member

    Force-pushed with changes suggested by josibake (https://github.com/bitcoin/bitcoin/pull/22330#discussion_r676542687, #22330 (review)). Thanks for the valuable reviews so far :)

  26. DrahtBot commented at 12:18 PM on July 28, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  27. josibake commented at 2:49 PM on July 28, 2021: member

    ACK https://github.com/bitcoin/bitcoin/pull/22330/commits/c1c0768b619ab463a00052d0a584fba191eb02a9

    thanks for adding the comment! it will make it much easier for others to followup and refactor the remaining tests to use miniwallet

  28. in test/functional/feature_rbf.py:137 in c1c0768b61 outdated 
     138 | -        # transactions might not be accepted by our peers.
 139 | -        self.sync_all()
 140 | +        # we use MiniWallet to create a transaction template with inputs correctly set,
 141 | +        # and modify the output (amount, scriptPubKey) according to our needs
 142 | +        utxo = self.wallet.get_utxo()
 143 | +        tx_template = self.wallet.create_self_transfer(from_node=self.nodes[0], utxo_to_spend=utxo)['tx']
    MarcoFalke commented at 3:01 PM on July 28, 2021: 
            tx_template = self.wallet.create_self_transfer(from_node=self.nodes[0])['tx']
    theStack commented at 4:17 PM on July 28, 2021:

    Thanks, done.

  29. in test/functional/feature_rbf.py:153 in c1c0768b61 outdated 
     162 |          assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx1b_hex, 0)
 163 |  
 164 |          # Extra 0.1 BTC fee
 165 | -        tx1b = CTransaction()
 166 | -        tx1b.vin = [CTxIn(tx0_outpoint, nSequence=0)]
 167 |          tx1b.vout = [CTxOut(int(0.9 * COIN), DUMMY_P2WPKH_SCRIPT)]
    MarcoFalke commented at 3:02 PM on July 28, 2021: 
            tx1b.vout[0].nValue -= int(0.1 * COIN)
    theStack commented at 4:17 PM on July 28, 2021:

    Way more readable, thanks. Done.

  30. MarcoFalke commented at 3:03 PM on July 28, 2021: member

    ACK c1c0768b619ab463a00052d0a584fba191eb02a9 🌴

    <details><summary>Show signature and timestamp</summary>

    Signature:

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

ACK c1c0768b619ab463a00052d0a584fba191eb02a9 🌴
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi2Fwv/ZxETYQ3y9g77WwtNwVdqsHBrnHUaGAdjmEL1sF9HoCIwN1Ag92X0cVZV
0thS0WL81c/c7jWDG7H4XGAMfA254XvIWsA8vOjrx48YmbFxUZU0BNbtonT7iY04
arLwF4NsAg++ZmZni+Ofqyt90moqnlNLdtUvU8IQjm3fqZ2mu+SMM4WVxuD9rP8J
cxoAq7R5B+QuLKpL+S6fvbZHHrtn0aAIv8C58kDMJDvQasrsI1Pn9gzmPmu0Kg5T
/zbLextjgQcQwMU3IBOCYkiENU1akjxJ9OMF5F6rsdILJ52hbqpAHJP3QxmgUXUB
83smxjrBzaLf3sBBvSbiUplfDpGdjhPIoGcs2rV1wPPJAHtaLmTk4LHMwsTI46S8
ulDmutPrXKOdzXTTg0MhvRksve+5sXOq+4HnEke4qbGp0ij2qabYwFJ0rOdZ7gM7
Vpbkufy2U4fSrOO+amRCR0/JTznFIOyymij/UWhfC0G9p/XRSGvKVFIr0XA4qoJS
IvWqeNPN
=LPRU
-----END PGP SIGNATURE-----

    Timestamp of file with hash 86af1901202bc55a0ba833a2df907819403554b0cc025a2c2c40822d8fff1ade -

    </details>

  31. test: use MiniWallet for simple doublespend test in feature_rbf.py aa02c64540
  32. theStack force-pushed on Jul 28, 2021
  33. theStack commented at 4:18 PM on July 28, 2021: member

    Force-pushed with simplification suggestions by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/22330#discussion_r678388776, #22330 (review)) in the last commit.

  34. MarcoFalke commented at 11:55 AM on July 30, 2021: member

    re-ACK aa02c64540cd77199c2834549786b9bc91fd4bc9 🌯

    <details><summary>Show signature and timestamp</summary>

    Signature:

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

re-ACK aa02c64540cd77199c2834549786b9bc91fd4bc9 🌯
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjpsAwAqmy0sSSxypNFh60782o9jMhgNWj8IOLk5Ba1qVoM4aYgm1jCnfsomCIU
hIYihGM/NajSzN9tq0nzpHILs3Tb3TY/GTwMeuh8+Uab8R4oUCYC6X+S/SxG13Oz
w5IaM3XbewO6MAdSFJq/nu0poCW8IfO4/fqxuVAZzDXAhgGlIldsu9+F9WPM03G1
y4K29E+Udff1dDowwtzu+EcndG0dXfpvctJyWXPhslOI2fLeyLK/6Ya6v0Sp3D5t
18444yWuSDmtRP/AjN0z1zOqzxN5D8m8JQP9+fuaG+/BvO4+F8v/abLGVaiZyV5h
4bt8x22GejTonIr/XmEM0icBLxrGAFOzdLVb50g35zV8yijtrbHKnyKn0oLJv6l3
FyoqUCLm4tczdXBUyFjzNGRpB3SlDGogn1ocJXNXrsehSSCqWY1cBE8ecfes9TsX
NJvmdyzQFjvoaRVuugWTHn0Gr2zizUMPrJHtVLx4RGgGvTzTnMwIzmuyNoFEtwBb
wzDGQOTL
=58KE
-----END PGP SIGNATURE-----

    Timestamp of file with hash 2623a0532a45417a171e97e69a61a4be006bec6bb4efa264a467e8da9aa7a356 -

    </details>

  35. MarcoFalke merged this on Jul 30, 2021
  36. MarcoFalke closed this on Jul 30, 2021
  37. theStack deleted the branch on Jul 31, 2021
  38. sidhujag referenced this in commit 4bc44154b3 on Aug 1, 2021
  39. DrahtBot locked this on Aug 16, 2022
Contributors
theStackbrunoergmjdietzxMarcoFalkeShubhamPalriwalasriramdvtpracticalswiftjosibakehg333DrahtBot
Labels
Tests
Linked (view graph)
#22210 test: Use MiniWallet in test_no_inherited_signaling RBF test
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-14 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me