Use the node's configuration to decide if adding a peer with DNS in the enode is allowed

[matthew1001]

PR description

Uses the node configuration to decide if admin_addPeer() should allow enodes with DNS in them. Previously this was hard coded to DNS-disabled.

I had to flow the enode DNS configuration through from RunnerBuilder to the JSON RPC method factory so that AdminAddPeer` has access to it. It may be that this isn't necessary, but I couldn't see another way to do it.

Fixed Issue(s)

Fixes #5583

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

[macfarla]

Looks good overall. one suggestion about dealing with the Optional.
also could do with a changelog entry

[macfarla]

better to pass in the actual value (not optional) and do Optional.of() within this method
similar to rpcMaxLogsRange() above

[matthew1001]

Thanks @macfarla that makes sense. I've made that change under the latest commit, along with an update to the change log.

[macfarla]

Hi @matthew1001 I just built besu from this PR locally and getting a parse error from the JSON-RPC

➜  besu-local-nodes git ✗ curl -s -H 'content-type:application/json' -X POST --data '{"jsonrpc":"2.0","method":"admin_addPeer","params":["enode://be1c3f9aa22ecf9e274040606cc530bb99153997e8dce77a9ea9e85802a0adb80bf78b24d67b0823cf7dca17efdf7f7e5e507b3376224484fa053ac7c2fb8a8b@11.11.11.101:30303"],"id":51}' http://localhost:8545
{
  "jsonrpc" : "2.0",
  "id" : 51,
  "result" : true
}%
➜  besu-local-nodes git ✗ curl -s -H 'content-type:application/json' -X POST --data '{"jsonrpc":"2.0","method":"admin_addPeer","params":["enode://be1c3f9aa22ecf9e274040606cc530bb99153997e8dce77a9ea9e85802a0adb80bf78b24d67b0823cf7dca17efdf7f7e5e507b3376224484fa053ac7c2fb8a8b@bozo.bob.net:30303"],"id":51}' http://localhost:8545
{
  "jsonrpc" : "2.0",
  "id" : 51,
  "error" : {
    "code" : -32700,
    "message" : "Parse error"
  }
}%

but besu log message would lead me to believe it worked ok

2023-06-15 10:52:28.409+10:00 | vert.x-eventloop-thread-2 | DEBUG | JsonRpcHttpService | Opened connection from 127.0.0.1:53113. Total of active connections: 1/160
2023-06-15 10:52:28.411+10:00 | vert.x-worker-thread-3 | DEBUG | JsonRpcExecutor | JSON-RPC request -> admin_addPeer ["enode://be1c3f9aa22ecf9e274040606cc530bb99153997e8dce77a9ea9e85802a0adb80bf78b24d67b0823cf7dca17efdf7f7e5e507b3376224484fa053ac7c2fb8a8b@bozo.bob.net:30303"]
2023-06-15 10:52:28.411+10:00 | vert.x-worker-thread-3 | DEBUG | AdminAddPeer | Adding (enode://be1c3f9aa22ecf9e274040606cc530bb99153997e8dce77a9ea9e85802a0adb80bf78b24d67b0823cf7dca17efdf7f7e5e507b3376224484fa053ac7c2fb8a8b@bozo.bob.net:30303) to peers

Can you look into the Parse error?

[matthew1001]

Hi @macfarla hmm that's odd. Let me look into it and see why it's still returning a parse error.

[matthew1001]

@macfarla I've re-tested locally and with -Xdns-enabled=true set admin_addPeer works correctly for me. Could you check if you had that set and if it's still failing let me know?

I added a unit test for the case where -Xdns-enabled=false as there wasn't one before, and it also made me go back and look at admin_removePeer because I hadn't applied the same logic to that RPC call. The latest commit updates admin_removePeer to follow the same behaviour as admin_addPeer, and also adds a unit test file (really just a port of the test file for admin_addPeer) because there weren't any unit tests for the remove case.

[matthew1001]

I've made the following additional changes to make the JSON/RPC errors clearer:

1. Added a new error string for the case where DNS is disabled but the enode passed in to the RPC call includes a hostname:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -32100,
        "message": "Enode DNS support is disabled"
    }
}

2. Added a new error string for the case where enode DNS is supported, but the hostname couldn't be resolved:

{
    "jsonrpc": "2.0",
    "id": 1,
    "error": {
        "code": -32100,
        "message": "Cannot resolve enode DNS hostname"
    }
}

[macfarla]

couple of nits. I tested it locally and my results match your description. so looking good overall.

[macfarla]

no need to update existing copyright lines, but if you are changing it - it should be Copyright Hyperledger Besu contributors

[matthew1001]

Weirdly I don't actually recall updating this one - must have been when I was adding copyright for the new file. I've reverted it to how it was for now.

[macfarla]

Suggested change

	CANT_USE_PEER_ENODE_DNS(-32100, "Enode DNS support is disabled"),
	DNS_NOT_ENABLED(-32100, "Enode DNS support is disabled"),

[macfarla]

suggestion for a simpler name. also "X not enabled" matches other error codes

[matthew1001]

Thanks, new commit renames the constant and updates the references to it.

[macfarla]

this will need to move to next release 23.4.3

[matthew1001]

I've created a new section in the change-log under the latest commit, and removed this line from the 23.4.2 section.