Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve DNS/hostnames for signer node_host config #4475

Merged
merged 7 commits into from Mar 5, 2024

Conversation

zone117x
Copy link
Member

@zone117x zone117x commented Mar 4, 2024

Fixes #4466

@zone117x zone117x changed the base branch from master to next March 4, 2024 14:35
@zone117x zone117x changed the title fix: resolve DNS/hostnames for signer node_host value fix: resolve DNS/hostnames for signer node_host config Mar 4, 2024
@zone117x
Copy link
Member Author

zone117x commented Mar 4, 2024

Changes work as expected in local e2e testing using hostnames

tacks-signer-1-1  | INFO [1709564711.725348] [stacks-signer/src/runloop.rs:202] [signer_runloop] Signer #1 (ST18MDW2PDTBSCR1ACXYRJP2JX70FWNM6YY2VX4SS) is registered for reward cycle 6.
stacks-signer-1-1  | INFO [1709564711.866834] [stacks-signer/src/signer.rs:1172] [signer_runloop] Signer #1 is the current coordinator for 6 and must trigger DKG. Queuing DKG command...
stacks-signer-1-1  | INFO [1709564711.866859] [stacks-signer/src/runloop.rs:310] [signer_runloop] Runloop successfully initialized!
stacks-signer-1-1  | INFO [1709564711.910008] [stacks-signer/src/signer.rs:263] [signer_runloop] Signer #1: Starting DKG vote, round: 1, cycle: 6
stacks-signer-1-1  | WARN [1709564711.986812] [stacks-signer/src/runloop.rs:268] [signer_runloop] Signer is not registered for reward cycle 5. Waiting for confirmed registration...
stacks-signer-1-1  | INFO [1709564712.033942] [stacks-signer/src/runloop.rs:310] [signer_runloop] Runloop successfully initialized!

@zone117x zone117x marked this pull request as ready for review March 4, 2024 15:06
@zone117x zone117x requested review from jferrant and kantai March 4, 2024 15:09
kantai
kantai previously approved these changes Mar 4, 2024
jferrant
jferrant previously approved these changes Mar 4, 2024
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not NACKing this outright since it might be workable pending some deeper digging, but a decisive advantage of keeping SocketAddr over String for node hosts is that a DNS resolution won't stall the calling thread. Right now, DNS resolution in Rust is synchronous -- the implementation of ToSocketAddrs for (&str, u16) and friends does a blocking call to gethostbyname(3). This can lead to inexplicable delays every time you try to send data to a host named by a DNS hostname instead of an IP address.

Most systems that use DNS avoid this through caching, but I don't know what Rust does here. Does the ToSocketAddrs implementation cache DNS names? If so, for how long? Can the policy be changed by the operator? Where is the policy that enforces this -- for example, is it the host's libc config? Should we cache DNS names? (the Stacks node does this directly by running ToSocketAddrs calls in a separate thread, for example).

@zone117x
Copy link
Member Author

zone117x commented Mar 4, 2024

@jcnelson this is fixing the issue with using DNS at all. Whatever dns caching or similar optimizations should be in a different issue.

@netrome netrome self-requested a review March 4, 2024 16:18
@kantai
Copy link
Member

kantai commented Mar 4, 2024

Most systems that use DNS avoid this through caching, but I don't know what Rust does here. Does the ToSocketAddrs implementation cache DNS names? If so, for how long? Can the policy be changed by the operator? Where is the policy that enforces this -- for example, is it the host's libc config? Should we cache DNS names? (the Stacks node does this directly by running ToSocketAddrs calls in a separate thread, for example).

Rust just uses getaddrinfo in libc -- this seems like actually the "right" thing. Caching is controlled by the OS in this case, and adding a cache on top of that removes that control.

@netrome
Copy link
Contributor

netrome commented Mar 4, 2024

Most systems that use DNS avoid this through caching, but I don't know what Rust does here. Does the ToSocketAddrs implementation cache DNS names? If so, for how long? Can the policy be changed by the operator? Where is the policy that enforces this -- for example, is it the host's libc config? Should we cache DNS names? (the Stacks node does this directly by running ToSocketAddrs calls in a separate thread, for example).

