Skip to content

Conversation

knst
Copy link
Collaborator

@knst knst commented Aug 29, 2024

Issue being fixed or feature implemented

Lately our CI for tsan is flapping many functional tests and take long times.

This PR has several important changes backported from the latest bitcoin's version to improve CI experience

What was done?

This PR has several backports that improved CI experience drastically.

Firstly, it aims to fix flapping test p2p_node_network_limited.py

For example: https://gitlab.com/dashpay/dash/-/jobs/7692635307

p2p_node_network_limited.py | ✖ Failed | 28 s
 test  2024-08-29T02:50:53.929000Z TestFramework (ERROR): Assertion failed 
Traceback (most recent call last):
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_framework.py", line 158, in main
  self.run_test()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/p2p_node_network_limited.py", line 79, in run_test
  self.nodes[0].disconnect_p2ps()
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/test_node.py", line 611, in disconnect_p2ps
  wait_until_helper(check_peers, timeout=5)
File "/builds/dashpay/dash/build-ci/dashcore-linux64_tsan/test/functional/test_framework/util.py", line 262, in wait_until_helper
  raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
  AssertionError: Predicate ''''
       def check_peers():
           for p in self.getpeerinfo():
               for p2p in self.p2ps:
                   if p['subver'] == p2p.strSubVer:
                       return False
           return True
''' not true after 5.0 seconds

Secondly, it improves performance of Descriptor wallets significantly for case of tsan CI. It is tiny improvement for Release build and local runs, but some fucnctional tests run as fast as twice:

https://gitlab.com/dashpay/dash/-/jobs/7694458953
https://gitlab.com/dashpay/dash/-/jobs/7665132625

wallet_create_tx.py --descriptors                  | ✓ Passed  | 236 s <-- new version
wallet_create_tx.py --legacy-wallet                | ✓ Passed  | 108 s 
wallet_basic.py --descriptors                      | ✓ Passed  | 135 s <---- new version
wallet_basic.py --legacy-wallet                    | ✓ Passed  | 97 s


wallet_create_tx.py --descriptors                  | ✓ Passed  | 456 s <-- old version
wallet_create_tx.py --legacy-wallet                | ✓ Passed  | 98 s
wallet_basic.py --descriptors                      | ✓ Passed  | 189 s <--- old version
wallet_basic.py --legacy-wallet                    | ✓ Passed  | 131 s

See performance investigation here: #6226

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

furszy and others added 4 commits August 29, 2024 17:23
…r spkm descriptor ID

BACKPORT NOTICE

It includes only this commit: 97a965d
refactor: extract descriptor ID calculation from spkm GetID()

This allows us to verify the descriptor ID on the descriptors
unit tests in different software versions without requiring to
use the entire DescriptorScriptPubKeyMan machinery.

Note:
The unit test changes are introduced after the bugfix commit
but this commit + the unit test commit can be cherry-picked
on top of the v25 branch to verify IDs correctness. IDs must
be the same for v25 and after the bugfix commit.
…eated descriptor string creation

BACKPORT NOTICE:
It doesn't include changes related to wallet_miniscript.py

5e6bc6d test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner (Sebastian Falbesoner)
f811a24 wallet: cache descriptor ID to avoid repeated descriptor string creation (Sebastian Falbesoner)

Pull request description:

  Right now a wallet descriptor is converted to its string representation (via `Descriptor::ToString`) repeatedly at different instances:
  - on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
  - whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like `FastWalletRescanFilter` etc.

  As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that.

  This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.

  Fixes bitcoin#28800.

ACKs for top commit:
  Sjors:
    ACK 5e6bc6d
  S3RK:
    Code Review ACK 5e6bc6d
  achow101:
    ACK 5e6bc6d
  BrandonOdiwuor:
    ACK 5e6bc6d

Tree-SHA512: 98b43963a5dde6055bb26cecd3b878dadd837d6226af4c84142383310495da80b3c4bd552e73b9107f2f2ff1c11f5e18060c6fd3d9e44bbd5224114c4d245c1c
…e functional tests

BACKPORT NOTICE
It includes only this commit: be25ac3

[init] Remove -addrmantest command line arg

-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.

Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2eadcf2

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2eadcf2

@PastaPastaPasta PastaPastaPasta merged commit cddbc2a into dashpay:develop Aug 30, 2024
35 checks passed
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants