Add a `bitcoin-io` crate #2066

TheBlueMatt · 2023-09-13T05:18:31Z

EDIT posthumous: This PR was merged not closed, some GitHub bug led to it being marked as closed.

In order to support standard (de)serialization of structs, the
rust-bitcoin ecosystem uses the standard std::io::{Read,Write}
traits. This works great for environments with std, however sadly
the std::io module has not yet been added to the core crate.

Thus, in no-std, the rust-bitcoin ecosystem has historically
used the core2 crate to provide copies of the std::io module
without any major dependencies. Sadly, its one dependency,
memchr, recently broke our MSRV.

Worse, because we didn't want to take on any excess dependencies
for std builds, rust-bitcoin has had to have
mutually-exclusive std and no-std builds. This breaks general
assumptions about how features work in Rust, causing substantial
pain for applications far downstream of rust-bitcoin crates.

This is mostly done, I'm still finalizing the io::Error commit at the end to drop the core2 required dep in no-std, but its getting there. Would love further feedback on the approach or code-level review on these first handful of commits.

tcharding · 2023-09-13T09:54:47Z

I wrote this while reviewing, posting here in case its useful to add. It explains the impl_write macro feature gating

// !(core2 || std) : crate::io::Write
// std             : std::io::Write
// core2 && !std   : core2::io::Write

Partial review only, will come back, thanks for working on this man, its been a long time itch huh.

tcharding · 2023-09-14T13:41:06Z

bitcoin/src/crypto/key.rs

+        // Note that we must temporarily use std::io::Cursor here until libsecp256k1 converts to
+        // bitcoin_io.


Are you planning on doing this yourself after this merges? If not can you please use the form // TODO: ... so I find it when I'm clearing todo's.

I opened the followup for removal at #2080. I may or may not get around to secp256k1 moving to bitcoin_io.

Where's secp256k1 using IO? rg [^a-z]io[^a-z] doesn't return anything relevant neither does rg core2

Also Cursor wasn't required here in the first place, just use &mut &[u8].

Lol oops, I misread the error message and confused the secp publickey type for the rust-bitcoin publickey type (why are there two identically named types here 😭 )

tcharding · 2023-09-14T13:47:33Z

Finished going over it, all looking pretty sane to me.

Its draft still so this isn't code style review but if it is at all helpful you could get this to a state that its technically correct then I could take over and do the boring stuff of getting it to conform to rust-bitcoin style (whitespace, cut'n pasting docs from core/std etc). Just an offer to save you time, in no way meaning to step of your toes. I'm going to have to be reading core/std docs on the implemented traits anyways to do a deeper review.

We're working with rust-bitcoin to remove the `core2` dependency at rust-bitcoin/rust-bitcoin#2066 but until that lands and we can upgrade rust-bitcoin we're stuck with it. In the mean time, we should still pass our MSRV tests.

TheBlueMatt · 2023-09-18T21:52:36Z

Sorry for the delay here, I finished up the patchset. I ended up dropping the error::Error type entirely since its not really practical to get compatibility with the std one, but the io::Error type is pretty decent, I think.

tcharding

ACK baf0fc26c198d9d238661cfa2a4b09febb0bfdfe

Although I'd like to see the package import suggestion used. All my other comments are just comments.

I'm guessing you mostly cut'n paste this from stdlib, right? What guided your choices about what trait methods to include? I'm just interested, for example why not read _to_string?

Nice PR to review, I glad it was not me working out the compiler errors that led to all the "Stop relying on ..." changes.

Thanks

Cargo-minimal.lock

tcharding · 2023-09-19T00:37:28Z

bitcoin/Cargo.toml


+bitcoin_io = { version = "0.1", default-features = false }


Suggested change

bitcoin_io = { version = "0.1", default-features = false }

io = { package = "bitcoin_io", version = "0.1", default-features = false }

Then we can:

use plain old "io" instead of "bitcoin_io" in the featuers above

remove use bitocin_io::io from lib.rs

remove all the use crate::io statements.

If you choose to use this suggestion its a non-trivial amount of changes, I did it locally to prove it works, can push a branch for you if you want it.

Branch tmp-review-core2-pr on my tree.

