It would make more sense to use the following code as it is uniform with the codebase but it breaks compatibility because passing a hash would require wrapping it between quotes.

#19949 fixes this and should probably be merged first?

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 082e1a76a..cb171f27b 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -642,7 +642,7 @@ static RPCHelpMan getblock()
                 "If verbosity is 2, returns an Object with information about block <hash> and information about each transaction.\n"
                 "If verbosity is 3, returns an Object with information about block <hash> and information about each transaction, including prevout information for inputs (only for unpruned blocks in the current best chain).\n",
                 {
-                    {"blockhash|blockheight", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash or height", RPCArgOptions{.type_str={"", "string or numeric"}}},
+                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::NO, "The block hash or height", RPCArgOptions{.type_str={"", "string or numeric"}}},
                     {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{1}, "0 for hex-encoded data, 1 for a JSON object, 2 for JSON object with transaction data, and 3 for JSON object with transaction data including prevout information for inputs"},
                 },
                 {
@@ -706,18 +706,6 @@ static RPCHelpMan getblock()
 {
     uint256 hash;
     ChainstateManager& chainman = EnsureAnyChainman(request.context);
-    if(request.params[0].get_str().length() == 64){
-        hash = *ParseHashOrHeight(request.params[0], chainman)->phashBlock;
-    }
-    else{
-        uint64_t blockHeight;
-        if(ParseUInt64(request.params[0].get_str(),&blockHeight)){
-            hash = *ParseHashOrHeight(blockHeight, chainman)->phashBlock;
-        }
-        else{
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block height invalid");
-        }
-    }
 
     int verbosity = 1;
     if (!request.params[1].isNull()) {
@@ -729,17 +717,11 @@ static RPCHelpMan getblock()
     }
 
     CBlock block;
-    const CBlockIndex* pblockindex;
     const CBlockIndex* tip;
+    const auto* pblockindex{ParseHashOrHeight(request.params[0], chainman)};
     {
         LOCK(cs_main);
-        pblockindex = chainman.m_blockman.LookupBlockIndex(hash);
         tip = chainman.ActiveChain().Tip();
-
-        if (!pblockindex) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
-        }
-
         block = GetBlockChecked(chainman.m_blockman, pblockindex);
     }
 
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index 8688263ef..4273063da 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -96,6 +96,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listunspent", 2, "addresses" },
     { "listunspent", 3, "include_unsafe" },
     { "listunspent", 4, "query_options" },
+    { "getblock", 0, "hash_or_height" },
     { "getblock", 1, "verbosity" },
     { "getblock", 1, "verbose" },
     { "getblockheader", 1, "verbose" },

It would make more sense to use the following code as it is uniform with the codebase but it breaks compatibility because passing a hash would require wrapping it between quotes.

The below code is more uniform agreed. The point of this was that this a fully backward compatible drop in implementation with a minimal impact on the codebase. There is an advantage to this method is that it becomes possible to throw not only on invalid hash but also independently on invalid height input.

Edit:
Commit chain fixed thanks @sipa

Applied clang formatting to @aureleoules request to make the code more uniform.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26039 (refactor: Run type check against RPCArgs (1/2) by MarcoFalke)

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.

It is possible to run the test locally. This way you don't have to push and wait for CI.

#19949 fixes this and should probably be merged first?

I agree, #19949 doesn't take getblock into account, I don't recall why, probably an oversight. This PR here could be a follow-up building on top of it implementing the needed further changes for getblock.

Concept NACK. See also more reasonable (yet still rejected) proposals #8457, #16439, #14858, #16345, #16317

Concept NACK. See also more reasonable (yet still rejected) proposals #8457, #16439, #14858, #16345, #16317

These are my thoughts about those particular listed PRs. I would disagree that they are all 'more reasonable' but none the less here are my thoughts.

Just curious as to why you would say an implementation such as #16317 is more reasonable since it clearely violates multiple suggestions outlined in the developer notes including the use of c_str and and atoi instead of parse ParseInt32. Then you have #14858. This solution introduces a whole new include for regex expressions and again uses stoi() and defeats the purpose of the guidelines calling for a ParseInt(n)

#16345 sure could be considered more reasonable since it adds a whole new RPC call as well as #16439 since it's very different solution and removes the ambiguity of using height as an argument. Also effects multiple RPCs. Both of these PRs actually create a new way defining the argument or create a new RPCs

Lastly #8457. This solution is quite close to the one in this PR. Why it's more reasonable other than it has a throw for invalid height and height out of range.

More reasonable mainly in the sense of using a Number parameter rather than a String.

But regardless, the main issue is the concept.

I like @promag's argument here --> #8457 (comment)

    > I don't like overloading the argument, making it ambiguous.

Disagree, there is a place for overloading in APIs. In this case blocks are usually referenced by height or hash.

There is also overloading in the protocol: 4 lock_time uint32_t A time (Unix epoch time) or block number. See the locktime parsing rules.

It seems messy, and we don't do that anywhere else on the API.

From help importaddress: 1. "script" (string, required) The hex-encoded script (or address) either.

E.g. a long hash with no hexadecimal characters could be interpreted as a long number.

Unlikely? Still it's simple to distinguish that block hash from a number.

Though even then I'm not convinced it's a worthwhile addition.

From the API point of view, I think it's fair to fetch a block from it's height. From a technical point of view, making 2 calls causing locks on cs_main etc on a heavy loaded daemon is resource wasting.

So, +1 for getblock hash|height (verbose).

Originally posted by @promag in #8457 (comment)

Per the ambiguity argument in #8457

E.g. a long hash with no hexadecimal characters could be interpreted as a long number.

Unlikely? Still it's simple to distinguish that block hash from a number.

This is currently not even possible with the current implementation .23. This is because any attempt to input a block hash that is more or less than 64 characters will be met with an error -8, blockhash must be of length 64

Example getblock "001" doesn't attempt to get the hex padded version of 0x01 instead the user is presented with blockhash must be of length 64 (not 3, for '001')

This is because of the behavior implemented in ParseHashV.

The implementation of parsing the hash in getblock that was a part of #8457 was just a call of uint256 hash(uint256S(strHash)) as string input that was less than 64 chars would be parsed into a hash. example in #8457 if the user called getblock "001" it would parse that as `bitcoin-cli getblock "000000000000000000000000000000000000000000000000000000000000001"

Since the implementation of ParseHashV into getblock this is no longer possible because of the check for a length == 64 followed by a throw if that case is not met. As such the ambiguity arguments of a collision might need to be looked at again.

To the concern about the input for a block height not being a number.

More reasonable mainly in the sense of using a Number parameter rather than a String.

So if the block hash could be input as a HEX_STR and a height could be parsed as NUM then the idea could potentially become reasonable?

More reasonable mainly in the sense of using a Number parameter rather than a String.

But regardless, the main issue is the concept.

@luke-jr a solution to the problem is just to create a switch that cases over the result of UNIVALUE::VType of request.params[0] my most recent commit supports not only string inputs of hash or height for getblock but an integer input as well. All while maintaining complete backwards compatibility.

So at minimum I hope these changes would class this solution into the same league as the 'more reasonable' ones.

Edit... taking it even further if it was desired any integer input could be parsed as a height where as any string could be processed as a hash.

🐙 This pull request conflicts with the target branch and needs rebase.

Concept NACK

After further consideration:

This change sets up the condition that getblock has to
evaluate inputs:

getblock <hash> <int verbosity>

or

getblock <int> <int verbosity>

This seems error prone to me!

Using a nested command such as:

 bitcoin-cli getblock $(bitcoin-cli getblockhash 0)
 bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 0
 bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 1
 bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 2

forces the user to handle the data query with intent.

The command is currently simple to use!

The added convenience of over loading this command doesn't out weigh potential problems it could create. ^{1 2}

Additionally:

The getblock help output is simple to understand as is.

Arguments:
1. blockhash    (string, required) The block hash
2. verbosity    (numeric, optional, default=1) 0 for hex-encoded data, 1 for a JSON object, 2 for JSON object with transaction data, and 3 for JSON object with transaction data including prevout information for inputs

This was meant to be closed some time ago.

Though I don't agree completely in the case of a hash vs height overload since height and hashes do not and can not intersect ever. With that said, there are already other rpc calls that use this same method to get the block with a parseHashOrHeight

All and all, thanks for your input.