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

Update README.md #281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthiasdebernardini
Copy link

Changing localhost to actual IP address, on macOS this doesn't get interpreted correctly and there is no error to show you how to fix it.

Changing localhost to actual IP address, on macOS this doesn't get interpreted correctly and there is no error to show you how to fix it.
Given below is an example of how to connect to the Bitcoin Core JSON-RPC for a Bitcoin Core node running on `localhost`
and print out the hash of the latest block.
Given below is an example of how to connect to the Bitcoin Core JSON-RPC for a Bitcoin Core node running on `localhost`
and print out the hash of the latest block.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you've added trailing whitespace here. Can you instead remove all the existing trailing whitespace?

@@ -26,7 +26,7 @@ use bitcoincore_rpc::{Auth, Client, RpcApi};

fn main() {

let rpc = Client::new("http://localhost:8332",
let rpc = Client::new("http://127.0.0.1:8332",
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty skeptical of this change, TBH. It explicitly encodes ipv4 and will break if Core is not listening on ipv4.

Choose a reason for hiding this comment

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

Understood, maybe just revert it back and add a short caveat at the bottom for those on macOS? its a pretty subtle "feature" of using this crate with macOS - kinda frustrating to run into (I had known about this actually but completely forgot).

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious if there's any other network client crate that gets this "right".

Definitely happy to add some extra text to the README suggesting that people try replacing localhost with 127.0.0.1. But I think we should leave the default example code as is.

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.

2 participants