Details

Reviewers

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

Repository

rABC Bitcoin ABC

Branch

cnode-ava-enabled

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 20626
Build 40916: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 40915: arc lint + arc unit

Event Timeline

sdulfari created this revision.Oct 20 2022, 02:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 20 2022, 02:45

sdulfari requested review of this revision.Oct 20 2022, 02:45

Harbormaster completed remote builds in B20626: Diff 35870.Oct 20 2022, 03:05

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	That's not the networking thread, you want to use an atomic bool
5491	OK, removing the check on pnode is safe here
5495	Remove the unnecessary scope

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

feedback

sdulfari planned changes to this revision.Oct 20 2022, 18:10

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

Harbormaster completed remote builds in B20655: Diff 35902.Oct 20 2022, 18:44

sdulfari added a child revision: D12345: [avalanche] Fix a data race where updating availability scores can coincide with initialization of the state container.Oct 20 2022, 20:04

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.

Harbormaster completed remote builds in B20670: Diff 35942.Oct 21 2022, 17:17

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

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

sdulfari added a child revision: D12355: [avalanche] Fix a data race where updating availability scores can coincide with initialization of state.Oct 21 2022, 17:53

Failure appears unrelated to changes. Restarting builds...

Fabien accepted this revision.Oct 22 2022, 09:03

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

Closed by commit rABC2ace692109ea: [avalanche] Record peers that have avalanche enabled (authored by sdulfari). · Explain WhyOct 24 2022, 15:57

This revision was automatically updated to reflect the committed changes.

sdulfari added a commit: rABC2ace692109ea: [avalanche] Record peers that have avalanche enabled.