rpc: Add missing cs_main lock in getblocktemplate(...) #11694

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:missing-lock-in-getblocktemplate changing 1 files +7 −1

practicalswift commented at 3:21 PM on November 15, 2017: contributor
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 commented at 3:23 PM on November 15, 2017: contributor

@luke-jr @promag, would you mind reviewing this revised version? :-)

in src/rpc/mining.cpp:487 in dbb4a14c7a outdated

 483 | +            bool keepRunning;
 484 | +            {
 485 | +                LOCK(cs_main);
 486 | +                keepRunning = chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning();
 487 | +            }
 488 | +            while (keepRunning)

promag commented at 3:29 PM on November 15, 2017:

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()) {
    ...
}

promag commented at 3:30 PM on November 15, 2017: member

Concept ACK.
fanquake added the label RPC/REST/ZMQ on Nov 15, 2017
promag commented at 4:11 PM on November 15, 2017: member

Restarted travis job.

rpc: Add missing cs_main lock in getblocktemplate(...)

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())
```

6e65f35f23

practicalswift force-pushed on Nov 15, 2017
luke-jr commented at 4:43 PM on November 15, 2017: member

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.
practicalswift commented at 4:45 PM on November 15, 2017: contributor

@promag Now using a lambda to keep it DRY - please re-review :-)
practicalswift commented at 7:08 PM on November 21, 2017: contributor

@luke-jr Do you mean that fixing this make things worse? If so, please describe why :-)

I'm not familiar with the csBestBlock issue you are describing, so I'm afraid I'll have to leave fixing that to someone else :-)
practicalswift commented at 8:32 AM on November 30, 2017: contributor

Pinging @luke-jr :-)
TheBlueMatt commented at 5:58 PM on December 4, 2017: contributor

No reason to wait on a "better fix" before merging this. Lets just merge and then @luke-jr can fix it in the way he wants in a cleanup follow-up.

utACK 6e65f35f23337cf0b284acb2bb2ec038ae83aeca
theuni commented at 6:42 PM on December 4, 2017: member

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.
TheBlueMatt commented at 6:45 PM on December 4, 2017: contributor

@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.
theuni commented at 7:06 PM on December 4, 2017: member

@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.
practicalswift commented at 7:28 PM on December 4, 2017: contributor

If the scope of this grows beyond this PR's goal of fixing the missing cs_main lock I'm afraid I'll have to leave that for others to fix. Let me know when a fix for the issue described by @luke-jr is available and I'll close this PR.

If that is something that is unlikely to happen soon I'd vote for merging this fix while waiting for a future better fix.
TheBlueMatt commented at 7:53 PM on December 4, 2017: contributor

@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.
theuni commented at 8:15 PM on December 4, 2017: member

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 6e65f35f23337cf0b284acb2bb2ec038ae83aeca. Looks correct to fix the unguarded access.
practicalswift cross-referenced this on Jan 29, 2018 from issue Optimise lock behaviour for GuessVerificationProgress() by jonasschnelli
practicalswift cross-referenced this on Mar 12, 2018 from issue [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) by practicalswift
practicalswift commented at 8:02 PM on March 14, 2018: contributor

Any chance of getting this merged? :-)

in src/rpc/mining.cpp:487 in 6e65f35f23 outdated

 483 | +            auto KeepRunning = [&]() -> bool {
 484 | +                LOCK(cs_main);
 485 | +                assert(chainActive.Tip());
 486 | +                return chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning();
 487 | +            };
 488 | +            while (KeepRunning())

promag commented at 9:00 PM on March 14, 2018:

Sorry but how about plain code?

while (IsRPCRunning()) {
    {
        LOCK(cs_main);
        if (chainActive.Tip()->GetBlockHash() != hashWatchedChain) break;
    }
    ...

in src/rpc/mining.cpp:484 in 6e65f35f23 outdated

 478 | @@ -479,7 +479,12 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
 479 |              checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
 480 |  
 481 |              WaitableLock lock(csBestBlock);
 482 | -            while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
 483 | +            auto KeepRunning = [&]() -> bool {
 484 | +                LOCK(cs_main);
 485 | +                assert(chainActive.Tip());

promag commented at 9:09 PM on March 14, 2018:

I would drop the assert, otherwise should we add all over where tip is used?

promag commented at 9:11 PM on March 14, 2018: member

utACK 6e65f35, but I have 2 comments.
Simplify KeepRunning logic a8e661b6b2
practicalswift commented at 10:26 PM on March 14, 2018: contributor

@promag Good suggestion. Please re-review! @TheBlueMatt @theuni Would you mind re-reviewing in light of the latest commit? :-)
sipa cross-referenced this on Mar 21, 2018 from issue Fix csBestBlock/cvBlockChange waiting in rpc/mining by sipa
sipa commented at 4:06 AM on March 21, 2018: member

Here is an alternative: #12743
practicalswift commented at 7:09 AM on March 21, 2018: contributor

Closing in favour of #12743 which is a better alternative :-)
practicalswift closed this on Mar 21, 2018
sipa referenced this in commit 4ba6da5574 on Apr 13, 2018
HashUnlimited cross-referenced this on Sep 5, 2018 from issue Fix csBestBlock/cvBlockChange waiting in rpc/mining by HashUnlimited
PastaPastaPasta referenced this in commit 1cfba98147 on Apr 12, 2020
PastaPastaPasta referenced this in commit beb57b1333 on Apr 16, 2020
PastaPastaPasta referenced this in commit e480ae9fa8 on Apr 16, 2020
ckti referenced this in commit d5ee70493f on Mar 28, 2021
practicalswift deleted the branch on Apr 10, 2021
gades referenced this in commit f6742a4a7f on Mar 17, 2022
bitcoin locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#11226 [WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock)#12287 Optimise lock behaviour for GuessVerificationProgress()#12743 Fix csBestBlock/cvBlockChange waiting in rpc/mining