test: speed up wallet_avoidreuse, add logging #17362

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:wallet_avoidreuse-test-improvements changing 1 files +9 −0
  1. jonatack commented at 9:19 AM on November 4, 2019: member

    Inspired by PRs #17340 and #15881.

    • add logging
    • pass -whitelist in set_test_params to speed up transaction relay

    wallet_avoidreuse.py is not intended to test P2P transaction relay/timing, so it should be fine to do this here. This reduces test run time variability and speeds up the test by 2-3 times on average.

    Test run times in seconds:

    • before: 20, 24, 22, 17, 27, 40, 30

    • after: 10, 10, 8, 9, 10, 7, 8

  2. jonatack commented at 9:28 AM on November 4, 2019: member

    Test output with logging:

    $ test/functional/wallet_avoidreuse.py 
2019-11-04T09:25:24.487000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_5geycz8_
2019-11-04T09:25:25.657000Z TestFramework (INFO): Test wallet files persist avoid_reuse flag
2019-11-04T09:25:26.715000Z TestFramework (INFO): Test immutable wallet flags
2019-11-04T09:25:27.637000Z TestFramework (INFO): Test fund send fund send dirty
2019-11-04T09:25:31.799000Z TestFramework (INFO): Test fund send fund send
2019-11-04T09:25:34.649000Z TestFramework (INFO): Stopping nodes
2019-11-04T09:25:34.955000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_5geycz8_ on exit
2019-11-04T09:25:34.955000Z TestFramework (INFO): Tests successful
  3. in test/functional/wallet_avoidreuse.py:72 in 5ea05c96d7 outdated 
      66 | @@ -67,6 +67,9 @@ class AvoidReuseTest(BitcoinTestFramework):
  67 |      def set_test_params(self):
  68 |          self.setup_clean_chain = False
  69 |          self.num_nodes = 2
  70 | +        # This test isn't testing txn relay/timing, so set whitelist on the
  71 | +        # peers for instant txn relay. This speeds up the test run time 2-3x.
  72 | +        self.extra_args = [['-whitelist=127.0.01']] * 2
    practicalswift commented at 9:33 AM on November 4, 2019:

    127.0.0.1? :)

    jonatack commented at 9:50 AM on November 4, 2019:

    Thanks @practicalswift, good eye :)

    jonatack commented at 9:58 AM on November 4, 2019:

    Fixed.

  4. jonatack force-pushed on Nov 4, 2019
  5. DrahtBot added the label Tests on Nov 4, 2019
  6. in test/functional/wallet_avoidreuse.py:93 in d2edf02d54 outdated 
      94 |      def test_persistence(self):
  95 | -        '''Test that wallet files persist the avoid_reuse flag.'''
  96 | -        # Configure node 1 to use avoid_reuse
  97 | -        self.nodes[1].setwalletflag('avoid_reuse')
  98 | +        """Test that wallet files persist the avoid_reuse flag."""
  99 | +        self.log.info("Test wallet files persist avoid_reuse flag")
    jachiang commented at 2:18 PM on November 4, 2019:

    s/Test wallet files/Test that wallet files

    jonatack commented at 6:18 PM on November 4, 2019:

    Thanks @jachiang. I removed both articles from the docstring ("that" / "the") for the log version to keep it shorter, as it was longer than the other logs, and for consistency because the other added logs don't have articles either.

  7. jachiang changes_requested
  8. jachiang commented at 2:31 PM on November 4, 2019: contributor

    Thanks @jonatack!

    tACK d2edf02d54ac3286f721ee7d637f5c5e886fbd34 Reviewed test, also don't see any need for tx propagation delay.

    My informal run times confirm improvement. Before 463eab5e : 31.9 / 22.9 / 35.7 / 29.8 / 34.8 / 29.7 / With patch d2edf02: 14.7 / 11.9 / 10.7/ 12.9 / 13.7 / 13.7 /12.4

    However, I am unsure of the source of variability. I don't see an improvement there.

  9. in test/functional/wallet_avoidreuse.py:2 in e439470796 outdated 
       0 | @@ -1,8 +1,8 @@
   1 |  #!/usr/bin/env python3
   2 | -# Copyright (c) 2018 The Bitcoin Core developers
   3 | +# Copyright (c) 2018-2019 The Bitcoin Core developers
    Sjors commented at 5:35 PM on November 4, 2019:

    Generally don't touch this: maintainers run a script once a year to update this for all files.

    jonatack commented at 9:05 AM on November 7, 2019:

    Done

  10. Sjors approved
  11. Sjors commented at 5:48 PM on November 4, 2019: member

    ACK d2edf02 on macOS 10.15 with --enable-debug (which is slower, but better at catching issues).

    Before: 18-25 seconds After: 10 seconds

    Part of inconsistency can come from caching.

  12. in test/functional/wallet_avoidreuse.py:223 in d2edf02d54 outdated 
     223 |  
 224 | -        # listunspent should show 2 total outputs (5, 10 btc), one unused (5), one reused (10)
 225 | +        # listunspent should show 2 total outputs (5, 10 btc), one unused (5), one reused (10).
 226 |          assert_unspent(self.nodes[1], total_count=2, total_sum=15, reused_count=1, reused_sum=10)
 227 | -        # getbalances should show 10 used, 5 btc trusted
 228 | +        # getbalances should show 10 used, 5 btc trusted.
    MarcoFalke commented at 6:16 PM on November 4, 2019:

    I don't know if it makes sense to add trailing dots to each comment that is just a single sentence

    jonatack commented at 6:31 PM on November 4, 2019:

    I was following https://www.python.org/dev/peps/pep-0008/#comments but yes, didn't need to change it.

  13. in test/functional/wallet_avoidreuse.py:243 in d2edf02d54 outdated 
     248 | +        # Node 1 should now have about 1 btc left (no dirty) and 11 (including dirty).
 249 |          assert_approx(self.nodes[1].getbalance(), 1, 0.001)
 250 |          assert_approx(self.nodes[1].getbalance(avoid_reuse=False), 11, 0.001)
 251 |  
 252 | -if __name__ == '__main__':
 253 | +if __name__ == "__main__":
    MarcoFalke commented at 6:17 PM on November 4, 2019:

    Why is this changed. Most other tests use single quotes. In python both serve the same function and I don't see the need to change this.

    jonatack commented at 6:32 PM on November 4, 2019:

    I agree that the change isn't necessary as part of this PR.

  14. in test/functional/wallet_avoidreuse.py:188 in d2edf02d54 outdated 
     197 |          Test the simple case where [1] generates a new address A, then
 198 | -        [0] sends 10 BTC to A.
 199 | -        [1] spends 5 BTC from A. (leaving roughly 5 BTC useable)
 200 | -        [0] sends 10 BTC to A again.
 201 | -        [1] tries to spend 10 BTC (fails; dirty).
 202 | +        [0] sends 10 BTC to A
    MarcoFalke commented at 6:17 PM on November 4, 2019:

    Here you remove the dots, elsewhere you add them?

    jonatack commented at 6:33 PM on November 4, 2019:

    This is a list of partial sentences, but yes, the change isn't required as part of this PR.

  15. MarcoFalke commented at 6:19 PM on November 4, 2019: member

    Just some general notes. I don't see a reason to do most of these changes. Most look like irrelevant character shuffling, which neither the python interpreter should care about nor any human reader of the code.

    No need to change them and invalidate the ACK, but something to keep in mind for the future.

  16. jonatack commented at 6:44 PM on November 4, 2019: member

    @MarcoFalke, thank you for reviewing. I empathise with your remarks and understand the increased review burden and issues of churn/git blame/git history. Will minimise the changeset in the future.

  17. jonatack force-pushed on Nov 6, 2019
  18. test: add logging to wallet_avoidreuse.py 6d50b2606e
  19. test: speed up wallet_avoidreuse.py
    Use -whitelist to speed up transaction relay.

The wallet_avoidreuse.py test is not intended to test transaction relay/timing,
so it should be fine to do this here.

This greatly reduces test run time variability and speeds up the test by 2-3
times on average, e.g. on my system from 20-30 seconds down to 8-10 seconds.
    0e7c90eb37
  20. jonatack force-pushed on Nov 7, 2019
  21. jonatack commented at 9:09 AM on November 7, 2019: member

    Rebased to minimal changeset, no code changes except the following:

    -self.extra_args = [["-whitelist=127.0.0.1"]] * 2
+self.extra_args = [["-whitelist=127.0.0.1"]] * self.num_nodes
  22. MarcoFalke commented at 2:01 PM on November 7, 2019: member

    Thanks. This patch is a lot more candy to my eyes than the previous one.

  23. MarcoFalke commented at 2:03 PM on November 7, 2019: member

    ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012 🐊

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

    Signature:

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

ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012 🐊
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjgfgv/c+HAPiENUjhlN/9JBHfsmMEootFcApeIeuXMOontskood9TPTOARVNt+
jOMDycGM8zEcJh+Fb/Oe6UyI82rUKtKfP6bGGxbhZKRGV+qBIQLOijcu+HyFdbKO
WC6Xg7L8l9v0DllSkWuDx13kgGMUmSLjLGLEptguPobmeSDrHdL/0/HZH35aH7zA
1lzEly99JabUDJckc78G1XYXHWXKImfGxsZHmDwZbGqTJaygEiCa0Psab4iI4W17
VUcaRU/kGCA5owhy6n4tYeGo6/bi4oZ5ImPJ9NkhcNAQJyjV18Rt9y1h2B5T1hXE
CeqXvEvfcXMsrKk1cG9cyNY/V6DnR0zp8jEdDu8BO4LHI9Kx5DmVVFS0p5Icpmgj
w0kD2fGy0KDPDECauFhDB0VTL1x1Fh3iet+QXeW/G4fd6/4FEGYoIUWLtwN9hVwT
M4f50hLAjTyr7qKtebkirMXH2u3dhIwXosLsxK8o+QdyB8YnkMl2mHBre18bgLxU
1pv7IBZ7
=u/gW
-----END PGP SIGNATURE-----

    Timestamp of file with hash 6cefb31533317d8b2a2f9fe6e2a75f803e92835ac7b9cf4100482a5a252a11c1 -

    </details>

  24. fanquake approved
  25. fanquake commented at 4:45 PM on November 7, 2019: member

    ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012

    master (772673dfbe703a383ea05a3960a98a702288d2c6):

    time test/functional/test_runner.py wallet_avoidreuse.py
