Page MenuHomePhabricator
Differential D14643

Fix chain tip data race and corrupt rest response
ClosedPublic
Actions

Authored by PiRK on Oct 16 2023, 08:01.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC6722d8264193: Fix chain tip data race and corrupt rest response
Summary

https://github.com/bitcoin/bitcoin/pull/25077/commits/fa530bcb9c13b58ab1b2068b48aa3fff910e2f87

Add ChainstateManager::GetMutex(), an alias for ::cs_main

https://github.com/bitcoin/bitcoin/pull/25077/commits/fa97a528d6382a0163d5aa7d37ecbf93579b8186

Fix UB/data-race in RPCNotifyBlockChange

ActiveTip() is *not* thread-safe, as the required ::cs_main lock will be
released as ActiveChainstate() returns.

ActiveTip() is an alias for ActiveChainstate().m_chain.Tip(), so m_chain
may be involved in a data-race (UB).

https://github.com/bitcoin/bitcoin/pull/25077/commits/fac15ff673f0d6f84ea1eaae855597da02b0e510

Fix logical race in rest_getutxos

Calling ActiveHeight() and ActiveTip() subsequently without holding the
::cs_main lock over both calls may result in a height that does not
correspond to the tip due to a race.

Fix this by holding the lock.

https://github.com/bitcoin/bitcoin/pull/25077/commits/fac04cb6ba1d032587bd02eab2247fd655a548cd

refactor: Add lock annotations to Active* methods

This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.

This is a backport of core#25077

Depends on D14642

Test Plan

with debug & tsan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Oct 16 2023, 08:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 16 2023, 08:01
PiRK requested review of this revision.Oct 16 2023, 08:01
teamcity edited the summary of this revision. (Show Details)Oct 16 2023, 08:01
PiRK edited the summary of this revision. (Show Details)Oct 16 2023, 08:02
PiRK added a parent revision: D14642: sync: Use decltype(auto) return type for WITH_LOCK.
PiRK removed a parent revision: D14462: [electrum] remove unnecessary hex conversion when converting int to bytes.
PiRK added a comment.Oct 16 2023, 08:06

@bot build-tsan

teamcity awarded a token.Oct 16 2023, 08:06
PiRK added a child revision: D14644: refactor: Add lock annotations to Active* methods.Oct 16 2023, 08:08
Harbormaster completed remote builds in B25357: Diff 42680.Oct 16 2023, 08:18
teamcity added a comment.Oct 16 2023, 08:23
Build Bitcoin ABC Diffs / Diff Testing (build-tsan) failed.

Failed tests logs:

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

