Skip to content

[Wallet][Consensus] Resolve misc clang lock annotation warnings.#952

Merged
codeofalltrades merged 1 commit intoVeil-Project:masterfrom
Zannick:clang4
May 27, 2021
Merged

[Wallet][Consensus] Resolve misc clang lock annotation warnings.#952
codeofalltrades merged 1 commit intoVeil-Project:masterfrom
Zannick:clang4

Conversation

@Zannick
Copy link
Collaborator

@Zannick Zannick commented May 18, 2021

Problem

Building with clang results in lots of warnings about lock annotations not being obeyed.

Solution

Follow instructions on warnings:

  • Replace double lock of cs_main with an assertion.
  • Reorganize code to release cs_main before calling blockheaderToJSON.
  • Lock cs_main before calling any of: GetBlocksToMaturity, GetAddressBalances, IsSpent, TestBlockValidity.

Tested

Configured with --with-sanitizers=thread, built with clang, run on regtest.

@Zannick Zannick self-assigned this May 18, 2021
@Zannick
Resolve misc clang lock annotation warnings.
8d58da5 
- Double lock of cs_main can be replaced with an assertion.
- blockheaderToJSON requires **not** holding cs_main.
- cs_main is required for: GetBlocksToMaturity, GetAddressBalances,
  IsSpent, and TestBlockValidity.
@CaveSpectre11 CaveSpectre11 added Component: Consensus Part of the core cryptocurrency consensus protocol Component: Core App Related to the application itself. Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: Waiting For Code Review Waiting for code review from a core developer labels May 19, 2021
@CaveSpectre11 CaveSpectre11 requested a review from seanPhill May 19, 2021 00:01
CaveSpectre11
CaveSpectre11 reviewed May 19, 2021
View reviewed changes
codeofalltrades
codeofalltrades approved these changes May 22, 2021
View reviewed changes
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 8d58da5

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

codeofalltrades commented May 26, 2021

@CaveSpectre11 is your review a NIT or need to change?

CaveSpectre11
CaveSpectre11 approved these changes May 26, 2021
View reviewed changes
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 ab801ee

@codeofalltrades codeofalltrades merged commit 6e8a420 into Veil-Project:master May 27, 2021
@Zannick Zannick deleted the clang4 branch May 28, 2021 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CaveSpectre11 CaveSpectre11 CaveSpectre11 approved these changes

@codeofalltrades codeofalltrades codeofalltrades approved these changes

@WetOne WetOne Awaiting requested review from WetOne

@seanPhill seanPhill Awaiting requested review from seanPhill

Assignees

@Zannick Zannick

Labels

Code Review: Passed Component: Consensus Part of the core cryptocurrency consensus protocol Component: Core App Related to the application itself. Component: Wallet Relating to keystore, tx creation, and balance tracking Tag: Waiting For QA A pull review is waiting for QA to test the pull request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Zannick @codeofalltrades @CaveSpectre11