Skip to content
New issue

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

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

Already on GitHub? # to your account

change bitcoind -> corepc_node #89

Merged
merged 4 commits into from
Jan 22, 2025
Merged

change bitcoind -> corepc_node #89

merged 4 commits into from
Jan 22, 2025

Conversation

RCasatta
Copy link
Owner

@RCasatta RCasatta commented Jan 20, 2025

#88

@RCasatta RCasatta marked this pull request as draft January 20, 2025 10:50
@RCasatta RCasatta force-pushed the corepc branch 2 times, most recently from 67b72ef to 615d568 Compare January 20, 2025 11:08
@RCasatta
Copy link
Owner Author

will investigate the errors another day... Maybe @tcharding could give an hint in the meantime

@@ -10,12 +10,11 @@ mod error;
mod ext;
mod versions;

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagined one would just alias back to bitcoind

use corepc_node as bitcoind;

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah or rename directly at Cargo.toml level... I just prefer to have it explicit so that looking at the code you cannot mistakely think it's still the other crate

@tcharding
Copy link
Contributor

Sweeet, I think there are multiple problems. One is easy and one I thought I fixed - let me look more.

The easy one is that how I added a default feature in corepc-node is a foot gun, if default features are enabled corepc-node uses v28. So to correctly handle features here the dependency has to be

corepc-node = { version = "0.5.0", default-features = false }

The second one is to do with how Bitcoin Core v28 binds to ports now, something to do with the Tor changes. I thought I fixed it but let me look some more.

@tcharding
Copy link
Contributor

tcharding commented Jan 21, 2025

Ouch, I did "fix" it. Using #[cfg(not(feature = "28_0"))]

Created issue: rust-bitcoin/corepc#41

@RCasatta RCasatta marked this pull request as ready for review January 21, 2025 10:04
@RCasatta
Copy link
Owner Author

Thanks @tcharding removing default features fixed the issues...

@MaxFangX I am going to merge/release tomorrow or so

@MaxFangX
Copy link
Contributor

Thank you @RCasatta !

@tcharding
Copy link
Contributor

Oh I'm surprised CI passes using Core v28 - I thought I hit the "cannot run units tests in parallel because of port binding" issue yesterday when running them.

@MaxFangX
Copy link
Contributor

MaxFangX commented Jan 22, 2025

EDIT: Should be -bind=<addr> instead of -bind=<port>

The second one is to do with how Bitcoin Core v28 binds to ports now, something to do with the Tor changes. I thought I fixed it but let me look some more.

I recently had to muck around with this trying to run multiple bitcoind regtest instances on one machine. I don't know what your exact problem is, but in our case setting P2P::Yes still leaves port 18445=onion unchanged meaning two instances will both try to bind to 18445. I worked around this by setting -bind=<addr>. Maybe that will help in your case. @tcharding

Our code which uses electrsd, on latest released version:

// Configure bitcoind
let bitcoind_exe_path = std::env::var("BITCOIND_EXE")
    .or_else(|_| bitcoind::downloaded_exe_path())
    .expect("Didn't specify oneof `$BITCOIND_EXE` or bitcoind version in feature flags");
debug!(%bitcoind_exe_path);

let mut bitcoind_conf = bitcoind::Conf::default();
bitcoind_conf.staticdir = data_dir.as_ref().map(|d| d.join("bitcoind"));

// In bitcoind 28.0 (our current nix packaged version), `-bind` defaults
// to `127.0.0.1:18445=onion` for regtest (see `bitcoind -help`).
// Setting `bitcoind::Conf::p2p` to `P2P::Yes` sets the `-port=<port>`
// CLI arg, but it still leaves a listener at `18445=onion`, which
// causes bind conflicts when running multiple bitcoind instances. To
// overcome this, we set `-bind=[::1]:<port>`, which overrides anything
// set with `-port`. Unfortunately, setting `P2P::No` sets `-listen=0`
// which is incompatible with `-bind`. So we use `P2P::Yes` which sets
// `-listen=1` but emits a confusing unreachable p2p port in the logs.
//
// One bonus of all this is now we can specify which port the p2p
// listener should bind to. For now, we just use an ephemeral port.
let bitcoind_p2p_port =
    get_ephemeral_port().expect("Couldn't get ephemeral port");
let bitcoind_p2p_addr =
    SocketAddr::from(([0, 0, 0, 0, 0, 0, 0, 1], bitcoind_p2p_port));
info!(%bitcoind_p2p_addr);
let bind_arg = format!("-bind={bitcoind_p2p_addr}");
bitcoind_conf.args.push(&bind_arg);
bitcoind_conf.p2p = P2P::Yes;

// Init bitcoind
let bitcoind = BitcoinD::with_conf(bitcoind_exe_path, &bitcoind_conf)
    .expect("Failed to init bitcoind");

Full implementation of our wrapper here: https://gist.github.com/MaxFangX/a2c42179915b1c61f168db86f121d90c

@tcharding
Copy link
Contributor

Sweeeet, legendary @MaxFangX. Thanks man

@RCasatta RCasatta merged commit 90f52e3 into master Jan 22, 2025
10 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants