test: fix immediate tx relay in wallet_groups.py #26970

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202301-test-fix_wallet_groups_immediate_tx_relay changing 1 files +5 −0
  1. theStack commented at 2:06 am on January 26, 2023: contributor

    In the functional test wallet_groups.py we whitelist peers on all nodes (-whitelist=noban@127.0.0.1) to enable immediate tx relay for fast mempool synchronization. However, considering that this setting only applies to inbound peers and the default test topology looks like this:

    0    node0 <--- node1 <---- node2 <--- ... <-- nodeN
    

    txs propagate fast only from lower- to higher-numbered nodes (i.e. “left to right” in the above diagram) and take long from higher- to lower-numbered nodes (“right to left”) since in the latter direction we only have outbound peers, where the trickle relay is still active. As a consequence, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.

    This PR fixes this by simply adding another connection from node0 to the last node, leading to a ~2-3x speedup (5 runs measured via time ./test/functional/wallet_groups.py are shown):

     0master:
     1    0m53.31s real     0m08.22s user     0m05.60s system
     2    0m32.85s real     0m07.44s user     0m04.08s system
     3    0m46.40s real     0m09.18s user     0m04.23s system
     4    0m46.96s real     0m11.10s user     0m05.74s system
     5    0m57.23s real     0m10.53s user     0m05.59s system
     6
     7PR:
     8    0m19.64s real     0m09.58s user     0m05.50s system
     9    0m18.05s real     0m07.77s user     0m04.03s system
    10    0m18.99s real     0m07.90s user     0m04.25s system
    11    0m17.49s real     0m07.56s user     0m03.92s system
    12    0m18.11s real     0m07.74s user     0m03.88s system
    

    Note that in most tests this is not a problem since txs very often originate from node0.

  2. test: fix immediate tx relay in wallet_groups.py ab4efad51b
  3. DrahtBot commented at 2:06 am on January 26, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK brunoerg

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. DrahtBot added the label Tests on Jan 26, 2023
  5. furszy commented at 1:07 pm on January 26, 2023: member

    Good catch.

    I think that would be preferable to enable whitelisting for outbound peers rather than tackle each specific test case with workarounds like this one (the trickle mechanism is irrelevant for the large majority of tests).

    Without knowing the background story, it seems odd to only apply the whitelist flag to connections that weren’t originated by the node. Supposedly, as the node picked it, outbound connections should be “more” trustworthy than inbound (so, disable the trickle mechanism if the address was whitelisted shouldn’t be a problem).

  6. brunoerg commented at 5:21 pm on January 27, 2023: contributor

    Got these results from my macOS 13 machine:

    master: ./test/functional/wallet_groups.py 2.69s user 0.81s system 9% cpu 35.928 total ./test/functional/wallet_groups.py 2.65s user 0.84s system 9% cpu 36.626 total ./test/functional/wallet_groups.py 2.80s user 0.88s system 9% cpu 40.715 total ./test/functional/wallet_groups.py 2.72s user 0.90s system 10% cpu 34.687 total ./test/functional/wallet_groups.py 2.54s user 0.78s system 13% cpu 25.363 total

    pr branch: ./test/functional/wallet_groups.py 2.29s user 0.63s system 24% cpu 11.881 total ./test/functional/wallet_groups.py 2.39s user 0.65s system 25% cpu 12.164 total ./test/functional/wallet_groups.py 2.30s user 0.71s system 22% cpu 13.155 total ./test/functional/wallet_groups.py 2.29s user 0.72s system 28% cpu 10.510 total ./test/functional/wallet_groups.py 2.37s user 0.74s system 25% cpu 12.234 total

  7. brunoerg approved
  8. brunoerg commented at 8:22 pm on February 7, 2023: contributor
    utACK ab4efad51b9ba276ffeb6871931e13772493f7cc
  9. furszy commented at 3:33 pm on February 12, 2023: member
    Seems that what I wrote above was suggested years ago #9923. Might be a good opportunity to revive the topic.
  10. brunoerg commented at 3:43 pm on February 12, 2023: contributor

    Seems that what I wrote above was suggested years ago #9923. Might be a good opportunity to revive the topic.

    See #26441

  11. maflcko merged this on Feb 13, 2023
  12. maflcko closed this on Feb 13, 2023

  13. sidhujag referenced this in commit fd4f02c200 on Feb 13, 2023
  14. theStack deleted the branch on Feb 13, 2023
  15. bitcoin locked this on Feb 13, 2024

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: 2024-07-03 13:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me