feat: allow hostnames where we take addresses

[Davidson-Souza]

Closes #284

Now you can pass a hostname (i.e.: localhost, node, vps) for addresses in configs like rpc-address and electrum-address. The format is [:port].

One question is whether we should just pick the default one or to crash if we can't resolve the name. I'm leaning towards crashing, at least for the binding addresses.

[jaoleal]

All tests passed locally on my machine.

That can be a dumb question but worth to make it... This feature wouldnt fit better inside floresta-wire ?

One question is whether we should just pick the default one or to crash if we can't resolve the name.

Maybe report the error and connect back on the default ones would be better or, instead, you could leave this as to-do until we fix our logs

[Davidson-Souza]

That can be a dumb question but worth to make it... This feature wouldnt fit better inside floresta-wire ?

This is actually a good idea. At least for the second commit (the --connect part). Pushed 1348a82 with this change. Now you can pass a hostname to UtreexoNode and we will resolve it.

[Davidson-Souza]

1d0cc9e just improves some formatting in the test

[jaoleal]

I think 'resolve_connect_host' should have some docstrings. besides it LGMT

[Davidson-Souza]

After a little fight with CI, 9ab9919 adds docstrings for both new methods

[lucasbalieiro]

Hi, @Davidson-Souza,

Thanks for your hard work on this PR. Here's my feedback based on running the build process and tests:

Environment Details

I tested the build on my machine with the following setup:

• OS:

  $ lsb_release -a
  No LSB modules are available.
  Distributor ID:	Ubuntu
  Description:	Ubuntu 22.04.5 LTS
  Release:	22.04
  Codename:	jammy
  

• Rust Version:

  $ rustc --version
  rustc 1.82.0 (f6e511eec 2024-10-15)
  

Build Results

The build completed successfully. However, I noticed a minor warning:

warning: function `create_false_acc` is never used
   --> crates/floresta-wire/src/p2p_wire/tests/utils.rs:191:8
    |
191 | pub fn create_false_acc(tip: usize) -> Vec<u8> {
    |        ^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

This function doesn’t appear to be used elsewhere. While not blocking for this PR, it might be worth addressing in a follow-up to ensure the codebase remains clean. Potential actions include:

• Removing the function if it’s no longer needed.
• Documenting its intended use if it’s planned for future implementation.
• Suppressing the warning with #[allow(dead_code)] if it’s intentionally kept for specific scenarios.

I tracked this function through the commit history and found it only in its creation.
Commands used:
git log -S 'create_false_acc' --source -p and git log --grep 'create_false_acc'.

Additionally, this warning does not appear when building with the Dockerfile. This may be due to different build flags or the absence of certain warnings in the Docker environment.

Tests

I ran the test suite using cargo test --release, and all tests passed successfully. Great job on ensuring test coverage!

Docker

The Docker image built without issues using the provided Dockerfile.

About the code

Just left some comments in some parts, just really minor, things that could be improved, for me. It look already great!

Let me know if there’s anything specific you’d like me to verify or revisit.

[lucasbalieiro]

The test coverage looks solid, but we could enhance it by including additional edge cases for better robustness. Specifically, consider adding tests for:

• Addresses with leading/trailing spaces: e.g., " 127.0.0.1:8333 " or " example.com:8333 ".
• Boundary port values: the minimum (0) and maximum (65535) valid port numbers.
• Empty string or None as input: to ensure proper handling of invalid or missing addresses.

To reduce code duplication and make it easier to add new test cases in the future, a helper function like the one below can streamline the assertions:

fn check_address_resolving(
    address: &str,
    port: u16,
    should_succeed: bool,
    description: &str,
) {
    let result = UtreexoNode::<RunningNode, PartialChainState>::resolve_connect_host(address, port);
    if should_succeed {
        assert!(result.is_ok(), "Failed: {}", description);
    } else {
        assert!(result.is_err(), "Unexpected success: {}", description);
    }
}

This helper function encapsulates the repetitive logic, making individual test cases clearer and more concise. For example:

check_address_resolving("[::1]", 8333, true, "Valid IPv6 without port");
check_address_resolving("[::1", 8333, false, "Invalid IPv6 format");
check_address_resolving("[::1]:8333", 8333, true, "Valid IPv6 with port");

Suggested Test Module

Here’s how the updated test module could look with the enhancements:

#[cfg(test)]
mod tests {
    use floresta_chain::pruned_utreexo::partial_chain::PartialChainState;
    use crate::node::UtreexoNode;
    use crate::running_node::RunningNode;

    fn check_address_resolving(
        address: &str,
        port: u16,
        should_succeed: bool,
        description: &str,
    ) {
        let result = UtreexoNode::<RunningNode, PartialChainState>::resolve_connect_host(address, port);
        if should_succeed {
            assert!(result.is_ok(), "Failed: {}", description);
        } else {
            assert!(result.is_err(), "Unexpected success: {}", description);
        }
    }

    #[test]
    fn test_parse_address() {
        // IPv6 Tests
        check_address_resolving("[::1]", 8333, true, "Valid IPv6 without port");
        check_address_resolving("[::1", 8333, false, "Invalid IPv6 format");
        check_address_resolving("[::1]:8333", 8333, true, "Valid IPv6 with port");
        check_address_resolving("[::1]:8333:8333", 8333, false, "Invalid IPv6 with multiple ports");

        // IPv4 Tests
        check_address_resolving("127.0.0.1", 8333, true, "Valid IPv4 without port");
        check_address_resolving("321.321.321.321", 8333, false, "Invalid IPv4 format");
        check_address_resolving("127.0.0.1:8333", 8333, true, "Valid IPv4 with port");
        check_address_resolving("127.0.0.1:8333:8333", 8333, false, "Invalid IPv4 with multiple ports");

        // Hostname Tests
        check_address_resolving("example.com", 8333, true, "Valid hostname without port");
        check_address_resolving("example", 8333, false, "Invalid hostname");
        check_address_resolving("example.com:8333", 8333, true, "Valid hostname with port");
        check_address_resolving("example.com:8333:8333", 8333, false, "Invalid hostname with multiple ports");

        // Edge Cases
        check_address_resolving("", 8333, false, "Empty string address");
        check_address_resolving(" 127.0.0.1:8333 ", 8333, false, "Address with leading/trailing spaces");
        check_address_resolving("127.0.0.1:0", 0, true, "Valid address with port 0");
        check_address_resolving("127.0.0.1:65535", 65535, true, "Valid address with maximum port");
    }
}

[Davidson-Souza]

Hi @lucasbalieiro. Thanks for your review!

Pushed 584154d applying your comments. Only the unused function that I didn't remove because it's part of an ongoing project (#214 and #180).

There's a little api breakage, but I think it's ok. I didn't promise stable api yet 😂

[Davidson-Souza]

This is funny. Do windows accept empty strings when resolving hostnames???

[lucasbalieiro]

@Davidson-Souza, this is indeed odd. We could potentially address the issue by adding a conditional target. However, if we have the time, I can spin up a Windows VM and take a closer look at the root cause to ensure we're solving it properly.

Alternatively, we could just remove the test for now, haha.

#[cfg(target_os = "windows")]
check_address_resolving("", 8333, true, "Empty string resolves on Windows");
#[cfg(not(target_os = "windows"))]
check_address_resolving("", 8333, false, "Empty string fails on other platforms");

Let me know how you’d like to proceed!

[Davidson-Souza]

609e43d Feature-gated the test and added one extra small edge case with out-of-range port

[lucasbalieiro]

@Davidson-Souza , Nice. Thanks for your pacience.

Everything looks nice to me.

Great Work!