Skip to content

[Consensus] Add locking on the chain index vector.#950

Merged
codeofalltrades merged 1 commit intoVeil-Project:masterfrom
Zannick:vchain
May 26, 2021
Merged

[Consensus] Add locking on the chain index vector.#950
codeofalltrades merged 1 commit intoVeil-Project:masterfrom
Zannick:vchain

Conversation

@Zannick
Copy link
Collaborator

@Zannick Zannick commented May 18, 2021

Problem

tsan identified a data race on CChain, particularly concurrent access to the underlying vector's operator[], size, and resize member functions.

Solution

Create a new lock cs_vchain and mark vchain as guarded by it, and ensure that the lock is taken prior to any use.
For the operator== member function, grab both the locks in a prescribed order to avoid a deadlock.

Tested

Configured with --with-sanitizers=tsan, built with clang, run on regtest with 2 SHA mining threads enabled.

This alleviates a data race reported by tsan (concurrent access to
size(), operator[], and resize()).
@Zannick Zannick self-assigned this May 18, 2021
@CaveSpectre11 CaveSpectre11 added Component: Consensus Part of the core cryptocurrency consensus protocol Component: Core App Related to the application itself. Tag: Waiting For Code Review Waiting for code review from a core developer labels May 18, 2021
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3cf8acf

@CaveSpectre11 CaveSpectre11 requested a review from seanPhill May 19, 2021 00:19
@WetOne
Copy link
Collaborator

WetOne commented May 23, 2021

When does cs_vchain get unlocked?

@Zannick
Copy link
Collaborator Author

Zannick commented May 23, 2021

When the lock object is destroyed, when it goes out of scope.

@WetOne
Copy link
Collaborator

WetOne commented May 24, 2021

utACK 3cf8acf

@codeofalltrades codeofalltrades added Code Review: Passed Tag: Waiting For QA A pull review is waiting for QA to test the pull request and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels May 24, 2021
Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3cf8acf

@codeofalltrades codeofalltrades merged commit 00001fa into Veil-Project:master May 26, 2021
@Zannick Zannick deleted the vchain branch October 22, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Review: Passed Component: Consensus Part of the core cryptocurrency consensus protocol Component: Core App Related to the application itself. Tag: Waiting For QA A pull review is waiting for QA to test the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants