addnode RPC call should return success/failure indicator #20552

1. 
   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
2. ajtowns added the label Feature on Dec 3, 2020
3. 
   ajtowns commented at 3:07 am on December 3, 2020: contributor

   cc @amitiuttarwar @jnewbery @jonatack
4. 
   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.

5. 
   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.

   0{
   1  "result": (added/removed/onetry)
   2  "address":
   3  "subversion":
   4  "peer_id":
   5  "error": optional
   6}
   

6. maflcko added this to the milestone 22.0 on Dec 3, 2020
7. 
   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

8. 
   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.

9. 
   naumenkogs commented at 1:45 am on December 4, 2020: member

   Concept ACK.
10. 
    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.

11. maflcko removed this from the milestone 22.0 on Jun 14, 2021
12. 
    willcl-ark commented at 10:26 am on July 3, 2024: member

    Opened #30381 to address this issue.

Contributors
ajtowns amitiuttarwar jonatack jnewbery naumenkogs laanwj willcl-ark

Labels
Feature