Skip to content

rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON #12151

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 4, 2019
Merged

rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON #12151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54dc13b
rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures
promag Jan 11, 2018
343b98c
rpc: Specify chain tip instead of chain in GetDifficulty
promag Jan 11, 2018
b9f226b
rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON
promag Jan 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions src/rest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,12 @@ static bool rest_headers(HTTPRequest* req,
if (!ParseHashStr(hashStr, hash))
return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);

const CBlockIndex* tip = nullptr;
std::vector<const CBlockIndex *> headers;
headers.reserve(count);
{
LOCK(cs_main);
tip = chainActive.Tip();
const CBlockIndex* pindex = LookupBlockIndex(hash);
while (pindex != nullptr && chainActive.Contains(pindex)) {
headers.push_back(pindex);
Expand Down Expand Up @@ -178,11 +180,8 @@ static bool rest_headers(HTTPRequest* req,
}
case RetFormat::JSON: {
UniValue jsonHeaders(UniValue::VARR);
{
LOCK(cs_main);
for (const CBlockIndex *pindex : headers) {
jsonHeaders.push_back(blockheaderToJSON(pindex));
}
for (const CBlockIndex *pindex : headers) {
jsonHeaders.push_back(blockheaderToJSON(tip, pindex));
}
std::string strJSON = jsonHeaders.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
Expand Down Expand Up @@ -210,8 +209,10 @@ static bool rest_block(HTTPRequest* req,

CBlock block;
CBlockIndex* pblockindex = nullptr;
CBlockIndex* tip = nullptr;
{
LOCK(cs_main);
tip = chainActive.Tip();
pblockindex = LookupBlockIndex(hash);
if (!pblockindex) {
return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
Expand Down Expand Up @@ -243,11 +244,7 @@ static bool rest_block(HTTPRequest* req,
}

case RetFormat::JSON: {
UniValue objBlock;
{
LOCK(cs_main);
objBlock = blockToJSON(block, pblockindex, showTxDetails);
}
UniValue objBlock = blockToJSON(block, tip, pblockindex, showTxDetails);
std::string strJSON = objBlock.write() + "\n";
req->WriteHeader("Content-Type", "application/json");
req->WriteReply(HTTP_OK, strJSON);
Expand Down
57 changes: 28 additions & 29 deletions src/rpc/blockchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,7 @@ static CUpdatedBlock latestblock;
*/
double GetDifficulty(const CBlockIndex* blockindex)
{
if (blockindex == nullptr)
{
return 1.0;
}
assert(blockindex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rpc code, assert should be replaced with throw JSONRPCError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a usage error, should be a programatic error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could avoid the assert by passing in a const reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but out of scope here and I'm happy to submit that refactor in a separate PR.


int nShift = (blockindex->nBits >> 24) & 0xff;
double dDiff =
Expand All @@ -81,15 +78,22 @@ double GetDifficulty(const CBlockIndex* blockindex)
return dDiff;
}

UniValue blockheaderToJSON(const CBlockIndex* blockindex)
static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* blockindex, const CBlockIndex*& next)
{
next = tip->GetAncestor(blockindex->nHeight + 1);
if (next && next->pprev == blockindex) {
return tip->nHeight - blockindex->nHeight + 1;
}
next = nullptr;
return blockindex == tip ? 1 : -1;
}

UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex)
{
AssertLockHeld(cs_main);
UniValue result(UniValue::VOBJ);
result.pushKV("hash", blockindex->GetBlockHash().GetHex());
int confirmations = -1;
// Only report confirmations if the block is on the main chain
if (chainActive.Contains(blockindex))
confirmations = chainActive.Height() - blockindex->nHeight + 1;
const CBlockIndex* pnext;
int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext);
result.pushKV("confirmations", confirmations);
result.pushKV("height", blockindex->nHeight);
result.pushKV("version", blockindex->nVersion);
Expand All @@ -105,21 +109,17 @@ UniValue blockheaderToJSON(const CBlockIndex* blockindex)

if (blockindex->pprev)
result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex());
CBlockIndex *pnext = chainActive.Next(blockindex);
if (pnext)
result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex());
return result;
}

UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool txDetails)
UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, bool txDetails)
{
AssertLockHeld(cs_main);
UniValue result(UniValue::VOBJ);
result.pushKV("hash", blockindex->GetBlockHash().GetHex());
int confirmations = -1;
// Only report confirmations if the block is on the main chain
if (chainActive.Contains(blockindex))
confirmations = chainActive.Height() - blockindex->nHeight + 1;
const CBlockIndex* pnext;
int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext);
result.pushKV("confirmations", confirmations);
result.pushKV("strippedsize", (int)::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS));
result.pushKV("size", (int)::GetSerializeSize(block, SER_NETWORK, PROTOCOL_VERSION));
Expand Down Expand Up @@ -151,7 +151,6 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx

if (blockindex->pprev)
result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex());
CBlockIndex *pnext = chainActive.Next(blockindex);
if (pnext)
result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex());
return result;
Expand Down Expand Up @@ -747,7 +746,7 @@ static UniValue getblockheader(const JSONRPCRequest& request)
return strHex;
}

return blockheaderToJSON(pblockindex);
return blockheaderToJSON(chainActive.Tip(), pblockindex);
}

static CBlock GetBlockChecked(const CBlockIndex* pblockindex)
Expand Down Expand Up @@ -848,7 +847,7 @@ static UniValue getblock(const JSONRPCRequest& request)
return strHex;
}

return blockToJSON(block, pblockindex, verbosity >= 2);
return blockToJSON(block, chainActive.Tip(), pblockindex, verbosity >= 2);
}

struct CCoinsStats
Expand Down Expand Up @@ -1121,7 +1120,7 @@ static UniValue verifychain(const JSONRPCRequest& request)
}

/** Implementation of IsSuperMajority with better feedback */
static UniValue SoftForkMajorityDesc(int version, CBlockIndex* pindex, const Consensus::Params& consensusParams)
static UniValue SoftForkMajorityDesc(int version, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
{
UniValue rv(UniValue::VOBJ);
bool activated = false;
Expand All @@ -1141,7 +1140,7 @@ static UniValue SoftForkMajorityDesc(int version, CBlockIndex* pindex, const Con
return rv;
}

static UniValue SoftForkDesc(const std::string &name, int version, CBlockIndex* pindex, const Consensus::Params& consensusParams)
static UniValue SoftForkDesc(const std::string &name, int version, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
{
UniValue rv(UniValue::VOBJ);
rv.pushKV("id", name);
Expand Down Expand Up @@ -1247,20 +1246,21 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)

LOCK(cs_main);

const CBlockIndex* tip = chainActive.Tip();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change these unless you're gonna actually kill the cs_main held for the whole time, I'd think, no (and little reason to, its not like its being held for an extended period)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This moves up tip variable declared below.

UniValue obj(UniValue::VOBJ);
obj.pushKV("chain", Params().NetworkIDString());
obj.pushKV("blocks", (int)chainActive.Height());
obj.pushKV("headers", pindexBestHeader ? pindexBestHeader->nHeight : -1);
obj.pushKV("bestblockhash", chainActive.Tip()->GetBlockHash().GetHex());
obj.pushKV("difficulty", (double)GetDifficulty(chainActive.Tip()));
obj.pushKV("mediantime", (int64_t)chainActive.Tip()->GetMedianTimePast());
obj.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), chainActive.Tip()));
obj.pushKV("bestblockhash", tip->GetBlockHash().GetHex());
obj.pushKV("difficulty", (double)GetDifficulty(tip));
obj.pushKV("mediantime", (int64_t)tip->GetMedianTimePast());
obj.pushKV("verificationprogress", GuessVerificationProgress(Params().TxData(), tip));
obj.pushKV("initialblockdownload", IsInitialBlockDownload());
obj.pushKV("chainwork", chainActive.Tip()->nChainWork.GetHex());
obj.pushKV("chainwork", tip->nChainWork.GetHex());
obj.pushKV("size_on_disk", CalculateCurrentUsage());
obj.pushKV("pruned", fPruneMode);
if (fPruneMode) {
CBlockIndex* block = chainActive.Tip();
const CBlockIndex* block = tip;
assert(block);
while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
block = block->pprev;
Expand All @@ -1277,7 +1277,6 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
}

const Consensus::Params& consensusParams = Params().GetConsensus();
CBlockIndex* tip = chainActive.Tip();
UniValue softforks(UniValue::VARR);
UniValue bip9_softforks(UniValue::VOBJ);
softforks.push_back(SoftForkDesc("bip34", 2, tip, consensusParams));
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/blockchain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ double GetDifficulty(const CBlockIndex* blockindex);
void RPCNotifyBlockChange(bool ibd, const CBlockIndex *);

/** Block description to JSON */
UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool txDetails = false);
UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, bool txDetails = false);

/** Mempool information to JSON */
UniValue mempoolInfoToJSON();
Expand All @@ -36,7 +36,7 @@ UniValue mempoolInfoToJSON();
UniValue mempoolToJSON(bool fVerbose = false);

/** Block header to JSON */
UniValue blockheaderToJSON(const CBlockIndex* blockindex);
UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex);

/** Used by getblockstats to get feerates at different percentiles by weight */
void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES], std::vector<std::pair<CAmount, int64_t>>& scores, int64_t total_weight);
Expand Down
7 changes: 0 additions & 7 deletions src/test/blockchain_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,4 @@ BOOST_AUTO_TEST_CASE(get_difficulty_for_very_high_target)
TestDifficulty(0x12345678, 5913134931067755359633408.0);
}

// Verify that difficulty is 1.0 for an empty chain.
BOOST_AUTO_TEST_CASE(get_difficulty_for_null_tip)
{
double difficulty = GetDifficulty(nullptr);
RejectDifficultyMismatch(difficulty, 1.0);
}

BOOST_AUTO_TEST_SUITE_END()