Skip to content
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] decodeblock #14191

Closed
wants to merge 4 commits into from
Closed

[RPC] decodeblock #14191

wants to merge 4 commits into from

Conversation

instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 10, 2018

I find this useful for looking at not-PoW-valid blocks rather than using an external parser.

@DrahtBot
Copy link
Contributor

Note to reviewers: This pull request conflicts with the following ones:

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.

@laanwj
Copy link
Member

laanwj commented Sep 11, 2018

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
Copy link
Contributor

Needs rebase

@Sjors
Copy link
Member

Sjors commented Sep 11, 2018

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

@promag
Copy link
Member

promag commented Sep 11, 2018

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

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Agree with @laanwj above, this is a good candidate for a local tool.

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

Choose a reason for hiding this comment

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

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

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

}

return blockToJSON(block, nullptr, verbosity >= 2);

Copy link
Member

Choose a reason for hiding this comment

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

Nit, remove empty line.

+ HelpExampleRpc("decodeblock", "\"<block hex>\"")
);

LOCK(cs_main);
Copy link
Member

Choose a reason for hiding this comment

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

Could lock after DecodeHexBlk below.

@laanwj
Copy link
Member

laanwj commented Sep 12, 2018

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
Copy link
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 instagibbs closed this Sep 14, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants