From ad5f01b96045f304b6cf9100879592b835c49c40 Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 16 Feb 2021 11:32:49 -0800 Subject: [PATCH 1/2] [validation] Move the lock annotation from function definition to declaration When the annotation is on the definition, it does not check call sites between the declaration and the definition. --- src/test/txvalidationcache_tests.cpp | 2 +- src/validation.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index af0090cc108..18c1c674503 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -13,7 +13,7 @@ #include -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); BOOST_AUTO_TEST_SUITE(txvalidationcache_tests) diff --git a/src/validation.cpp b/src/validation.cpp index 31609ea3e57..7f5b3e0b22c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -198,7 +198,7 @@ CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlo std::unique_ptr pblocktree; -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks = nullptr); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); @@ -1450,7 +1450,7 @@ void InitScriptExecutionCache() { * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks) { if (tx.IsCoinBase()) return true; From 25c57d640992255ed67964a44b17afbfd4bed0cf Mon Sep 17 00:00:00 2001 From: Amiti Uttarwar Date: Tue, 16 Feb 2021 11:41:41 -0800 Subject: [PATCH 2/2] [doc] Add a note about where lock annotations should go. --- doc/developer-notes.md | 5 +++++ src/test/txvalidationcache_tests.cpp | 5 ++++- src/validation.cpp | 11 +++++++++-- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 011c38321c0..8f2d7af0899 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -785,6 +785,11 @@ Threads and synchronization get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with run-time asserts in function definitions: + - In functions that are declared separately from where they are defined, the + thread safety annotations should be added exclusively to the function + declaration. Annotations on the definition could lead to false positives + (lack of compile failure) at call sites between the two. + ```C++ // txmempool.h class CTxMemPool diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index 18c1c674503..f9d93942b91 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -13,7 +13,10 @@ #include -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); BOOST_AUTO_TEST_SUITE(txvalidationcache_tests) diff --git a/src/validation.cpp b/src/validation.cpp index 7f5b3e0b22c..4c33342c448 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -198,7 +198,11 @@ CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlo std::unique_ptr pblocktree; -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false); static FlatFileSeq BlockFileSeq(); static FlatFileSeq UndoFileSeq(); @@ -1450,7 +1454,10 @@ void InitScriptExecutionCache() { * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector* pvChecks) +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, + const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, + bool cacheFullScriptStore, PrecomputedTransactionData& txdata, + std::vector* pvChecks) { if (tx.IsCoinBase()) return true;