Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Dec 1, 2018

Conversation

Projects
None yet
10 participants
@hebasto
Copy link
Member

commented Nov 29, 2018

This is an alternative to #14803.

Refs:

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?

@fanquake fanquake added the Consensus label Nov 29, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

#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.

@hebasto hebasto force-pushed the hebasto:20181129-checkblock-mutex branch Nov 29, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

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

@kallewoof
Copy link
Member

left a comment

utACK

Show resolved Hide resolved src/validation.cpp Outdated
Move CheckBlock() call to critical section
This prevents data race for CBlock::fChecked.

@hebasto hebasto force-pushed the hebasto:20181129-checkblock-mutex branch to c5ed6e7 Nov 30, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

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

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

utACK. I talked to @sdaftuar and it sounded like this change is fine, it was an incidental side effect of an earlier change.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

How can the comment line modifying cause mempool_persist.py test failure in Travis?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

utACK c5ed6e7

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Has someone validated that all other calls to checkblock are also under a lock?

@promag

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Yes, but adding the annotation would poison the blockencodings header. Not sure if that is worth it.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Great! (still utACK)

@meshcollider

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

utACK c5ed6e7

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

utACK c5ed6e7

@laanwj laanwj merged commit c5ed6e7 into bitcoin:master Dec 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 1, 2018

Merge #14841: consensus: Move CheckBlock() call to critical section
c5ed6e7 Move CheckBlock() call to critical section (Hennadii Stepanov)

Pull request description:

  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

Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb

@hebasto hebasto deleted the hebasto:20181129-checkblock-mutex branch Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.