rpc: Add missing cs_main lock in getblocktemplate(...) #11694
Conversation
src/rpc/mining.cpp
Outdated
There was a problem hiding this comment.
My suggestion is to create a function like:
bool KeepRunning() {
LOCK(cs_main);
assert(chainActive.Tip());
return chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning();
}And here just:
while (KeepRunning()) {
...
}
|
Restarted travis job. |
Reading the variable `chainActive` requires holding the mutex `cs_main`. Prior to this commit the `cs_main` mutex was not held when accessing `chainActive` in: ``` while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning()) ```
practicalswift
force-pushed
the
missing-lock-in-getblocktemplate
branch
from
dbb4a14 to
6e65f35
Compare
|
The whole point of csBestBlock was that we shouldn't need cs_main to simply check the best block. It looks like the logic for that got lost somewhere, and I'm not sure we're even safe from races anymore. :( Suggest instead of this, fixing csBestBlock so it is sufficient/works. |
|
@promag Now using a lambda to keep it DRY - please re-review :-) |
|
@luke-jr Do you mean that fixing this make things worse? If so, please describe why :-) I'm not familiar with the |
|
Pinging @luke-jr :-) |
|
I agree with @luke-jr on this one. I'd rather fix it to use csBestBlock properly than waste the time figuring out the possible impact of locking cs_main in the loop. |
|
@theuni i find the current code relatively readable, and I'm really not at all sure what "fixing csBestBlock" means, given a condition variable is allowed to wake up spuriously whenever it wants. |
|
@TheBlueMatt as @luke-jr mentioned, presumably cvBlockChange was at one point paired with a uint256 reflecting the miner's best known hash, which at some point got removed. We shouldn't need to take cs_main to check whether the new tip matches the cached one. |
|
If the scope of this grows beyond this PR's goal of fixing the missing If that is something that is unlikely to happen soon I'd vote for merging this fix while waiting for a future better fix. |
|
@theuni "fixing" cvBlockChanged/csBestBlock probably means wholesale removing it and replacing it with appropriate event listeners, but for that, I think, we (realistically) need to beef up the infrastructure behind CValidationInterface so that we dont end up with wallet or other things blocking each other just because they're all on the validation interface (or add some new interface for this stuff that isnt "call into RPC stuff from inside ConnectBlock"), but unless you want to do that, I really think it shouldnt block a simple lock fix. |
|
Ok, since we re-lock cs_main before returning anyway, and since cs_main being locked for any notable period of time after a legitimate cvBlockChange notification would likely indicate a rapid 2nd block anyway, I suppose no regressions would be introduced. So, -0 to 6e65f35. Looks correct to fix the unguarded access. |
|
Any chance of getting this merged? :-) |
src/rpc/mining.cpp
Outdated
| assert(chainActive.Tip()); | ||
| return chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning(); | ||
| }; | ||
| while (KeepRunning()) |
There was a problem hiding this comment.
Sorry but how about plain code?
while (IsRPCRunning()) {
{
LOCK(cs_main);
if (chainActive.Tip()->GetBlockHash() != hashWatchedChain) break;
}
...
src/rpc/mining.cpp
Outdated
| while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning()) | ||
| auto KeepRunning = [&]() -> bool { | ||
| LOCK(cs_main); | ||
| assert(chainActive.Tip()); |
There was a problem hiding this comment.
I would drop the assert, otherwise should we add all over where tip is used?
|
@promag Good suggestion. Please re-review! @TheBlueMatt @theuni Would you mind re-reviewing in light of the latest commit? :-) |
|
Here is an alternative: #12743 |
|
Closing in favour of #12743 which is a better alternative :-) |
4a6c0e3 Modernize best block mutex/cv/hash variable naming (Pieter Wuille) 45dd135 Fix csBestBlock/cvBlockChange waiting in rpc/mining (Pieter Wuille) Pull request description: This is an alternative to #11694. It reintroduces a uint256 variable with the best block hash, protected by csBestBlock, and only updated while holding it. Also rename the involved variable to modern guidelines, as there are very few uses. Tree-SHA512: 826a86c7d3cee7fe49f99f4398ae99e81cb0563197eaeba77306a3ca6072b67cdb932bc35720fc0f99c2a57b218efa029d0b8bdfb240591a629b2e90efa3199d
4a6c0e3 Modernize best block mutex/cv/hash variable naming (Pieter Wuille) 45dd135 Fix csBestBlock/cvBlockChange waiting in rpc/mining (Pieter Wuille) Pull request description: This is an alternative to bitcoin#11694. It reintroduces a uint256 variable with the best block hash, protected by csBestBlock, and only updated while holding it. Also rename the involved variable to modern guidelines, as there are very few uses. Tree-SHA512: 826a86c7d3cee7fe49f99f4398ae99e81cb0563197eaeba77306a3ca6072b67cdb932bc35720fc0f99c2a57b218efa029d0b8bdfb240591a629b2e90efa3199d
4a6c0e3 Modernize best block mutex/cv/hash variable naming (Pieter Wuille) 45dd135 Fix csBestBlock/cvBlockChange waiting in rpc/mining (Pieter Wuille) Pull request description: This is an alternative to bitcoin#11694. It reintroduces a uint256 variable with the best block hash, protected by csBestBlock, and only updated while holding it. Also rename the involved variable to modern guidelines, as there are very few uses. Tree-SHA512: 826a86c7d3cee7fe49f99f4398ae99e81cb0563197eaeba77306a3ca6072b67cdb932bc35720fc0f99c2a57b218efa029d0b8bdfb240591a629b2e90efa3199d
7 participants
Reading the variable
chainActiverequires holding the mutexcs_main.Prior to this commit the
cs_mainmutex was not held when accessingchainActivein: