Page MenuHomePhabricator

[avalanche] Record peers that have avalanche enabled
ClosedPublic

Authored by sdulfari on Oct 20 2022, 02:45.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC2ace692109ea: [avalanche] Record peers that have avalanche enabled
Summary

m_avalanche_state is currently serving a dual purpose: to store state of peers that have been polled
and to determine if a peer supports avalanche. This patch splits those responsibilities apart so that
the latter is recorded as a new state m_avalanche_enabled. This will become more relevant as additional
refactors are done to fix a TSAN data race related to m_avalanche_state.

There is no change in behavior.

This patch also includes some refactors and redundant checks to make the code easier to understand and
refactor further.

Test Plan
ninja check check-functional

Diff Detail

Event Timeline

Fabien requested changes to this revision.Oct 20 2022, 07:03
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/net_processing.cpp
1712 ↗(On Diff #35870)

That's not the networking thread, you want to use an atomic bool

5491 ↗(On Diff #35870)

OK, removing the check on pnode is safe here

5495 ↗(On Diff #35870)

Remove the unnecessary scope

This revision now requires changes to proceed.Oct 20 2022, 07:03
  • Fix the score comparator to meet Comparator requirements when at least one of the CNodes has no m_avalanche_state
  • Set m_avalanche_enabled to true in addavalanchenode so AVAHELLO is appropriately ignored like past behavior
Fabien requested changes to this revision.Oct 21 2022, 07:21
Fabien added inline comments.
src/net_processing.cpp
4918 ↗(On Diff #35902)

Use compare_exchange_strong() (https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange) or test_and_set() (https://en.cppreference.com/w/cpp/atomic/atomic_flag/test_and_set, but I don't think we use it anywhere in our codebase)

This revision now requires changes to proceed.Oct 21 2022, 07:21

Use compare_exchange_strong per feedback. This also fixes the race between AVAHELLO and addavalanchenode,
but the race between UpdateAvalancheStatistics and AVAHELLO/addavalanchenode still exists.

Failed tests logs:

====== Bitcoin ABC functional tests: feature_asmap.py ======

------- Stdout: -------
2022-10-21T17:16:22.811000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20221021_171430/feature_asmap_94
2022-10-21T17:16:23.106000Z TestFramework (INFO): Test bitcoind with no -asmap arg passed
2022-10-21T17:16:23.619000Z TestFramework (INFO): Test bitcoind -asmap=<absolute path>
2022-10-21T17:16:24.079000Z TestFramework (INFO): Test bitcoind -asmap=<relative path>
2022-10-21T17:16:24.535000Z TestFramework (INFO): Test bitcoind -asmap (using default map file)
2022-10-21T17:16:25.041000Z TestFramework (INFO): Test bitcoind -asmap= (using default map file)
2022-10-21T17:16:25.546000Z TestFramework (INFO): Test bitcoind -asmap restart with addrman containing new and tried entries
2022-10-21T17:16:28.518000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/feature_asmap.py", line 139, in run_test
    self.test_asmap_interaction_with_addrman_containing_entries()
  File "/work/test/functional/feature_asmap.py", line 105, in test_asmap_interaction_with_addrman_containing_entries
    self.node.getnodeaddresses()
  File "/usr/lib/python3.9/contextlib.py", line 124, in __exit__
    next(self.gen)
  File "/work/test/functional/test_framework/test_node.py", line 532, in assert_debug_log
    self._raise_assertion_error(
  File "/work/test/functional/test_framework/test_node.py", line 212, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['Addrman checks started: new 2, tried 2, total 4', 'Addrman checks completed successfully']" does not partially match log:

 - 2022-10-21T17:16:26.508382Z [../../src/httpserver.cpp:249] [http_request_cb] Received a POST request for / from 127.0.0.1:34314
 - 2022-10-21T17:16:26.508426Z [../../src/rpc/request.cpp:182] [parse] ThreadRPCServer method=getnodeaddresses user=__cookie__
 - 2022-10-21T17:16:26.508459Z [../../src/addrman.cpp:1058] [ForceCheckAddrman] Addrman checks started: new 3, tried 1, total 4
 - 2022-10-21T17:16:26.508527Z [../../src/addrman.cpp:1160] [ForceCheckAddrman] Addrman checks completed successfully
 - 2022-10-21T17:16:26.508535Z [../../src/addrman.cpp:886] [GetAddr_] GetAddr returned 1 random addresses
 - 2022-10-21T17:16:26.508540Z [../../src/addrman.cpp:1058] [ForceCheckAddrman] Addrman checks started: new 3, tried 1, total 4
 - 2022-10-21T17:16:26.508599Z [../../src/addrman.cpp:1160] [ForceCheckAddrman] Addrman checks completed successfully


2022-10-21T17:16:28.569000Z TestFramework (INFO): Stopping nodes
2022-10-21T17:16:28.771000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20221021_171430/feature_asmap_94
2022-10-21T17:16:28.771000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20221021_171430/feature_asmap_94/test_framework.log
2022-10-21T17:16:28.771000Z TestFramework (ERROR): 
2022-10-21T17:16:28.771000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20221021_171430/feature_asmap_94' to consolidate all logs
2022-10-21T17:16:28.771000Z TestFramework (ERROR): 
2022-10-21T17:16:28.771000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-10-21T17:16:28.771000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2022-10-21T17:16:28.771000Z TestFramework (ERROR):

Each failure log is accessible here:
Bitcoin ABC functional tests: feature_asmap.py

Failure appears unrelated to changes. Restarting builds...

This revision is now accepted and ready to land.Oct 22 2022, 09:03