consensus: Move CheckBlock() call to critical section #14841

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20181129-checkblock-mutex changing 2 files +5 −6

hebasto commented at 11:55 AM on November 29, 2018: member
This is an alternative to #14803.

Refs:
- #14058
- #14072
- #14803 (comment) by @gmaxwell
It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here?
- #14803 (comment) by @MarcoFalke
fanquake added the label Consensus on Nov 29, 2018
practicalswift commented at 12:53 PM on November 29, 2018: contributor

Concept ACK

Thanks for working on this @hebasto

You might want to review #14829 which enables TSan testing in Travis for the functional tests (which are the relevants tests from a data race/threading perspective :-)).
DrahtBot commented at 1:00 PM on November 29, 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:
- #14829 (travis: Enable functional tests in the ThreadSanitizer (TSan) build job by practicalswift)
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.
promag commented at 5:41 PM on November 29, 2018: member

#9765 moved the lock after CheckBlock call for unknown reason cc @sdaftuar.

Verified that all CheckBlock calls hold cs_main, except PartiallyDownloadedBlock::FillBlock. It would be cool to add annotation but because of that exception I don't think it's possible (?)

utACK 9cab291.
MarcoFalke added the label Validation on Nov 29, 2018
MarcoFalke removed the label Consensus on Nov 29, 2018
MarcoFalke added the label Refactoring on Nov 29, 2018

in src/validation.cpp:3536 in 9cab291e47 outdated

3529 | @@ -3530,12 +3530,12 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
3530 |          CBlockIndex *pindex = nullptr;
3531 |          if (fNewBlock) *fNewBlock = false;
3532 |          CValidationState state;
3533 | -        // Ensure that CheckBlock() passes before calling AcceptBlock, as
3534 | -        // belt-and-suspenders.
3535 | -        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
3536 |  
3537 |          LOCK(cs_main);

MarcoFalke commented at 7:38 PM on November 29, 2018:

Could add a comment to explain why this lock is here?

hebasto force-pushed on Nov 29, 2018
hebasto commented at 9:08 PM on November 29, 2018: member

@MarcoFalke Thank you for your review. Your comment has been addressed.

in src/validation.cpp:3534 in 5b6ae66108 outdated

3529 | @@ -3530,12 +3530,14 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<cons
3530 |          CBlockIndex *pindex = nullptr;
3531 |          if (fNewBlock) *fNewBlock = false;
3532 |          CValidationState state;
3533 | -        // Ensure that CheckBlock() passes before calling AcceptBlock, as
3534 | -        // belt-and-suspenders.
3535 | -        bool ret = CheckBlock(*pblock, state, chainparams.GetConsensus());
3536 |  
3537 | +        // CheckBlock() does not support multi-threaded block validation due to CBlock::fChecked can cause data race.

kallewoof commented at 8:57 AM on November 30, 2018:

Wording: due to -> because

kallewoof commented at 8:58 AM on November 30, 2018: member

utACK
Move CheckBlock() call to critical section
This prevents data race for CBlock::fChecked.
c5ed6e73d3
hebasto force-pushed on Nov 30, 2018
hebasto commented at 10:41 AM on November 30, 2018: member

@kallewoof Thank you for your review. Your comment has been addressed.
gmaxwell commented at 10:43 AM on November 30, 2018: contributor

utACK. I talked to @sdaftuar and it sounded like this change is fine, it was an incidental side effect of an earlier change.
hebasto commented at 11:05 AM on November 30, 2018: member

~How can the comment line modifying cause mempool_persist.py test failure in Travis?~
MarcoFalke commented at 3:28 PM on November 30, 2018: member

utACK c5ed6e73d3e18f26558c49fa3ecb1b7b0bb40d12
gmaxwell commented at 6:50 PM on November 30, 2018: contributor

Has someone validated that all other calls to checkblock are also under a lock?
promag commented at 7:04 PM on November 30, 2018: member

@gmaxwell yes ☝️ #14841 (comment)
MarcoFalke commented at 7:10 PM on November 30, 2018: member

Yes, but adding the annotation would poison the blockencodings header. Not sure if that is worth it.
gmaxwell commented at 11:51 PM on November 30, 2018: contributor

Great! (still utACK)
meshcollider commented at 5:44 AM on December 1, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/c5ed6e73d3e18f26558c49fa3ecb1b7b0bb40d12
laanwj commented at 9:27 AM on December 1, 2018: member

utACK c5ed6e73d3e18f26558c49fa3ecb1b7b0bb40d12
laanwj merged this on Dec 1, 2018
laanwj closed this on Dec 1, 2018
laanwj referenced this in commit 5ab5341d13 on Dec 1, 2018
hebasto deleted the branch 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

Labels

Refactoring Validation

Linked (view graph)

#14058 unit tests fail --with-sanitizers=thread #14072 [primitives, consensus] Make fChecked an atomic<bool>#14803 consensus: Avoid data race in CBlock class

consensus: Move CheckBlock() call to critical section #14841

Conflicts