------- Stdout: -------
2023-10-16T08:13:37.534000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-tsan/test/tmp/test_runner_₿₵_  _20231016_081219/abc_p2p_avalanche_voting_10
2023-10-16T08:14:39.173000Z TestFramework (INFO): Poll for the chain tip...
2023-10-16T08:14:39.280000Z TestFramework (INFO): Poll for a selection of blocks...
2023-10-16T08:14:39.344000Z TestFramework (INFO): Poll for a selection of blocks, but some are now invalid...
2023-10-16T08:14:42.444000Z TestFramework (INFO): Poll for unknown blocks...
2023-10-16T08:14:42.559000Z TestFramework (INFO): Trigger polling from the node...
2023-10-16T08:14:48.141000Z TestFramework (INFO): Answer all polls to finalize...
2023-10-16T08:15:22.596000Z TestFramework (INFO): Answer all polls to park...
2023-10-16T08:15:36.007000Z TestFramework (INFO): Verify finalization sticks...
2023-10-16T08:15:36.007000Z TestFramework (INFO): ...for a chain 1 block long...
2023-10-16T08:15:36.097000Z TestFramework (INFO): ...for a chain 2 blocks long...
2023-10-16T08:15:39.674000Z TestFramework (INFO): ...for a chain 3 blocks long...
2023-10-16T08:15:43.537000Z TestFramework (INFO): ...for a chain 4 blocks long...
2023-10-16T08:15:48.143000Z TestFramework (INFO): ...for a chain 5 blocks long...
2023-10-16T08:15:52.223000Z TestFramework (INFO): Check the node is discouraging unexpected avaresponses.
2023-10-16T08:15:52.990000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 142, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_avalanche_voting.py", line 350, in run_test
    poll_node.send_avaresponse(
  File "/usr/lib/python3.9/contextlib.py", line 124, in __exit__
    next(self.gen)
  File "/work/test/functional/test_framework/test_node.py", line 593, in assert_debug_log
    self._raise_assertion_error(
  File "/work/test/functional/test_framework/test_node.py", line 262, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Unexpected message "Misbehaving" partially matches log:

 - 2023-10-16T08:15:52.940371Z (mocktime: 2023-10-16T08:15:52Z) [../../src/net_processing.cpp:3490] [ProcessMessage] received: avaresponse (77 bytes) peer=1
 - 2023-10-16T08:15:52.940782Z (mocktime: 2023-10-16T08:15:52Z) [../../src/net_processing.cpp:1976] [Misbehaving] Misbehaving: peer=1 (0 -> 2): unexpected-ava-response


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

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

Fabien requested changes to this revision.Oct 16 2023, 08:28
Fabien added a subscriber: Fabien.

Is the backport complete ? It seems to be missing lot of content

src/init.cpp
2618 ↗(On Diff #42680)

You missed this one

This revision now requires changes to proceed.Oct 16 2023, 08:28
PiRK added a comment.Oct 16 2023, 08:30
In D14643#328922, @Fabien wrote:

Is the backport complete ? It seems to be missing lot of content

It is partial. The last commit is in its own diff in D14644

PiRK updated this revision to Diff 42686.Oct 16 2023, 09:21

rebase

PiRK planned changes to this revision.Oct 16 2023, 09:22

might need squashing with D14644 when it finally works

PiRK added a comment.Oct 16 2023, 09:23

@bot build-tsan

teamcity updated a token.Oct 16 2023, 09:23
Harbormaster completed remote builds in B25360: Diff 42686.Oct 16 2023, 09:47
PiRK updated this revision to Diff 42692.Oct 16 2023, 13:10
PiRK edited the summary of this revision. (Show Details)

squash with D14644

PiRK added a comment.Oct 16 2023, 13:10

@bot build-tsan

teamcity updated a token.Oct 16 2023, 13:10
Harbormaster completed remote builds in B25365: Diff 42692.Oct 16 2023, 13:24
Fabien accepted this revision.Oct 17 2023, 10:04
This revision is now accepted and ready to land.Oct 17 2023, 10:04
Closed by commit rABC6722d8264193: Fix chain tip data race and corrupt rest response (authored by MacroFake <falke.marco@gmail.com>, committed by PiRK). · Explain WhyOct 18 2023, 04:25
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC6722d8264193: Fix chain tip data race and corrupt rest response.

Revision Contents
Changeset List

Diff 42695

View Options

src/avalanche/test/peermanager_tests.cpp

Loading...
View Options

src/avalanche/test/processor_tests.cpp

Loading...
View Options

src/init.cpp

Loading...
View Options

src/net_processing.cpp

Loading...
View Options

src/qt/test/wallettests.cpp

Loading...
View Options

src/rest.cpp

Loading...
View Options

src/test/blockstorage_tests.cpp

Loading...
View Options

src/test/interfaces_tests.cpp

Loading...
View Options

src/test/miner_tests.cpp

Loading...
View Options

src/test/txvalidationcache_tests.cpp

Loading...
View Options

src/test/util/setup_common.cpp

Loading...
View Options

src/test/validation_block_tests.cpp

Loading...
View Options

src/test/validation_chainstatemanager_tests.cpp

Loading...
View Options

src/test/validationinterface_tests.cpp

Loading...
View Options

src/validation.h

Loading...
View Options

src/wallet/test/wallet_tests.cpp

Loading...