addnode RPC call should return success/failure indicator #20552

issue ajtowns opened this issue on December 3, 2020

ajtowns commented at 3:06 AM on December 3, 2020: contributor

Calling bitcoin-cli addnode i-am-not-an-address onetry fails immediately and permanently, but does not give any indication that happened. Having addnode .. onetry instead block and return either an error or indicate the peer id of the new connection would probably be better. See also #19315#pullrequestreview-528084558
ajtowns added the label Feature on Dec 3, 2020
ajtowns commented at 3:07 AM on December 3, 2020: contributor

cc @amitiuttarwar @jnewbery @jonatack
amitiuttarwar commented at 3:26 AM on December 3, 2020: contributor

thanks AJ!

As mentioned in that conversation thread, I've started a branch to incorporate. The branch builds on 19315, but the feature doesn't need to. Happy for someone to pick this up if interested, otherwise I will see it through after 19315.
jonatack commented at 9:06 AM on December 3, 2020: contributor
I agree. It would be good for addnode to return a JSON object with either an error description or info.
```
{
  "result": (added/removed/onetry)
  "address":
  "subversion":
  "peer_id":
  "error": optional
}
```
maflcko added this to the milestone 22.0 on Dec 3, 2020
jonatack commented at 9:16 AM on December 3, 2020: contributor
- the help could use some updating, e.g. node: The node (see getpeerinfo for nodes) isn't very clear about what it accepts
- case insensitivity for add/remove/onetry would be a nice touch (if others agree); we do this in other RPCs (sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, bumpfee) for estimate_mode...I think it's a reasonable convenience for params that require the user to enter a string word to select an option
jnewbery commented at 10:31 AM on December 3, 2020: contributor

case insensitivity for add/remove/onetry would be a nice touch

I strongly disagree with this. There should be a canonical capitalization for all rpc arguments.
naumenkogs commented at 1:45 AM on December 4, 2020: member

Concept ACK.
laanwj commented at 10:52 AM on June 14, 2021: member

Not much time for 20.0. Should we change the milestone?

I strongly disagree with this. There should be a canonical capitalization for all rpc arguments.

I agree with this; having malleability in the protocol adds complication. Most straightforward is to treat enums as fixed byte sequences.

Also, case insensitivity is a complex issue in the general unicode case. Sure, RPC arguments are all ASCII but it's one of those seemingly trivial requirements that can inadvertently drag in a ton of dependencies.
maflcko removed this from the milestone 22.0 on Jun 14, 2021
willcl-ark commented at 10:26 AM on July 3, 2024: member

Opened #30381 to address this issue.

Contributors

Labels

Linked (view graph)

#19315 [tests] Allow outbound & block-relay-only connections in functional tests. #30381 [WIP] net: return result from addnode RPC