feat(rpc): Implement remaining addnode functionality (`remove`, evaluate `onetry`)

[oxarbitrage]

In #9604 we introduced the addnode RPC method and implemented support for the add command. Follow-up work remained to complete the interface:

• Implement remove
• Research and decide on onetry (this was hard to implement in Zebra iirc)

Reopened thanks to the discussion in #9555 (comment).

[str4d]

#9555 (comment):

remove should be relatively straightforward; onetry is likely trickier in Zebra (iirc it wasn’t a clean fit with our peer system).

Yep, I just noticed in zcash/integration-tests#1 that you'd replaced onetry with add in the connect_nodes helper.

To help prioritize: do you need onetry as well, or is remove enough?

I only need remove at the moment, and only if/while #10332 is unresolved. (It would still be good to have given that you support add, and the two are complementary commands.)

I think onetry would be useful to have if you can make it work. As-documented, add only adds the node to the list of peers to try periodically connecting to, but has no guarantees on when the next connection attempt will be (in zcashd it was every 2 minutes), which could mean that an RPC test blocks on average for quite some time. By contrast, onetry means that an attempt to connect to the node is made immediately (and if it fails, a second attempt won't be made, unlike with add). So onetry is a better command to have in the RPC tests, but if it's not feasible to implement then add is sufficient as a fallback.

[arya2]

onetry seems unnecessary for Zebra if we set a higher floor on how many peers Zebra should have and how frequently it should attempt outbound connections (dialing addresses is pretty cheap).

Otherwise, removing peers and overloading the peer set with requests could cause its poll_ready() method to request more peers from the crawl and dial task.

remove will probably mean banning the peer address to prevent reconnections.

It might be nice to create a new peer set request type instead of adding another variant for removing peers that's only used in the peer set and unreachable elsewhere.

[str4d]

remove will probably mean banning the peer address to prevent reconnections.

Hmm, reading this, I realised I was misreading remove. I don't think it actually would work for the RPC tests, because I think it doesn't actually remove an existing connection or prevent an inbound connection from being made, it just prevents a new outbound connection being attempted if the peer is being disconnected. This is further evidenced by the split_network RPC test helper working by shutting down and restarting the nodes.

Given that, I don't think remove helps me at all, so it's no longer a priority for me (instead the regtest block propagation issue is the priority).