fix: Discovery not taking nat manager port-mapping into account

[daanporon]

PR description

Proposal to fix the issue i reported in #6573

• PingPacketData should take into account the NATManager portMapping for the discoveryPort.
• Take into account the udpPort from the PingPacketData if the host is also taken from there.

Fixed Issue(s)

fixes: https://github.com/hyperledger/besu/issues/6573

[github-actions[bot]]

• [ ] 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
• [ ] I thought about running CI.
• [ ] If I did not run CI, I ran as much locally as possible before pushing.

[daanporon]

This is my first PR and i haven't tested it yet .. at the moment it's more a proposal on how i think it can be fixed. If it makes sense i can try to finish the PR :)

[daanporon]

This PR is ready for review. Only had one question. I saw that even when discovery is disabled we are still starting the peerDiscoveryAgent, does this make sense?

I noticed, because in the tests enodeUrlShouldHaveAdvertisedHostWhenDiscoveryDisabled discovery is disabled but still the enodeUrl returned a discport because the discoveryAgent was started and so we fetched the discoveryPort from the NatManager. The NatManager was enabled because on the CI we are running in Docker and by default natMode == AUTO.

[matthew1001]

@daanporon this seems like a reasonable change.

Re. the PeerDiscoveryAgent always starting, I'm not sure I'm afraid. It may be that disabling discovery doesn't prevent "outbound" UDP-based discovery via bootnodes, in which case I guess the agent is still needed?

[daanporon]

@daanporon this seems like a reasonable change.

Re. the PeerDiscoveryAgent always starting, I'm not sure I'm afraid. It may be that disabling discovery doesn't prevent "outbound" UDP-based discovery via bootnodes, in which case I guess the agent is still needed?

@matthew1001 is something else needed in order to get this approved?

About the PeerDiscoveryAgent i'm not familiar enough with the whole architecture. It was just a consideration i had while looking at the tests and the code. I tried to make my code changes in such a way that it only gets the PortMapping when needed, when the local discoveryPort != 0. This way the thing with the Docker NatMethod was not causing an issue anymore ... so it works the way it worked before. But i still chose to set the NatMethod to NONE, just for clarity.

[matthew1001]

@matthew1001 is something else needed in order to get this approved?

It looks OK to me, but I think it would be sensible to get @pinges to take a look and do the actual approval.

[daanporon]

These changes look good to me. @daanporon what scenarios have you tested that in? What kind of nodes have you started to test this?

@pinges I have only run the tests to see if it works and started a local node using: https://wiki.hyperledger.org/display/BESU/Building+from+source#running-developer-builds, but this doesn't use the NAT manager.

If possible it would be good if i can tests this in my k8s environment. Any idea what would be the easiest to build this and push it to a container registy so that i can easily test this?

[macfarla]

@daanporon develop builds (updated every push to main) are here https://github.com/hyperledger/besu/releases/tag/develop - can you work with that?

[daanporon]

@daanporon develop builds (updated every push to main) are here https://github.com/hyperledger/besu/releases/tag/develop - can you work with that?

but that will only work once it is merged in main i think? but i will try to trigger the workflow in my own repo and push it to a container registry i own so that i can test it.

[daanporon]

I have been testing this on AWS and Azure and the change is working as expected. I still have other issues with discovery on kubernetes but at least this works already.