Rust just uses getaddrinfo in libc -- this seems like actually the "right" thing. Caching is controlled by the OS in this case, and adding a cache on top of that removes that control.

Also, if the provided address can be parsed as an IPv4 or IPv6 address - no DNS resolution takes place

From https://doc.rust-lang.org/src/std/net/socket_addr.rs.html#251-268

#[stable(feature = "rust1", since = "1.0.0")]
impl ToSocketAddrs for (&str, u16) {
    type Iter = vec::IntoIter<SocketAddr>;
    fn to_socket_addrs(&self) -> io::Result<vec::IntoIter<SocketAddr>> {
        let (host, port) = *self;

        // try to parse the host as a regular IP address first
        if let Ok(addr) = host.parse::<Ipv4Addr>() {
            let addr = SocketAddrV4::new(addr, port);
            return Ok(vec![SocketAddr::V4(addr)].into_iter());
        }
        if let Ok(addr) = host.parse::<Ipv6Addr>() {
            let addr = SocketAddrV6::new(addr, port, 0, 0);
            return Ok(vec![SocketAddr::V6(addr)].into_iter());
        }

        resolve_socket_addr((host, port).try_into()?)
    }
}

@jcnelson
Copy link
Member

jcnelson commented Mar 4, 2024

@kantai Rust just uses getaddrinfo in libc -- this seems like actually the "right" thing. Caching is controlled by the OS in this case, and adding a cache on top of that removes that control.

@netrome Also, if the provided address can be parsed as an IPv4 or IPv6 address - no DNS resolution takes place

Okay, these two facts are what I needed to be certain of. Approving...

@jcnelson jcnelson self-requested a review March 4, 2024 16:50
libsigner/src/session.rs Show resolved Hide resolved
jcnelson
jcnelson previously approved these changes Mar 4, 2024
@zone117x zone117x dismissed stale reviews from jcnelson, jferrant, and kantai via e9290cb March 4, 2024 17:00
netrome
netrome previously approved these changes Mar 4, 2024
@zone117x zone117x requested review from kantai and jferrant March 4, 2024 17:44
@zone117x zone117x enabled auto-merge March 4, 2024 17:44
libsigner/src/session.rs Outdated Show resolved Hide resolved
@jferrant
Copy link
Collaborator

jferrant commented Mar 4, 2024

I am seeing a lot of build errors mostly due to clones. As soon as those are addressed, will approve.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 60.03%. Comparing base (11d7434) to head (8c1dab8).

Additional details and impacted files
@@             Coverage Diff             @@
##             next    #4475       +/-   ##
===========================================
- Coverage   82.92%   60.03%   -22.90%     
===========================================
  Files         453      453               
  Lines      325610   325579       -31     
  Branches      318      318               
===========================================
- Hits       270010   195447    -74563     
- Misses      55592   130124    +74532     
  Partials        8        8               
Files Coverage Δ
libsigner/src/http.rs 80.70% <100.00%> (-5.85%) ⬇️
libsigner/src/tests/http.rs 68.40% <100.00%> (-28.26%) ⬇️
stacks-signer/src/client/mod.rs 95.19% <100.00%> (-3.91%) ⬇️
stacks-signer/src/client/stackerdb.rs 91.56% <100.00%> (ø)
stacks-signer/src/runloop.rs 82.71% <100.00%> (-8.14%) ⬇️
testnet/stacks-node/src/nakamoto_node/miner.rs 82.10% <100.00%> (-0.10%) ⬇️
...net/stacks-node/src/tests/nakamoto_integrations.rs 84.41% <100.00%> (-11.82%) ⬇️
testnet/stacks-node/src/tests/signer.rs 80.28% <100.00%> (-13.12%) ⬇️
libsigner/src/session.rs 52.21% <75.00%> (-1.24%) ⬇️
stacks-signer/src/cli.rs 9.67% <0.00%> (-24.20%) ⬇️
... and 2 more

... and 291 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d7434...8c1dab8. Read the comment docs.

@zone117x zone117x added this pull request to the merge queue Mar 5, 2024
Merged via the queue into next with commit 38cd24d Mar 5, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stacks-signer cannot resolve hostnames for node_host config
5 participants