wallet_avoidreuse.py | ✓ Passed  | 38 s
wallet_avoidreuse.py | ✓ Passed  | 14 s
wallet_avoidreuse.py | ✓ Passed  | 27 s
wallet_avoidreuse.py | ✓ Passed  | 22 s

    This PR (0e7c90eb37a687158c261ddd1ff9f1028a1e7012):

    time test/functional/test_runner.py wallet_avoidreuse.py
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s
  26. jonatack commented at 4:57 PM on November 7, 2019: member

    Thanks. This patch is a lot more candy to my eyes than the previous one.

    I agree -- sorry about that brainfart.

  27. fanquake referenced this in commit 270616228b on Nov 7, 2019
  28. fanquake merged this on Nov 7, 2019
  29. fanquake closed this on Nov 7, 2019
  30. jonatack deleted the branch on Nov 7, 2019
  31. deadalnix referenced this in commit bf0471f9ce on Jun 15, 2020
  32. MarcoFalke referenced this in commit a57af897ec on Aug 16, 2020
  33. sidhujag referenced this in commit 9f7e4e2bb0 on Aug 16, 2020
  34. DrahtBot locked this on Dec 16, 2021
Contributors
jonatackpracticalswiftjachiangSjorsMarcoFalkefanquake
Labels
Tests
Linked (view graph)
#15881 net: Send txs without delay on regtest#17340 Tests: speed up fundrawtransaction test#19564 test: p2p_feefilter improvements (logging, refactoring, speedup)
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