RPC: Switch getblockfrompeer back to standard param names blockhash and nodeid #24294
Conversation
…nd nodeid This commit partially reverts 923312f.
There was a problem hiding this comment.
ACK f5e0087
I agree with @luke-jr. I checked that throughout the src/rpc folder and src/rpc/wallet folder, wherever block_hash is used, it is used as a function variable and not as an RPC argument. For the RPC arguments, blockhash is used.
The same goes for peer_id. peer_id is used for function variable, and nodeid is used as RPC argument.
So I think it makes sense to keep RPC argument naming consistent with other RPC functions.
@Sjors, I would like to hear what you think about this?
|
No strong preference. There's something to be said for moving towards more consistent argument naming whenever a new RPC method or argument is introduced. However, it also seems unlikely that we'll change the argument names in ancient RPC calls like
I don't remember if somebody suggested the rename, or if I did it to be consistent with variable names. I'd also like to know @jnewbery, @MarcoFalke and @jonatack's take. |
| {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, | ||
| {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"}, | ||
| {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, | ||
| {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for node IDs)"}, |
There was a problem hiding this comment.
I might be missing something, but the other, existing, RPC is called getpeerinfo and not getnodeinfo. Also, the result is called "peer index".
getpeerinfo ... Result:
[ (json array)
{ (json object)
"id" : n, (numeric) Peer index
|
Concept ACK
|
|
The word "peer" is used throughout the getblockfrompeer help, so if this is changed, the help ought to be updated to be consistent (as well as the RPC to "getblockfromnode"), as well as the developer notes' RPC interface guidelines, which since c26655e in 2017 state that snake case should be used for argument naming. ISTM that internal consistency within a call outweighs how legacy arguments are named in other calls; otherwise, the bar for applying the developer note guideline seems overly high. Also, "peer" is a more precise term, as a peer is a node that is not ours. Tend to NACK the current version of this pull for these reasons. |
|
So we already have references to both "peer" ( So I'm inclined to NACK the
Also in the context of lightning, nodes have an identifier, which is the same wether or not you're connected to them. Bitcoin nodes don't have such an identity, but "node id" might still be confused to refer to such an identity. |
|
At this point it might be a smaller diff (and better) to change the docs (not the args, though) to |
|
Usage of "peer" doesn't necessarily contradict "nodeid" - clearly a peer can have a nodeid, and this concept is expressed in That being said, I suppose with only one other method using |
|
Is this worth keeping open if it's not making it into 23.0? |
|
Closing for now, given that this appears to be controversial and doesn't justify holding back the 23.0 release process at this point. If the changes here are a goal going forward, it would be better to bind the name to the type and introduce finer-grained RPC arg/result types. This way everything will be enforced by the compiler and we don't need to waste review on it. |
|
Looks like this was partially picked up in #24806 again, but overall I think this is low-priority for this experimental RPC that should be used for testing only. Especially given the underdocumentation of it, see #24226 (comment) |
…e blockhash 88917f9 RPC: Switch getblockfrompeer back to standard param name blockhash (Luke Dashjr) Pull request description: This commit partially reverts 923312f. Portion of #24294. ACKs for top commit: MarcoFalke: review ACK 88917f9 ajtowns: ACK 88917f9 jonatack: Review-and-grep-only ACK 88917f9 Tree-SHA512: e42497ea6162623e449c5e60b83a5abbef568f226edc022aa14bbc1f1921618255d593968cf43f7a6d2c0bfd84cdd4b05fbce5c724759b20035e6eead758d443
…ram name blockhash 88917f9 RPC: Switch getblockfrompeer back to standard param name blockhash (Luke Dashjr) Pull request description: This commit partially reverts 923312f. Portion of bitcoin#24294. ACKs for top commit: MarcoFalke: review ACK 88917f9 ajtowns: ACK 88917f9 jonatack: Review-and-grep-only ACK 88917f9 Tree-SHA512: e42497ea6162623e449c5e60b83a5abbef568f226edc022aa14bbc1f1921618255d593968cf43f7a6d2c0bfd84cdd4b05fbce5c724759b20035e6eead758d443
#23706 changed the param names to
block_hashandpeer_id, which in a vacuum seems fine.But this new method is the only place we refer to them by these names. Many other places (including every RPC param) use
blockhashalready, and the one other RPC usage hasnodeid(and there are no other cases where it is called a "peer ID").For this reason, I think we should keep to the standard param names for now.
This commit partially reverts 923312f.