Skip to content

Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache(). Add missing locking annotations.#15192

Closed
practicalswift wants to merge 4 commits intobitcoin:masterfrom
practicalswift:validation-cs_main
Closed

Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache(). Add missing locking annotations.#15192
practicalswift wants to merge 4 commits intobitcoin:masterfrom
practicalswift:validation-cs_main

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Jan 17, 2019 •
edited by maflcko
Loading

  • Add missing cs_main lock required when accessing pblocktree. Add annotation.
  • Add missing cs_main lock required when accessing pcoinsTip. Add annotation.
  • Add missing cs_main lock required when accessing pcoinsdbview. Add annotation.
  • Add missing cs_main lock required when accessing scriptExecutionCache. Add annotation.

@practicalswift practicalswift changed the title Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add annotations. Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add missing locking annotations. Jan 17, 2019
@practicalswift practicalswift mentioned this pull request Jan 17, 2019
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 17, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16443 (refactor: have CCoins* data managed under CChainState by jamesob)
  • #15997 (refactor: GuessVerificationProgress requires cs_main lock by promag)
  • #13204 (Faster sigcache nonce by JeremyRubin)

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.

maflcko
maflcko reviewed Jan 25, 2019
View reviewed changes
@practicalswift
Copy link
Contributor Author

practicalswift commented Jan 25, 2019

@MarcoFalke Thanks for reviewing. Feedback addressed! Please re-review :-)

@practicalswift
Copy link
Contributor Author

practicalswift commented Jan 31, 2019

@MarcoFalke Would it be possible to get a release milestone for this PR? :-)

@maflcko maflcko added this to the 0.18.0 milestone Jan 31, 2019
maflcko
maflcko reviewed Feb 6, 2019
View reviewed changes
@practicalswift practicalswift force-pushed the validation-cs_main branch 2 times, most recently from b80fa6a to 6c689dd Compare February 7, 2019 07:54
@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 7, 2019

@MarcoFalke Fixed! Please re-review :-)

@maflcko maflcko removed this from the 0.18.0 milestone Feb 11, 2019
@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 12, 2019

@MarcoFalke What is the reason for the removal of the 0.18.0 milestone?

@maflcko
Copy link
Member

maflcko commented Feb 12, 2019

It couldn't be merged without review, and that seems not trivial in some of the hunks

@maflcko
Copy link
Member

maflcko commented Feb 12, 2019

anything in src/test and src/bench looks good and can be merged

@practicalswift practicalswift mentioned this pull request Feb 15, 2019
Merged
maflcko pushed a commit that referenced this pull request Feb 15, 2019
Merge #15413: tests: Add missing cs_main locks required when accessin…
4d2767c 
…g pcoinsdbview, pcoinsTip or pblocktree

543ef7d tests: Add missing cs_main locks required when accessing pcoinsdbview, pcoinsTip or pblocktree (practicalswift)

Pull request description:

  Add missing `cs_main` locks required when accessing `pcoinsdbview`, `pcoinsTip` or `pblocktree`.

  This is a subset of #15192: split up requested by MarcoFalke in #15192 (comment).

  The end goal is to get the corresponding `GUARDED_BY(...)`:s in (see #15192).

Tree-SHA512: 0eb1987dba1a2f1faf0910c421f6d90a20b8a253486eb3301d5bca66d128b19120664e3a8580bdce7b428df817284faf94243250bf561f91d2d31a52d134aa67
@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 15, 2019

@MarcoFalke Thanks for the quick turnaround with regards to the test/bench split-up PR. I've now rebased this one on top of master.

Regarding the remaining changes: perhaps I should exclude any non-trivial changes (by opting out) so that we get proper annotations (GUARDED_BY). That will help guard against future missed locks which is one step forward :-)

@practicalswift
Copy link
Contributor Author

practicalswift commented Feb 15, 2019 •
edited
Loading

@MarcoFalke Is it the added lock InitScriptExecutionCache you feel could be non-trivial to review? If so I can just skip it :-)

@practicalswift practicalswift mentioned this pull request Mar 4, 2019
Merged
@practicalswift practicalswift changed the title Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add missing locking annotations. Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache(). Add missing locking annotations. Mar 4, 2019
@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 5, 2019

Rebased!

@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 26, 2019

@MarcoFalke Should this locking PR be closed? Please advice.

@maflcko
Copy link
Member

maflcko commented Jun 26, 2019

Concept ACK

@maflcko maflcko added this to the 0.19.0 milestone Jun 26, 2019
@practicalswift practicalswift deleted the validation-cs_main branch April 10, 2021 19:38
Munkybooty pushed a commit to Munkybooty/dash that referenced this pull request Sep 13, 2021
@Munkybooty
Partial Merge bitcoin#15413: tests: Add missing cs_main locks require…
06925a1 
…d when accessing pcoinsdbview, pcoinsTip or pblocktree

543ef7d tests: Add missing cs_main locks required when accessing pcoinsdbview, pcoinsTip or pblocktree (practicalswift)

Pull request description:

  Add missing `cs_main` locks required when accessing `pcoinsdbview`, `pcoinsTip` or `pblocktree`.

  This is a subset of bitcoin#15192: split up requested by MarcoFalke in bitcoin#15192 (comment).

  The end goal is to get the corresponding `GUARDED_BY(...)`:s in (see bitcoin#15192).

Tree-SHA512: 0eb1987dba1a2f1faf0910c421f6d90a20b8a253486eb3301d5bca66d128b19120664e3a8580bdce7b428df817284faf94243250bf561f91d2d31a52d134aa67
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ Validation

Projects

None yet

Milestone

0.19.0

Development

Successfully merging this pull request may close these issues.

3 participants

@practicalswift @DrahtBot @maflcko