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 20655
Build 40974: Build Diff	lint-circular-dependencies · build-without-wallet · build-debug · build-diff · build-clang-tidy · build-clang
Build 40973: 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 ↗	(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

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	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.