New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc: Lock cs_main in blockToJSON/blockheaderToJSON #11618
rpc: Lock cs_main in blockToJSON/blockheaderToJSON #11618
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert lock is held in those functions?
src/rest.cpp
Outdated
@@ -179,6 +179,7 @@ static bool rest_headers(HTTPRequest* req, | |||
case RF_JSON: { | |||
UniValue jsonHeaders(UniValue::VARR); | |||
for (const CBlockIndex *pindex : headers) { | |||
LOCK(cs_main); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lock once, before loop?
93f57cb
to
d92e055
Compare
@promag Feedback addressed! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda sucks to hold cs_main while calling httpserver methods like WriteHeader/WriteReply - care to just use the recursiveness of cs_main and lock in the ToJSON functions themselves? Longer-term I really want to move chainActive to support read-only locks so we dont block the world for trivial things like chainActive.Contains() and chainActive.Height().
d92e055
to
758b126
Compare
@TheBlueMatt Good point. Locking now moved down one level into |
utACK 758b126897fdc596af70424b6e82cbcda0e3baa3, though the commit message is now somewhat confused (refers to "rest" but only touches src/rpc/*). |
@TheBlueMatt the lock scope could be reduced so that WriteHeader/WriteReply wouldn't keep it. The current approach can give inconsistencies: while iterating the chain can change, and the response can have mixed data. Even with RW locks care must be taken to ensure consistency in the responses. So, IMO this is the correct implementation: case RF_JSON: {
UniValue jsonHeaders(UniValue::VARR);
{
LOCK(cs_main);
for (const CBlockIndex* pindex : headers) {
jsonHeaders.push_back(blockheaderToJSON(pindex));
}
}
std::string strJSON = jsonHeaders.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
req->WriteReply(HTTP_OK, strJSON);
return true;
} WDYT? |
758b126
to
4765b59
Compare
@TheBlueMatt Commit message and PR title adjusted. |
@promag gah, indeed, missed that its called in a loop there - should indeed lock outside the loop. |
4765b59
to
9dda690
Compare
@TheBlueMatt @promag Updated! Please re-review :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 9dda6900.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 9dda690028c719ae074802933f76ef7c45f6dbb6
src/rest.cpp
Outdated
{ | ||
LOCK(cs_main); | ||
for (const CBlockIndex *pindex : headers) { | ||
jsonHeaders.push_back(blockheaderToJSON(pindex)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: bad indentation here.
9dda690
to
a9b6ba0
Compare
Indentation fixed. Please re-review :-) |
re-utACK a9b6ba0. |
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
utACK a9b6ba0 |
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
zcash: cherry picked from commit a9b6ba0 zcash: bitcoin/bitcoin#11618
Bitcoin 0.16 locking PRs These are locking changes from upstream (bitcoin core) release 0.16 (Aug 14, 2017, to Feb 16, 2018), oldest to newest (when merged to the master branch). Each commit also includes a reference both to the PR and the upstream commit. - bitcoin/bitcoin#11126 - Excludes changes to wallet tests that we don't have. - bitcoin/bitcoin#10916 - first commit only; second commit already merged by d9fcc2b - bitcoin/bitcoin#11107 - Only the last commit. - bitcoin/bitcoin#11593 - bitcoin/bitcoin#11585 - bitcoin/bitcoin#11618 - bitcoin/bitcoin#10286 - Only the third and last commits. - bitcoin/bitcoin#11870 - bitcoin/bitcoin#12330 - bitcoin/bitcoin#12366 - bitcoin/bitcoin#12368 - bitcoin/bitcoin#12333 - Only the first commit.
a9b6ba0 Add missing cs_main locks when calling blockToJSON/blockheaderToJSON (practicalswift) Pull request description: `blockToJSON(...)` and `blockheaderToJSON(...)` read the variable `chainActive` which requires holding the mutex `cs_main`. So does `GetDifficulty(...)`. Tree-SHA512: bfb94f5e3238accbf6a4daddde49d53f1891c38ae9b07e25b3098c485747159258f64bb66a50e147b32beac601de89d9d04ff717b6c4f1460d329c90a53d3333
blockToJSON(...)
andblockheaderToJSON(...)
read the variablechainActive
which requires holding the mutexcs_main
. So doesGetDifficulty(...)
.