Reproduce Bitcoin Core's handling of cookie files

[casey]

I was running into an confusing issue where a cookie file that worked with bitcoin-cli wasn't working with rust-bitcoincore-rpc. It turned out that the cookie file contained a newline, which Bitcoin Core ignored but rust-bitcoincore-rpc included as part of the password.

This PR reproduces the behavior of Bitcoin Core, so that all cookie files that with Bitcoin Core should work with rust-bitcoincore-rpc.

The commit message:

Bitcoin Core uses a single call to std::getline to read the contents of cookie files, making it ignore newlines in the file, as well as ignore any lines after the first. This reproduces that behavior, so that all cookie files that work with Bitcoin Core should work with this library.

[casey]

I think this is ready to go, have a look!

[Kixunil]

ACK ea0116f

[apoelstra]

ACK ea0116f except that split_once is not available on 1.41 so we will have to find something different. This PR does not compile on 1.41 as is.

I don't know why CI is not running here..

[tcharding]

Posting so others don't check, split_post is not available until Rust 1.57

[tcharding]

If you want it; this should make the PR work with MSRV.

    /// Convert into the arguments that jsonrpc::Client needs.
    pub fn get_user_pass(self) -> Result<(Option<String>, Option<String>)> {
        match self {
            Auth::None => Ok((None, None)),
            Auth::UserPass(u, p) => Ok((Some(u), Some(p))),
            Auth::CookieFile(path) => {
                let line = BufReader::new(File::open(path)?)
                    .lines()
                    .next()
                    .ok_or(Error::InvalidCookieFile)??;
                let mut split = line.splitn(2, ":");
                Ok((
                    Some(split.next().ok_or(Error::InvalidCookieFile)?.into()),
                    Some(split.next().ok_or(Error::InvalidCookieFile)?.into()),
                ))
            }
        }
    }

[casey]

Thanks for the ping! Just rewrote it using line.find(':'), which works with 1.41.

[tcharding]

ACK f5512ae

[RCasatta]

Could you please rebase on master since #312 has been merged and should solve CI issues

[casey]

Done!

[RCasatta]

sorry, it seems it's not enough due to tempfile dep. In bitcoind I use cargo update -p tempfile --precise 3.3.0

[casey]

@RCasatta I just added that line to contrib/test.sh Hopefully it passes now.

[tcharding]

Oh man, its quote causing the problem, I hoped I fixed this one. I can have a poke around if you don't get to it. This pinning is such a useful use of everyones time :)

[casey]

I'm not sure I understand, quote is a dependency of serde, but this PR didn't change anything with serde.

[tcharding]

Its not this PR, pinning has broken, again, for some other reason. It looks broken on rust-bitcoin right now too., wrong.

[tcharding]

What a PITA, it is the order of the pinning that is broken. This works

    cargo update -p tempfile --precise 3.3.0
    cargo update -p log --precise 0.4.18
    cargo update -p serde_json --precise 1.0.99
    cargo update -p serde --precise 1.0.156
    cargo update -p quote --precise 1.0.30
    cargo update -p proc-macro2 --precise 1.0.63

[tcharding]

FTR I ran: rm Cargo.lock; cargo clean; RUSTUP_TOOLCHAIN=1.48 contrib/test.sh

[casey]

What a PITA, it is the order of the pinning that is broken. This works

😭😭😭

Okay, should be fixed now. I updated contrib/test.sh with the order in your comment. Another option for pinning the version would be to use exact semver requirements in Cargo.toml, e.g., tempfile = "=3.3.0", although I assume that wasn't used to allow newer versions of rust to get updated dependencies.

[tcharding]

although I assume that wasn't used to allow newer versions of rust to get updated dependencies.

Yep, this is all MSRV bullshit because everyone keeps changing the Rust edition and releasing it in patch versions.

[Kixunil]

Another option for pinning the version would be to use exact semver requirements in Cargo.toml, e.g., tempfile = "=3.3.0"

That's horribly broken and an absolute no-go. Cargo.lock is the correct way to pin the dependencies. We should really commit it just as we did in rust-bitcoin.

changing the Rust edition and releasing it in patch versions.

That has nothing to do with it. It's the correct way to release a new version.

If anything, cargo missing tools to automatically downgrade dependencies based on MSRV is the real problem.

[tcharding]

That has nothing to do with it. It's the correct way to release a new version.

Oh is it because those projects do not have stated MSRV that doing this is not considered a breaking change?

[Kixunil]

Oh is it because those projects do not have stated MSRV that doing this is not considered a breaking change?

Not necessarily. Bumping MSRV doesn't break the API. It only requires you to update a dependency (which rustc is). Semver is for API changes not unrelated build issues. Perhaps more importantly, doing the opposite wreaks havoc in the entire ecosystem:

• crate A 1.0 has MSRV
• crate B 1.0 depends on `A = "1.0"
• crate C depends on B = "1.0"

Now if crate A changes version to 2.0 because of MSRV bump and the maintainer of B is on holiday, C can't upgrade A without pointlessly patching B. That in itself is annoying but it gets worse. If D also depends on A but manages to update then C will get two instances of the same crate. And that leads to even more hell if B and D interact using types from A. All of this just because of an unrelated change that actually doesn't break anything technically.

What about minor version? A higher patch version means the crate has a new feature. If a crate does not block upgrading minor version by using tilde requirements then the situation is the same as patch version - the dependency is updated regardless of MSRV and bumping minor version is just confusing.

• Crate A 1.0 has MSRV and promises to update minor version (as if it had a new feature)
• B 1.0 depends on A = "~1.0"
• C depends on B = "1.0"
• D depends on A = "1.0"

If A upgrades MSRV and the version to 1.1, B can't upgrade because of the exact same reason as above and the situation is exactly as horrible with multiple dependencies and such. Again because of silly choice by B encouraged by A.

Cargo.lock has none of these problems and allow everyone to set MSRV as they wish, so it should be used instead.

[RCasatta]

utACK b16f7ef

[casey]

Nice, thanks for merging!