[primitives, consensus] Make fChecked an atomic<bool> #14072

pull skeees wants to merge 1 commits into bitcoin:master from skeees:fchecked-atomic changing 1 files +19 −1

skeees commented at 6:22 PM on August 26, 2018: contributor

This addresses a data race / thread sanitizer issue uncovered in #14058

If there are alternate suggestions about how to effectuate this change in a more compact way without having to manually declare the copy constructor and copy assignment operator (because it is deleted for std::atomic<bool> that feedback would be welcome).

CheckBlock can be invoked from multiple threads in parallel. To avoid a data race, fChecked should be atomic. In practice, this would have had limited effect currently since these paths are effectively single-threaded in practice, except to maybe occasionally cause a slight bit of redundant work to be performed in CheckBlock.

Make fChecked an atomic<bool>

CheckBlock can be invoked from multiple threads in parallel. To avoid
a data race, fChecked should be atomic. In practice, this would have
had limited effect currently since these paths are effectively
single-threaded in practice, except to maybe occasionally cause a
slight bit of redundant work to be performed in CheckBlock.

ffb0adde09

DrahtBot commented at 8:32 PM on August 26, 2018: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #10785 (Serialization improvements by sipa)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/primitives/block.h:97 in ffb0adde09

  89 | @@ -89,6 +90,23 @@ class CBlock : public CBlockHeader
  90 |          *(static_cast<CBlockHeader*>(this)) = header;
  91 |      }
  92 |  
  93 | +    CBlock(const CBlock &block)
  94 | +        :CBlock(static_cast<CBlockHeader>(block))
  95 | +    {
  96 | +        vtx = block.vtx;
  97 | +        fChecked = false;

MarcoFalke commented at 1:53 AM on August 27, 2018:

Any reason to set this to false when copying?

nit: Also, could rename the member to m_checked, since it is only used in one function on master.

practicalswift commented at 7:34 AM on September 5, 2018:

Perform initialization in initialization list instead :-)

in src/primitives/block.h:96 in ffb0adde09

  89 | @@ -89,6 +90,23 @@ class CBlock : public CBlockHeader
  90 |          *(static_cast<CBlockHeader*>(this)) = header;
  91 |      }
  92 |  
  93 | +    CBlock(const CBlock &block)
  94 | +        :CBlock(static_cast<CBlockHeader>(block))
  95 | +    {
  96 | +        vtx = block.vtx;

practicalswift commented at 7:34 AM on September 5, 2018:

Perform initialization in initialization list instead :-)

laanwj commented at 7:44 AM on November 23, 2018: member

Concept ACK
MarcoFalke added the label Up for grabs on Nov 23, 2018
MarcoFalke commented at 10:21 PM on November 23, 2018: member

Could remove the tsan suppression for fChecked after a rebase.
fanquake commented at 11:27 PM on November 25, 2018: member

Closing in favour of #14803.
fanquake closed this on Nov 25, 2018
fanquake removed the label Up for grabs on Nov 25, 2018
laanwj referenced this in commit 5ab5341d13 on Dec 1, 2018
Munkybooty referenced this in commit f60f1954de on Aug 17, 2021
vijaydasmp referenced this in commit 845b6df19e on Sep 5, 2021
vijaydasmp referenced this in commit 67893ff277 on Sep 6, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#14058 unit tests fail --with-sanitizers=thread #14803 consensus: Avoid data race in CBlock class #14841 consensus: Move CheckBlock() call to critical section

[primitives, consensus] Make fChecked an atomic<bool> #14072

Conflicts