[validation] Two small clang lock annotation improvements #21202

amitiuttarwar commented at 7:48 PM on February 16, 2021: contributor

Based on reviewing #21188

the first commit switches the lock annotations on CheckInputScripts to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition.

the second commit adds a note to the developer-notes section to clarify where the annotations should be applied.

amitiuttarwar commented at 7:53 PM on February 16, 2021: contributor

to test this out, reviewers can apply this diff locally

- bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck>* pvChecks)
+ RecursiveMutex cs_new;
+ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_new)

and see that the compiler warns about the call site from ConnectBlock:

validation.cpp:2136:35: warning: calling function 'CheckInputScripts' requires holding mutex 'cs_new' exclusively [-Wthread-safety-analysis]
            if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], g_parallel_script_checks ? &vChecks : nullptr)) {

but there is no warning from the call site from CheckInputsFromMempoolAndCache or PolicyScriptChecks.

then you can apply the annotation to the declaration instead of the definition, and see that all the call sites generate warnings.

DrahtBot added the label Docs on Feb 16, 2021

DrahtBot added the label Validation on Feb 16, 2021

in src/validation.cpp:201 in a72bff2b4f outdated

 197 | @@ -198,7 +198,7 @@ CBlockIndex* BlockManager::FindForkInGlobalIndex(const CChain& chain, const CBlo
 198 |  
 199 |  std::unique_ptr<CBlockTreeDB> pblocktree;
 200 |  
 201 | -bool CheckInputScripts(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks = nullptr);
 202 | +bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck>* pvChecks = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

hebasto commented at 5:00 AM on February 17, 2021:

What about another declaration https://github.com/bitcoin/bitcoin/blob/36be9b821ae0882ba5ccba543f959636e413991d/src/test/txvalidationcache_tests.cpp#L16?

amitiuttarwar commented at 11:59 PM on February 17, 2021:

good call, added annotation there too.

in doc/developer-notes.md:788 in 9d0975c91d outdated

 784 | @@ -785,6 +785,11 @@ Threads and synchronization
 785 |    get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
 786 |    run-time asserts in function definitions:
 787 |  
 788 | +  - In functions that are declared seperately from where they are defined, the

hebasto commented at 5:02 AM on February 17, 2021:

https://cirrus-ci.com/task/6341367142023168?command=lint#L859:

doc/developer-notes.md:788: seperately ==> separately

amitiuttarwar commented at 12:00 AM on February 18, 2021:

fixed

hebasto commented at 5:06 AM on February 17, 2021: member

If this pr a case to apply #20986?

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

ad5f01b960

[doc] Add a note about where lock annotations should go. 25c57d6409

amitiuttarwar force-pushed on Feb 17, 2021

amitiuttarwar commented at 12:00 AM on February 18, 2021: contributor

thanks for the review @hebasto, took all your suggestions.

If this pr a case to apply #20986?

done

MarcoFalke removed the label Docs on Feb 18, 2021

MarcoFalke added the label Refactoring on Feb 18, 2021

promag commented at 2:32 PM on February 21, 2021: member

Code review ACK 25c57d640992255ed67964a44b17afbfd4bed0cf.

How about adding a validation_p.h with these "private" declarations?

MarcoFalke commented at 8:46 AM on February 22, 2021: member

ACK 25c57d640992255ed67964a44b17afbfd4bed0cf 🥘

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 29b3e3a94f59f0a1d3ed2364dbf2aa60e4373ca5a3c8c71576d43351cf60215b -

</details>

MarcoFalke merged this on Feb 22, 2021

MarcoFalke closed this on Feb 22, 2021

sidhujag referenced this in commit b657411889 on Feb 22, 2021

amitiuttarwar deleted the branch on Feb 22, 2021

amitiuttarwar commented at 7:33 PM on February 22, 2021: contributor

@promag where are other examples of forward declaring functions from the tests? I see examples for other files eg. denialofservice_tests have declarations for orphan handling stuff from net_processing. But I'm not seeing other declarations for internal validation.cpp functions. I'm definitely open to the pattern of test-only header files, but in this case, seems like it might be a bit of overkill to have an additional header file for just one function? Unless I'm missing something?

DrahtBot locked this on Aug 16, 2022