#23706 changed the param names to block_hash and peer_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 blockhash already, and the one other RPC usage has nodeid (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 923312fbf6a89efde1739da0b7209694d4f892ba.
This commit partially reverts 923312fbf6a89efde1739da0b7209694d4f892ba.
ACK f5e008774b5d6070fc661f37b9368dc3729bfb67
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 getblock, which means it may remain inconsistent for a long time.
blockhash and nodeid are easier to type, which is nice.
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.
783@@ -784,8 +784,8 @@ static RPCHelpMan getblockfrompeer()
784 "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
785 "\nReturns an empty JSON object if the request was successfully scheduled.",
786 {
787- {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
788- {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
789+ {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
790+ {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for node IDs)"},
I might be missing something, but the other, existing, RPC is called getpeerinfo and not getnodeinfo. Also, the result is called “peer index”.
0getpeerinfo ... Result:
1[ (json array)
2 { (json object)
3 "id" : n, (numeric) Peer index
Concept ACK
nodeid is an existing param name for disconnectpeer.
So we already have references to both “peer” (getpeerinfo, disconnectpeer command name) and “node” (disconnectpeer argument name) in existing RPC calls. I find “peer” more clear, so it makes sense to me to favor that in new methods.
So I’m inclined to NACK the nodeid part of this.
Also, “peer” is a more precise term, as a peer is a node that is not ours.
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.
s/node/peer/g instead of a global s/peer/node/g?
Usage of “peer” doesn’t necessarily contradict “nodeid” - clearly a peer can have a nodeid, and this concept is expressed in disconnectpeer.
That being said, I suppose with only one other method using nodeid, it wouldn’t be too bad to silently deprecate it and change it to peer_id instead…
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.