Deduplicate some block-to-JSON code #21114

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:getblock-dedup changing 1 files +1 −22

domob1812 commented at 12:07 PM on February 8, 2021: contributor

Some of the logic converting blocks and block headers to JSON for the blockchain RPC methods (getblock, getlockheader) was duplicated. Instead of that, the blockToJSON RPC method now calls blockheaderToJSON first, and then fills in the missing, block-specific bits explicitly.
fanquake added the label RPC/REST/ZMQ on Feb 8, 2021
in src/rpc/blockchain.cpp:160 in 5027330110 outdated
```
 155 | @@ -156,21 +156,14 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
 156 |  
 157 |  UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, bool txDetails)
 158 |  {
 159 | -    // Serialize passed information without accessing chain state of the active chain!
 160 | -    AssertLockNotHeld(cs_main); // For performance reasons
```
MarcoFalke commented at 12:12 PM on February 8, 2021:

why is this removed?

domob1812 commented at 12:36 PM on February 8, 2021:

Because it is also part of blockheaderToJSON, and that is also clear immediately to readers of the code in this case.

But I was thinking about whether or not to remove it myself and was not sure. So if you think it should be there (even though it will be asserted again in blockheaderToJSON), I'm happy to put it back.
domob1812 force-pushed on Feb 8, 2021
0xB10C commented at 11:06 AM on February 9, 2021: member

ACK 1c5474883836bcadbb7f649ef9bf95137d991e49. Tested that the contents of the getblock JSON response of master and this PR are equal (for two testnet blocks; one older and a recent block). The ordering of the JSON fields does change, but I don't think that's a problem.

Reviewers might want to use jq -S to sort the JSON fields alphabetically and then compare the RPC responses. Additionally, mainnet and a broader range of blocks could be tested.
0xB10C commented at 12:26 PM on February 9, 2021: member

Checked the rest/block/<hash>.json interface too as it calls blockToJSON() as well.

Deduplicate some block-to-JSON code.

Some of the logic converting blocks and block headers to JSON for
the blockchain RPC methods (getblock, getlockheader) was duplicated.
Instead of that, the blockToJSON RPC method now calls blockheaderToJSON
first, and then fills in the missing, block-specific bits explicitly.

fa2c521115

in src/rpc/blockchain.cpp:164 in 1c54748838 outdated

 166 | -    int confirmations = ComputeNextBlockAndDepth(tip, blockindex, pnext);
 167 | -    result.pushKV("confirmations", confirmations);
 168 |      result.pushKV("strippedsize", (int)::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS));
 169 |      result.pushKV("size", (int)::GetSerializeSize(block, PROTOCOL_VERSION));
 170 |      result.pushKV("weight", (int)::GetBlockWeight(block));
 171 |      result.pushKV("height", blockindex->nHeight);

laanwj commented at 9:32 PM on February 9, 2021:

It looks like height is also set upstream?

domob1812 commented at 8:28 AM on February 10, 2021:

Yes indeed, good catch! I've removed that line as well now.

domob1812 force-pushed on Feb 10, 2021
laanwj commented at 9:28 AM on February 10, 2021: member

Code review ACK fa2c52111544b0de93ac1002f5395bceeb8fea0e
laanwj merged this on Feb 10, 2021
laanwj closed this on Feb 10, 2021
sidhujag referenced this in commit e48e6638b3 on Feb 11, 2021
domob1812 deleted the branch on Feb 11, 2021
Fabcien referenced this in commit 184ba16e70 on Apr 13, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels