[RPC] decodeblock #14191

pull instagibbs wants to merge 4 commits into bitcoin:master from instagibbs:decodeblock changing 2 files +127 −11

instagibbs commented at 6:28 pm on September 10, 2018: member

I find this useful for looking at not-PoW-valid blocks rather than using an external parser.
allow blockToJSON to have null blockindex 7fdfffbe0b
blockToJSON less dependent on blockindex since block is present 6cab6f89e6
add decodeblock rpc 6fea07f283
add getblock/decodeblock verbosity checks 1aecb69d7f
instagibbs force-pushed on Sep 10, 2018
DrahtBot commented at 7:49 pm on September 10, 2018: member
- #13558 (Drop unused GetType() from CSizeComputer by Empact)
- #12153 (Avoid permanent cs_main lock in getblockheader by promag)
- #12151 (rpc: Remove cs_main lock from blockToJSON and blockheaderToJSON by promag)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
fanquake added the label RPC/REST/ZMQ on Sep 10, 2018
laanwj commented at 6:50 am on September 11, 2018: member

From a performance perspective using RPC for utility-only functions that could be performed locally, especially something that requires pumping over megabytes of data, is a bad idea.

I can imagine this is useful as a quick hack but it’s not something to encourage.
DrahtBot commented at 8:38 am on September 11, 2018: member
DrahtBot added the label Needs rebase on Sep 11, 2018
Sjors commented at 2:55 pm on September 11, 2018: member

What about expanding / renaming getblock such that it can take either a hash or a full block hex?
promag commented at 3:06 pm on September 11, 2018: member

@Sjors please no overloading, especially when the purposes are different.

in src/rpc/blockchain.cpp:118 in 1aecb69d7f

114@@ -115,16 +115,18 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool tx
115 {
116     AssertLockHeld(cs_main);
117     UniValue result(UniValue::VOBJ);
118-    result.pushKV("hash", blockindex->GetBlockHash().GetHex());
119+    result.pushKV("hash", block.GetHash().GetHex());

promag commented at 4:32 pm on September 11, 2018:

There is a performance penalty in not using blockindex->GetBlockHash(). Suggestion:

0result.pushKV("hash", blockindex ? blockindex->GetBlockHash().GetHex() : block.GetHash().GetHex());

in src/rpc/blockchain.cpp:834 in 1aecb69d7f

829+    if (!request.params[1].isNull()) {
830+        verbosity = request.params[1].get_int();
831+    }
832+
833+    return blockToJSON(block, nullptr, verbosity >= 2);
834+

promag commented at 4:36 pm on September 11, 2018:

Nit, remove empty line.

in src/rpc/blockchain.cpp:816 in 1aecb69d7f

811+            "\nExamples:\n"
812+            + HelpExampleCli("decodeblock", "\"<block hex>\"")
813+            + HelpExampleRpc("decodeblock", "\"<block hex>\"")
814+        );
815+
816+    LOCK(cs_main);

promag commented at 4:37 pm on September 11, 2018:

Could lock after DecodeHexBlk below.

promag commented at 4:40 pm on September 11, 2018: member

Agree with @laanwj above, this is a good candidate for a local tool.
laanwj commented at 7:53 am on September 12, 2018: member

What about expanding / renaming getblock such that it can take either a hash or a full block hex?

Disagree, if we end up adding this, then adding it as a separate “pure utility” (as in “this function could run without node”) RPC call with disparate functionality is best. I think coaxing it into an existing server function unnecessarily complicates the interface.
sdaftuar commented at 2:37 pm on September 14, 2018: member

I’m a -0; I agree with the sentiment that this should be done in a standalone tool rather than part of bitcoind.
instagibbs closed this on Sep 14, 2018
laanwj removed the label Needs rebase on Oct 24, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors
instagibbs DrahtBot laanwj Sjors promag sdaftuar

Labels
RPC/REST/ZMQ