rpc: Lock cs_main in blockToJSON/blockheaderToJSON #11618

blockToJSON(...) and blockheaderToJSON(...) read the variable chainActive which requires holding the mutex cs_main. So does GetDifficulty(...).

Assert lock is held in those functions?

@promag Feedback addressed! :-)

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

@TheBlueMatt Good point. Locking now moved down one level into blockToJSON/blockheaderToJSON. Looks good?

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?

@TheBlueMatt Commit message and PR title adjusted.

@promag gah, indeed, missed that its called in a loop there - should indeed lock outside the loop.

@TheBlueMatt @promag Updated! Please re-review :-)

utACK 9dda6900.

utACK 9dda690028c719ae074802933f76ef7c45f6dbb6

Indentation fixed. Please re-review :-)

re-utACK a9b6ba0.

utACK a9b6ba0b7