Add auto-detection of node.sock location for Bitcoin Core IPC

[xyephy]

• Add BitcoinNetwork enum and resolve_ipc_socket_path() to stratum-apps
• Make unix_socket_path optional, add network and data_dir fields
• Update Pool and JDC to use the resolver
• Update config examples to use new format

Closes #118

[GitGab19]

Given we're touching the config of our apps, we have to update the config templates we have at https://github.com/stratum-mining/sv2-apps/tree/main/docker/config, otherwise docker deployments are not gonna work.

About this, I think we should enforce this kind of check on our CI, by trying to run the docker images with the config templates that we have on the repo.

Wdyt @lucasbalieiro ?

[lucasbalieiro]

Given we're touching the config of our apps, we have to update the config templates we have at https://github.com/stratum-mining/sv2-apps/tree/main/docker/config, otherwise docker deployments are not gonna work.

About this, I think we should enforce this kind of check on our CI, by trying to run the docker images with the config templates that we have on the repo.

Wdyt @lucasbalieiro ?

Good idea. PR #129 also touches the config files, and I’d probably only notice this kind of issue once we’re at release time., bad timing.

I think we can catch this earlier. Not sure whether we can run a Docker image inside the GitHub runner, but if not, we can still validate it with a simple bash check.

I opened #137 so I can tackle this soon-ish.

[plebhash]

node.sock will always be inside datadir, be it default or custom

so it feels redundant to have both data_dir AND unix_socket_path as optional features, and we also unnecessarily open the door to edge cases that have to be treated at runtime

at this point, I would stick with data_dir, and infer unix_socket_path by appending node.sock to the Path/PathBuf before feeding it to BitcoinCoreSv2Config

[plebhash]

TLDR user doesn't need to ever explicitly provide unix_socket_path

they should always provide network, and optionally provide data_dir in case they're using a custom path

then the path to node.sock is deterministically inferred at runtime, with no room for edge cases

[plebhash]

anyways, it's easy to leave blindspots when thinking about this

so @xyephy please do a sanity check on the rationale I proposed here and let me know if you find some flaws

[xyephy]

let me test it then I will give you feedback, thank you.

[GitGab19]

Any updates on this @xyephy ?

[Shourya742]

Do we need socket_path anymore? Considering we have datadir and network, with us?

[xyephy]

yes we still need it for Docker use cases where the socket is mounted at a flat path
like /app/node.sock (not following the {data_dir}/{network_subdir}/node.sock pattern).

[plebhash]

@lucasbalieiro would it be possible to find an alternative approach on the Docker side?

something that doesn't force these decisions on the .toml config interface?

[Shourya742]

We can mount the file into the container at any path we choose, creating directories inside the container as needed.

[lucasbalieiro]

@lucasbalieiro would it be possible to find an alternative approach on the Docker side?

something that doesn't force these decisions on the .toml config interface?

yes, @Shourya742 is right.

Inside the container, I can place node.sock in a mocked Bitcoin directory (for example), which exists only within the container.

That way, the apps always know exactly where to find the file.
I’ll propose a concrete approach once I start working on this issue: #137

[lucasbalieiro]

I don’t remember exactly why I put it under app/node.sock, probably because it was easier to debug at the time. There’s nothing mandatory about that location, though.

[plebhash]

but if we merge this PR, removing unix_socket_path, without doing anything about the Docker files

will we end up with a broken Docker setup on main?

[lucasbalieiro]

but if we merge this PR, removing unix_socket_path, without doing anything about the Docker files

will we end up with a broken Docker setup on main?

Not exactly right now, because in the docker compose we are pointing to the latest release v0.1.0:

sv2-apps/docker/docker-compose.yml

Lines 2 to 4 in 39188c7

	pool_sv2:
	image: stratumv2/pool_sv2:v0.1.0
	container_name: pool_sv2

So it is expecting the config templates to be exactly as it was at that version

But it will break for the next release, that is one of the reasons for the #137.

[Shourya742]

Suggested change

	) -> Result<PathBuf, String> {
	) -> Result<PathBuf, io::Error> {

Shouldn't this be an IO Error?

[Shourya742]

I would just return an option for this method, something like this:

And during the CLI parsing itself, we can check if this is possible or not. If it isn't we won't even proceed with bootstrapping of application.

pub fn resolve_ipc_socket_path(
    unix_socket_path: Option<PathBuf>,
    network: Option<&BitcoinNetwork>,
    data_dir: Option<PathBuf>,
) -> Option<PathBuf> {
    unix_socket_path.or_else(|| {
        let network = network?;
        let base_dir = data_dir.or_else(default_bitcoin_data_dir)?;

        Some(match network.subdir() {
            Some(subdir) => base_dir.join(subdir).join("node.sock"),
            None => base_dir.join("node.sock"),
        })
    })
}

[xyephy]

Shouldn't this be an IO Error?

these are config validation errors, not I/O errors

[xyephy]

I would just return an option for this method, something like this:

And during the CLI parsing itself, we can check if this is possible or not. If it isn't we won't even proceed with bootstrapping of application.

pub fn resolve_ipc_socket_path(
    unix_socket_path: Option<PathBuf>,
    network: Option<&BitcoinNetwork>,
    data_dir: Option<PathBuf>,
) -> Option<PathBuf> {
    unix_socket_path.or_else(|| {
        let network = network?;
        let base_dir = data_dir.or_else(default_bitcoin_data_dir)?;

        Some(match network.subdir() {
            Some(subdir) => base_dir.join(subdir).join("node.sock"),
            None => base_dir.join("node.sock"),
        })
    })
}

Thank you for this suggestion, looks much cleaner.

[Shourya742]

ideally we should have a check if the file exist or not. That was the reason I suggested for IOError initially.