Hmmmmmm I guess we could. When I'd originally written this I had an error type too, and was thus matching the std layout. I'm open to changing it, but want to make sure we want to do that before I go down the path, indeed its a good bit of work to walk back. @apoelstra?

I don't think I understand the tradeoff here.

Question is mostly do we have an io module at the top level of this crate, I think.

I slightly prefer crate renaming but it's not worth huge rewrites.

io/src/lib.rs

tcharding · 2023-09-19T00:57:00Z

io/src/lib.rs

+//!
+//! Further, with the `core2` feature, if the `std` feature is not set, the crate traits will be
+//! implemented for `core2`'s `io` traits as well.
+


Would be nice to have:

// Coding conventions #![warn(missing_docs)]

But adding it causes loads of warnings, I can add it and all the extra docs after this merges if you like.

I was figuring we'd skip it here - the traits are identical to std, and either we copy the std docs or we have much worse docs than std, which I'm not a fan of either. I'd much rather just link to std and call it a day.

tcharding · 2023-09-19T00:58:19Z

io/src/lib.rs

+
+/// Standard I/O stream definitions which are API-equivalent to `std`'s `io` module. See
+/// [`std::io`] for more info.
+pub mod io {


Out of interest, why the module? Is this because of how the crate is imported? (I removed the module to get the "package" dependency trick to work.)

I was trying to replicate std::*, and it made much more sense when I also had an error module. Now that there's no error module I think I agree with you, but #2066 (comment)

FWIW I'm totally fine to move forward with the module layout as it is.

tcharding · 2023-09-19T01:17:51Z

io/src/lib.rs

+        }
+    }
+
+    macro_rules! define_errorkind {


Nice macro!

tcharding · 2023-09-19T01:19:41Z

io/src/lib.rs

+                fn from_std(o: std::io::ErrorKind) -> ErrorKind {
+                    match o {
+                        $(std::io::ErrorKind::$kind => ErrorKind::$kind),*,
+                        _ => ErrorKind::Other


Flagging this even though I don't see a better way to handle the non_exhaustive. This is a maintenance burden, if std::io::ErrorKind gets a new variant we are going to silently convert it to Other, even if there are multiple new variants. Said another way, in reality we are never going to update this line. It would be nice if there was a way to break the build if another variant was added.

Yea, I can leave a TODO about including more any time we bump our MSRV, but indeed I'm not sure any better way.

We will get a warning when we bump MSRV so that's good but I also don't like it. Maybe we should have another hidden variant Uncategorized like std does.

I mean...isn't that Other?

Not really, Other is meant for user-defined errors, Uncategorized for internal errors (and having an out-of-date library is one)

Hmm, so if we add an Uncategorized, what do we map that into when converting to std? We can't access the std Uncategorized as its marked unstable, so we'd have to make it Other, and then we're back to square one.

Oh, good point. I guess let's keep it as is, I hope we have generic error at some point anyway and then all of this is moot.

tcharding · 2023-09-19T01:37:25Z

io/src/lib.rs

+                    Ok(0) =>
+                        return Err(ErrorKind::UnexpectedEof.into()),
+                    Ok(len) => {
+                        buf = &mut buf[len..];


If you have time to educate me, why does the stdlib implementation use tmp?

Ok(n) => { let tmp = buf; buf = &mut tmp[n..]; }

For what its worth, I rekon n is nicer than len :)

Absolutely no idea. I definitely prefer len over the generic n :)

I believe it is from time before NLL was a thing and nobody bothered to change it.

io/src/lib.rs

tcharding · 2023-09-19T01:52:55Z

io/src/lib.rs

+        }
+        #[inline]
+        #[cfg(any(feature = "alloc", feature = "std"))]
+        fn read_to_end(&mut self, buf: &mut alloc::vec::Vec<u8>) -> Result<usize> {


Since stdlib's version has some heavy optimisations I wonder if we should just call through to it if the "std" feature is enabled?

In 9a1c9e5:

I am actually a little tempted to go in the opposite direction, changing behavior from stdlib to add a max allocation size and return an error if we over-read. This function has always made me a little nervous (same with read_to_string) since it's a memory-DoS vector.

In either case, can wait for a followup PR.

Interesting, I didn't think of that.

I didn't read the std implementations here cause it has a different license than us, but given our reads tend to be quite short, I'd be surprised if you could do much better than this. Maybe increase the read buffer to 4096.

There's an argument to be made that bitcoin&co should never call this method so perhaps it'd make sense to remove it completely?

Everywhere that we read vectors we check the varint-encoded length and reject if it's too high.

AFAIK we don't use read_to_end or read_to_string anywhere in this library exactly because they are uncontrolled DoS vectors. It is true that you can simulate them to get a DoS anyway, but that's no reason for us to implement them.

Exactly, we wrote a whole bunch of trait methods just to solve the DoS vector for consensus-encoded types, read_to_end is a missing piece.

read_to_end is used in psbt.

I know, I already checked. But it's actually reimplemented there so the method is not needed. And still I wonder if we have a DoS vector. I wasn't able to find out.

Sure, I dropped the commit which inlines it in Read rather than just having it in psbt/raw.rs.

tcharding · 2023-09-19T02:22:53Z

You can rebase on master to pick up #2077 to fix CI.

apoelstra · 2023-09-19T16:08:33Z

io/src/lib.rs

+/// users using those features. In any case, this crate's `io::Write` feature will be implemented
+/// for the given type, even if indirectly.
+#[cfg(not(any(feature = "core2", feature = "std")))]
+macro_rules! impl_write {


In dd8dd88:

Fine to wait for a followup PR, but all these macros should have invocations that "look like" the code that they generate. Rather than being lists of exprs.

Mmm, yea, good point.

io/src/lib.rs

apoelstra

ACK baf0fc26c198d9d238661cfa2a4b09febb0bfdfe

TheBlueMatt · 2023-09-19T17:28:52Z

Happy to respond to all the comments later today, or can do so in a followup, up to y'all.

apoelstra · 2023-09-19T17:59:01Z

For my part I'd prefer a followup since this is such a big PR. But I'm fine either way.

tcharding

Happy to ack and do as a follow up. We should ensure this doesn't get released to crates.io with the underscore in the crate name. Also I'd like to change (later) the dependency style to use "package" before release.

ACK baf0fc26c198d9d238661cfa2a4b09febb0bfdfe

tcharding · 2023-09-19T21:25:15Z

Needs rebase to get green on the MSRV ci job.

tcharding · 2023-09-22T02:47:35Z

I was just going through the Rust API guidelines and all our crates trying to work out how we get to v1.0.0 I realised that because the new io crate includes core2 in the public API we cannot stabalize it.

What do we want to do about that? I assume core2 is never going to get to v1.0.0.

ref: https://rust-lang.github.io/api-guidelines/necessities.html

apoelstra · 2023-09-23T16:43:19Z

@tcharding I think it's fine to be 1.0 with the caveat that if you enable an unstable feature then you'll get unstable stuff.

tcharding · 2023-09-24T08:24:16Z

And mark "core2" as unstable somewhere you mean? Or just assume its implied because of the version number of core2?

apoelstra · 2023-09-24T14:07:52Z

@tcharding yeah, we'd put it in the README or something. We could name the feature core2_unstable and use some tricks to make sure compiling with core2 directly doesn't work, but TBH that feels like overkill.

tcharding · 2023-09-24T19:31:05Z

Cool, ~~well this is good to go in then.~~ Let me know @TheBlueMatt if you want help doing the docs and trivial clean up, happy to help but I don't want to tread on your toes.

EDIT: After rebase

TheBlueMatt · 2023-11-07T05:54:20Z

Okay, squashed and rewrote the commit messages where relevant.

In the next commit, `std::io::Error` will be replaced with a different type, and as a result `std::io::Error` must be referred to explicitly.

In order to move towards our own I/O traits in the `rust-bitcoin` ecosystem, we have to slowly replace our use of the `std` and `core2` traits. This is the final step in removing the explicit `core2` dependency for I/O in no-std - replacing the `io::Error` type with our own. Sadly the `std::io::Error` type requires `std::error::Error` as a bound on the inner error, which is rather difficult to duplicate in a way that allows for mapping to `std` and back. To take a more general approach, we use bound on any `Debug` instead.

tcharding

ACK d6b33bae4115ca4d86796e129579caad5441a4db

tcharding · 2023-11-07T20:47:41Z

Thanks man, this is sick.

tcharding · 2023-11-07T20:49:16Z

Need to remove the memchr pin from contrib/test.sh.

TheBlueMatt · 2023-11-09T16:47:01Z

Done

tcharding

ACK add371d

apoelstra

ACK add371d

tcharding · 2023-11-17T20:02:04Z

@Kixunil can we merge this but not release the crate, and then get fix up your concerns before we release it? I'm super keen to get hacking on top of this.

apoelstra · 2023-11-18T15:25:34Z

I'm also super keen to merge this before it needs a rebase :)

Kixunil

ACK add371d

While there are thing I hope will change, removing core2 is a huge improvement.

add371d Remove `core2` dependency entirely (Matt Corallo) b7dd16d [IO] Use our own io::Error type (Matt Corallo) c95b593 Explicitly use `std::io::Error` when implementing `std` traits (Matt Corallo) 9e1cd37 Use `io::Error::get_ref()` over `std::error::Error::source()` (Matt Corallo) 3caaadf [IO] Replace the `io::Cursor` re-export with our own `Cursor` (Matt Corallo) 141343e [IO] Move to custom `Read` trait mirroring `std::io::Read` (Matt Corallo) 7395093 Stop relying on `Take`'s `by_ref` method (Matt Corallo) 2364e1a Stop relying on blanket Read impl for all &mut Read (Matt Corallo) 6aa7ccf [IO] Replace `std::io::Sink` usage with our own trivial impl (Matt Corallo) 7eb5d65 [IO] Provide a macro which implements `io::Write` for types (Matt Corallo) ac678bb [IO] Move to custom `Write` trait mirroring `std::io::Write` (Matt Corallo) 5f2395c Add missing `?Sized` bounds to `io::Write` parameters (Matt Corallo) 2348449 Stop relying on `std::io::Write`'s `&mut Write` blanket impl (Matt Corallo) 5e02095 Use `io::sink` rather than our custom `EmptyWrite` utility (Matt Corallo) a0ade88 [IO] Move io module into selected re-exports (Matt Corallo) 27c7c4e Add a `bitcoin_io` crate (Matt Corallo) Pull request description: In order to support standard (de)serialization of structs, the `rust-bitcoin` ecosystem uses the standard `std::io::{Read,Write}` traits. This works great for environments with `std`, however sadly the `std::io` module has not yet been added to the `core` crate. Thus, in `no-std`, the `rust-bitcoin` ecosystem has historically used the `core2` crate to provide copies of the `std::io` module without any major dependencies. Sadly, its one dependency, `memchr`, recently broke our MSRV. Worse, because we didn't want to take on any excess dependencies for `std` builds, `rust-bitcoin` has had to have mutually-exclusive `std` and `no-std` builds. This breaks general assumptions about how features work in Rust, causing substantial pain for applications far downstream of `rust-bitcoin` crates. This is mostly done, I'm still finalizing the `io::Error` commit at the end to drop the `core2` required dep in no-std, but its getting there. Would love further feedback on the approach or code-level review on these first handful of commits. ACKs for top commit: tcharding: ACK add371d apoelstra: ACK add371d Kixunil: ACK add371d Tree-SHA512: 18698ea8b1b65108ee0f695d5062d2562c8df2f50bf85d93442648da3b35a4184a5d5d2a493aed0adaadc83f663f0cd2ac735c34941cc9a6fa58d826e548e091

apoelstra · 2023-11-19T14:38:07Z

This was merged by me; Matt had nothing to do with it. It's just a github bug showing that he closed it.

tcharding · 2023-11-20T03:57:52Z

The additiveness is still not fully addressed.

Is this still an issue? I lost the discussion in this long thread.

Also this seems to be missing BufRead which is more important trait for parsing from IO (which we admittedly don't use yet and we should). I guess I could accept it as a temporary solution but not something I'm entirely satisfied with.

Since I pushed to get this PR merged I feel like its my responsibility to do the follow ups, mentioning it first incase you have the BufRead stuff queued up already @Kixunil? (Or anyone else :)

EDIT: Oh it will probably be part of #2179

Kixunil · 2023-11-23T19:06:03Z

@tcharding I'm still a bit sick so if you have the time to try rebasing #2179 feel free to go ahead! Even if I manage to do some work I will likely focus on other things for a bit.

tcharding · 2023-11-23T19:37:03Z

Sure thing man, can do!

tcharding · 2023-11-28T04:35:38Z

Hey man, the flurry of action on io is in preparation for me working out how to rebase #2179 - I have to add a stripped down version of BufRead that just serves our purposes, right? (As we do in the new io crate for the Write/Read traits.) I looked at the stuff in lgio but it seems too thorough so I started to hack something together using stdlib, am I on the right track?

Kixunil · 2023-11-28T08:43:07Z

Yeah, use the same error type as for Read and add some simple BufRead trait. fill_buf and consume might be enough.

TheBlueMatt force-pushed the master branch 2 times, most recently from 1a4b012 to b579940 Compare September 13, 2023 05:28

TheBlueMatt force-pushed the master branch from b579940 to 54cf06e Compare September 14, 2023 04:25

tcharding reviewed Sep 14, 2023

View reviewed changes

TheBlueMatt force-pushed the master branch from 54cf06e to 27fb60a Compare September 18, 2023 21:48

TheBlueMatt mentioned this pull request Sep 18, 2023

Drop core2 requirement #2080

Closed

TheBlueMatt marked this pull request as ready for review September 18, 2023 21:51

TheBlueMatt force-pushed the master branch 3 times, most recently from 75042df to baf0fc2 Compare September 18, 2023 22:20

tcharding reviewed Sep 19, 2023

View reviewed changes

apoelstra reviewed Sep 19, 2023

View reviewed changes

io/src/lib.rs Outdated Show resolved Hide resolved

apoelstra previously approved these changes Sep 19, 2023

View reviewed changes

tcharding previously approved these changes Sep 19, 2023

View reviewed changes

Kixunil mentioned this pull request Nov 7, 2023

Require BufRead instead of Read #2179

Closed

TheBlueMatt added 4 commits November 7, 2023 17:35

[IO] Replace the io::Cursor re-export with our own Cursor

3caaadf

Use io::Error::get_ref() over std::error::Error::source()

9e1cd37

Explicitly use std::io::Error when implementing std traits

c95b593

In the next commit, `std::io::Error` will be replaced with a different type, and as a result `std::io::Error` must be referred to explicitly.

TheBlueMatt force-pushed the master branch from 930d7f0 to d6b33ba Compare November 7, 2023 17:35

tcharding previously approved these changes Nov 7, 2023

View reviewed changes

Remove core2 dependency entirely

add371d

TheBlueMatt dismissed tcharding’s stale review via add371d November 9, 2023 16:46

TheBlueMatt force-pushed the master branch from d6b33ba to add371d Compare November 9, 2023 16:46

tcharding approved these changes Nov 10, 2023

View reviewed changes

apoelstra approved these changes Nov 16, 2023

View reviewed changes

tcharding changed the title ~~Add a bitcoin_io crate~~ Add a bitcoin-io crate Nov 17, 2023

Kixunil approved these changes Nov 18, 2023

View reviewed changes

TheBlueMatt closed this Nov 19, 2023

AnthonyRonning mentioned this pull request Dec 5, 2023

Remove lightning-invoice std feature from client fedimint/fedimint#3838

Closed

apoelstra mentioned this pull request Jan 16, 2024

Improve error handling in the sighash module #2329

Merged

		// Note that we must temporarily use std::io::Cursor here until libsecp256k1 converts to
		// bitcoin_io.


	bitcoin_io = { version = "0.1", default-features = false }
	io = { package = "bitcoin_io", version = "0.1", default-features = false }

Add a bitcoin-io crate #2066

Add a bitcoin-io crate #2066

Uh oh!

Conversation

TheBlueMatt commented Sep 13, 2023 • edited by tcharding Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Sep 18, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Add a `bitcoin-io` crate #2066

Add a `bitcoin-io` crate #2066

TheBlueMatt commented Sep 13, 2023 •

edited by tcharding

Loading

tcharding commented Sep 13, 2023 •

edited

Loading

tcharding commented Sep 14, 2023 •

edited

Loading

tcharding Sep 19, 2023 •

edited

Loading