new literacy::{Read, Write} traits to handle std/no_std (alternative) #127

RCasatta · 2021-05-06T11:33:49Z

Alternative to #126
see description there

TheBlueMatt · 2021-05-10T15:35:53Z

src/literacy.rs

@@ -0,0 +1,123 @@
+#[cfg(all(feature = "std", feature = "use-core2"))]
+compile_error!("feature \"std\" and \"use-core2\" cannot be enabled together.");


I think we should "default" to core2 in this case - if we already are building against the core2 dep, we might as well just assume the user set core2/std and then the core2 impl is the same as doing the std impl.

I agree but there are some implementation issue, in particular

we might as well just assume the user set core2/std

How so? with another feature use-core2-std = ["core2/std"]? This would cause other issues in feature combinations...
The other option, using std = ["core2/std"] will cause problem in default features compilation on rust 1.29

like you said in chat....

ugh, I wish there were a way in cargo.toml to say "if core2 && std -> require core2/std"

How so? with another feature use-core2-std = ["core2/std"]? This would cause other issues in feature combinations...

We don't have to specify it ourselves for core2/std to be enabled by some other dependency path. Its probably neater to just impl for std and not core2 than to compile-error, and if users intended to use core2 they can always fix it by, themselves, directly requiring core2/std. This avoids the issue of having some crate that depends on A and B and A enables std and B enables core2, giving the user no way to fix the issue aside from patching either A or B.

Sure, removed the compile_error and implementing std impl only if there is std but not core2

src/literacy.rs

TheBlueMatt · 2021-05-10T15:59:29Z

I think also we should have a not-std&&not-core2 impl block for Write into a Vec and Read from a slice, like @devrandom did in rust-bitcoin at https://github.com/rust-bitcoin/rust-bitcoin/pull/603/files#diff-76866598ce8fd16261a27ac58a84b2825e6e77fc37c163a6afa60f0f4477e569R199-R256

RCasatta · 2021-05-11T12:22:52Z

contrib/test.sh

    cargo clean
    CC='clang -fsanitize=memory -fno-omit-frame-pointer'                                         \
    RUSTFLAGS='-Zsanitizer=memory -Zsanitizer-memory-track-origins -Cforce-frame-pointers=yes'   \
-    cargo test --lib --all --features="$FEATURES" -Zbuild-std --target x86_64-unknown-linux-gnu


should be --all-features here and above, waiting to find a solution for defaulting to core2 if enabled (without conflicting with std)

RCasatta · 2021-05-11T13:16:09Z

I think also we should have a not-std&&not-core2 impl block for Write into a Vec and Read from a slice, like @devrandom did in rust-bitcoin at https://github.com/rust-bitcoin/rust-bitcoin/pull/603/files#diff-76866598ce8fd16261a27ac58a84b2825e6e77fc37c163a6afa60f0f4477e569R199-R256

added in 4291b4b and also removed write_all blanket impl

TheBlueMatt

I think this is basically almost there. Is it time to un-draft it?

TheBlueMatt · 2021-05-18T17:51:04Z

src/lib.rs

@@ -25,7 +25,7 @@
 #![deny(non_camel_case_types)]
 #![deny(non_snake_case)]
 #![deny(unused_mut)]
-#![deny(missing_docs)]
+//#![deny(missing_docs)]


I hope you didn't intend to land this :)

removed, and added doc where missing in 8bf2f9e

TheBlueMatt · 2021-05-18T17:53:15Z

src/literacy.rs

@@ -0,0 +1,123 @@
+#[cfg(all(feature = "std", feature = "use-core2"))]
+compile_error!("feature \"std\" and \"use-core2\" cannot be enabled together.");


How so? with another feature use-core2-std = ["core2/std"]? This would cause other issues in feature combinations...

We don't have to specify it ourselves for core2/std to be enabled by some other dependency path. Its probably neater to just impl for std and not core2 than to compile-error, and if users intended to use core2 they can always fix it by, themselves, directly requiring core2/std. This avoids the issue of having some crate that depends on A and B and A enables std and B enables core2, giving the user no way to fix the issue aside from patching either A or B.

src/literacy.rs

sgeisler

utACK 8bf2f9e for the rust code, not so sure about the tests, needs some clarification at least imo.

sgeisler · 2021-05-19T12:47:10Z

contrib/test.sh

-FEATURES="serde serde-std"
+# Combination of features to test, should be every features combination but std and core2 together
+# note std has a comma in the end so that following regex avoid matching serde-std
+FEATURES=("" "std," "use-core2" "use-core2-std" "std,use-core2" "std,serde-std" "use-core2,serde-std")


What happened to the serde feature? I assume serde without std is expected to work in no-std mode so it should be tested accordingly?

serde added in 7b909c9, I forget about it because it's used but not listed in the Cargo.toml feature section...

Not sure if other combination are needed though

sgeisler · 2021-05-19T12:48:52Z

contrib/test.sh

+          echo "--------------$feature----------------"
+          cargo build --no-default-features --features="$feature"
+          if [[ ${feature} =~ "std," ]] ; then
+              cargo test --no-default-features --features="$feature"


Why do we only test with std and just build the rest of the time?

because tests have std enabled

#![cfg_attr(all(not(test), not(feature = "std")), no_std)]

devrandom · 2021-05-19T17:05:54Z

I tried to compile the following example, which uses a pattern seen in rust-bitcoin. Namely, the caller passes in a reference, but the function called expects a non-reference Write trait. In particular, this is seen when a hash engine is passed into consensus_encode.

extern crate bitcoin_hashes;

use bitcoin_hashes::literacy::Write;
use bitcoin_hashes::{sha256d, Hash, hash_newtype, hex_fmt_impl, index_impl, serde_impl, borrow_slice_impl};

hash_newtype!(Txid, sha256d::Hash, 32, doc="A bitcoin transaction hash/transaction ID.");

fn do_it<W: Write>(mut w: W) {
	let _ = w.write_all(&[0u8; 1]);
}

fn main() {
	let mut enc = Txid::engine();
	do_it(&mut enc);
}

But this fails to compile. I tried creating an impl<W: ::std::io::Write> Write for &mut W {, but that conflicted with the existing impl. Any ideas?

TheBlueMatt · 2021-05-19T21:18:46Z

impl<W: ::std::io::Write> Write for &mut W {

What about impl<W: ::literacy::Write> ::literact::Write for &mut W {? That's closer to what std::io::Write does, though I have a feeling that will conflict, too, because of the blanket std::io::Write impl. I don't think its the end of the world to move rust-bitcoin to always taking a &mut W instead, though. @apoelstra ?

devrandom · 2021-05-20T09:31:47Z

Yeah, the other way results in the same conflict.

I'm playing with taking &mut W instead of mut W, and that seems to work.

A further issue is that rust-bitcoin uses Read::take, which we don't implement here. Should we grab that from std?

And one more question - how exactly do we concretize the Error type param? For example, is this what we want for the Encodeable trait in rust-bitcoin (and the few dozen impls)?

pub trait Encodable {
    fn consensus_encode<W: io::Write<Error=io::Error>>(&self, writer: &mut W) -> Result<usize, io::Error>;
}

(where the io::Error type depends on std vs no-std)

RCasatta · 2021-05-20T10:18:33Z

@afilini and I did some test for accepting both self and &mut self like std https://github.com/RCasatta/bitcoin_hashes/tree/literacy_associated_alek

we can't impl<W: ::std::io::Write> Write for &mut W { because we already have a generic impl (differently from std)

however, Alekos come up with one generic implementation that should support both self and &mut self

impl<W: BorrowMut<dyn (::std::io::Write)>> Write for W {

which works but the problem in @devrandom's example remains because engines implement directly the literacy trait so they are not getting the BorrowMut thing

sgeisler · 2021-05-20T14:03:25Z

I don't really like the BorrowMut<dyn (::std::io::Write)> solution, do we know if it will be optimized away? Or will people unknowingly be using dynamic dispatch? Especially for something kinda low-level like io that would be annoying.

I think implementing literacy::Write and literacy::Read directly is actually an outlier (although very prominent in hashes). Afaik the remaining API of rust-bitcoin is more a consumer of objects implementing these traits. Since std implements impl<W: Write + ?Sized> Write for &mut W and we implement impl<W: ::std::io::Write> Write for W we already get literacy impls for most relevant mut borrows, making the hacky blanket BorrowMut impl unnecessary except for the few cases where literacy traits are implemented directly. I propose to instead use a macro to just impl literacy::Write for both HashEngine and &mut HashEngine (19b74b7).

afilini · 2021-05-20T14:23:24Z

making the hacky blanket BorrowMut impl unnecessary except for the few cases where literacy traits are implemented directly.

Not even that, because implementing it that way made it only apply to types that already implemented std::io::Write and that we were "bridging" to literacy::Write, which as you pointed out already had an implementation for their references since the std trait is implemented on both automatically. We were just trying to come up with something cool but in the end I don't think it can be done automatically.

TheBlueMatt · 2021-05-20T14:49:23Z

Yea, I think we definitely can't do anything that relies on dyn here, the optimization story of this type of deserialization logic is already pretty rough (and we've had users with significant performance bottlenecks in our deserialization before, it probably still is the bottleneck for electrs), we can't make it worse.

RCasatta · 2021-05-20T15:18:17Z

I think 19b74b7 is the way to go, but there are some issues with lifetimes on 1.29 https://github.com/RCasatta/bitcoin_hashes/runs/2631403404?check_suite_focus=true, @sgeisler do you want to look at it?

sgeisler · 2021-05-20T15:20:18Z

Sure, will do, the commit was merely to illustrate my point, I'll also remove the example again.

Using a macro makes the code more compact and allows us to easily add an impl for &mut HashEngine which could not be achieved through the type system otherwise.

sgeisler · 2021-05-20T16:02:04Z

The embedded test broke somehow 😦 I'm not sure if it has anything to do with my commit. Did it increase the lib size so much that it doesn't fit into the flash anymore?

RCasatta · 2021-05-21T08:58:29Z

A further issue is that rust-bitcoin uses Read::take, which we don't implement here. Should we grab that from std?

added in a37f2be

Another option without adding the fn to the trait and the associated type would have been to restore the CappedRead RCasatta/rust-bitcoin@24015e8, however I preferred like it's done a37f2be (inheriting std/core2 implementation, and the simple implementation for the slice, there should not be any other case where we implement Read in rust-bitcoin if I am not mistaken)

devrandom · 2021-05-21T09:45:07Z

This question remains, how do we concretize Error for Encodable in rust-bitcoin?

One issue is that the hash engines already concretize Error to () in this crate, so that would require a bunch of error mapping if Encodable wants to concretize to io::Error.

And one more question - how exactly do we concretize the Error type param? For example, is this what we want for the Encodable trait in rust-bitcoin (and the few dozen impls)?
pub trait Encodable {
    fn consensus_encode<W: io::Write<Error=io::Error>>(&self, writer: &mut W) -> Result<usize, io::Error>;
}
(where the io::Error type depends on std vs no-std)

RCasatta · 2021-05-21T11:49:29Z

This question remains, how do we concretize Error for Encodable in rust-bitcoin?

with the latest commits it should be:
(literacy::Error it's a re-export according to the activated features)

  pub trait Encodable {
      fn consensus_encode<W: literacy::Write<Error=literacy::Error>(&self, writer: &mut W) -> Result<usize, literacy::Error>;
  }

One issue is that the hash engines already concretize Error to () in this crate, so that would require a bunch of error mapping if Encodable wants to concretize to io::Error.

1f500f1 use literacy::Error also for engines (the () was conveying more the idea of the absence of error but this way should be more convenient to use and I added a comment)

afilini · 2021-05-21T12:03:52Z

src/literacy.rs

+    /// The error type returned in Result
+    type Error;
+    /// The type to implement limited reads
+    type Take;


I would add a trait bound to literacy::Read here, it might help later on when working with generics

Alternatively you could also add another trait specifically for the Take type that "extends" Read and forces a specific constructor-like method. With that you can then provide a default implementation for take() like std does, because you can always construct an object of type Self::Take using the constructor defined by its trait.

added trait bound in 6b7b28f

RCasatta · 2021-05-21T12:24:16Z

After adding the literacy::Error logic I realized it may be possible to remove the associated parameter Error, adding something like alloc::Box<dyn OurErrorTrait> where OurErrorTrait: Display + Debug + … {} for the default_impl...

But I would like to ear opinions before doing such changes

TheBlueMatt · 2021-05-21T14:33:06Z

adding something like alloc::Box where OurErrorTrait: Display + Debug + … {} for the default_impl...

Why? Lets generally avoid dyn indirection unless we have a good reason to prefer it.

TheBlueMatt · 2021-05-21T14:34:42Z

This question remains, how do we concretize Error for Encodable in rust-bitcoin?

How much do we actually add an error ourselves? I assume we should basically never error unless the underlying Write does, so could we not just return Result<usize, W::Error>?

devrandom · 2021-05-21T15:13:36Z

Do we really want these verbose function signatures (illustrated for the std case):

pub trait Decodable: Sized {
    /// Decode an object with a well-defined format
    fn consensus_decode<D: io::Read<Error=encode::Error, Take=::std::io::Take<D>>>(d: &mut D) -> Result<Self, encode::Error>;
}

impl Decodable for OutPoint {
    fn consensus_decode<D: io::Read<Error=encode::Error, Take=::std::io::Take<D>>>(d: &mut D) -> Result<Self, encode::Error> {
        Ok(OutPoint {
            txid: Decodable::consensus_decode(d)?,
            vout: Decodable::consensus_decode(d)?,
        })
    }
}

sgeisler · 2021-05-21T16:01:36Z

After adding the literacy::Error logic I realized it may be possible to remove the associated parameter Error, adding something like alloc::Box<dyn OurErrorTrait> where OurErrorTrait: Display + Debug + … {} for the default_impl...

Please don't, the entire point of this PR was to have the error be an associated type instead of a Box<dyn …> compared to #126 .

This question remains, how do we concretize Error for Encodable in rust-bitcoin?

My vision for the API is as follows:

pub trait Encodable {
    fn consensus_encode<W: literacy::Write>(&self, writer: W) -> Result<usize, W::Error>;
}

pub trait Decodable: Sized {
    fn consensus_decode<D: literacy::Read>(d: D) -> Result<Self, DecodeError<D::Error>>;
}

pub enum DecodeError<E: Display + Debug + …> {
    Io(E),
    Psbt(psbt::Error),
    …
}

The caller of one of these functions typically knows if they use std or core2, or if not they will just have to do the same generic trick that we are doing here. So there is no need to use a Box<dyn> for errors.

Do we really want these verbose function signatures (illustrated for the std case):

I think there are some misconceptions about having to name associated types in that context, you don't have to. You can access them, for every struct-trait pair these are well-defined, so no need to actually specify them anywhere here.

RCasatta · 2021-05-21T17:02:29Z

@TheBlueMatt @sgeisler so it's good like it is?

src/literacy.rs

devrandom · 2021-05-21T17:58:57Z

src/literacy.rs

+    /// see [std::io::Read::read_exact]
+    fn read_exact(&mut self, buf: &mut [u8]) -> ::core::result::Result<(), Self::Error>;
+    /// see [std::io::Read::take]
+    fn take(self, limit: u64) -> Self::Take;


This doesn't seem to quite work with the &mut pattern:

fn consensus_decode<D: io::Read>(d: &mut D) -> Result<Self, encode::Error> { let mut d = d.take(MAX_VEC_SIZE as u64);

gives:

error[E0507]: cannot move out of `*d` which is behind a mutable reference let mut d = d.take(MAX_VEC_SIZE as u64); ^ move occurs because `*d` has type `D`, which does not implement the `Copy` trait

maybe removing take from the trait and using something like the CappedRead in RCasatta/rust-bitcoin@24015e8 ?

I'll give this a try.

Seems to work fine, let's take out the take.

rust-bitcoin/rust-bitcoin@4e2a451

take removed in c6d4ca4

sgeisler · 2021-05-21T20:28:22Z

I tried to migrate the consensus de/encoding API in rust-bitcoin to this PR and have to say it's not too pleasant 😆 some problems I noticed:

The generic error type is infecting way more parts of the code than I expected, partly because the encode::Error is not limited to io-related functionality. I think it would actually be a good thing to untangle it a bit, but that might turn out to be a very big task.
Some of our code actually uses the information in the io errors, e.g. there is StreamReader::read_next which tries to decode some data to see if it received enough bytes or if it needs more (being signaled by io::ErrorKind::UnexpectedEof).

This means we'd have to do a lot of work to make rust-bitcoin compatible. But I also noticed that the ErrorKind type from both std and core is the same. So a middle ground might be found by building our own error type that is slightly more intelligent than Box<dyn OurErrorTrait>:

struct IoError {
    kind: IoErrorKind,
    error: Box<dyn OurErrorTrait>, //
}

trait OurErrorTrait: Debug + Any {}

This would allow cheap conversion from both std and core2 errors, cheap superficial mathching of error causes and expensive deeper introspection when needed by downcasting the underlying error. It's not pretty, but probably a lot less work than restructuring the entire error handling of rust-bitcoin. What do you think?

RCasatta · 2021-05-24T14:16:09Z

Would like to ear from @devrandom, @apoelstra and @TheBlueMatt about @sgeisler proposal before doing changes

devrandom · 2021-05-24T15:07:25Z

It sounds plausible, but it's difficult to evaluate without trying it. If it can be quickly prototyped, I can try it out in rust-bitcoin.

BTW, I had the same experience as @sgeisler when trying to apply the current version to rust-bitcoin, which motivated some of my previous questions...

TheBlueMatt · 2021-05-24T15:42:28Z

Without having dug into it too much, building on @sgeisler's suggestion, what about:

trait ErrTrait { fn kind(&self) -> ErrorKind; }
impl Write { type Error: ErrTrait; ... }
impl ErrTrait for std::io::Error { .. }

…rospection, adds ErrorTrait to expose kind

RCasatta · 2021-05-25T13:51:07Z

So I worked on the ErrorKind in 38bb903, unfortunately, it is not in core so I had to duplicate the enum...

apoelstra · 2021-05-25T19:33:09Z

I'm having trouble following all the discussion, especially as the PR is now 14 commits, many of which undo changes that earlier commits do. I agree with others saying that we should not use dyn anywhere. @sgeisler's suggested IoError still involves an allocation when constructing the error type which is far too much overhead for something that (conceptually) shouldn't need to be much more than a u16.

I'm also unconvinced of the merits of creating our own parallel implementation of the core2 crate. Our types which implement our traits will not be usable with other crates that expect the core2 traits or vice-versa, and as we've seen there is actually a significant amount of complexity involved in simulating Read/Wirte well enough to make this usable. This is an extra maintenance burden that we don't want to take on, and it's very difficult to reason about all the potential feature-flag interactions.

TheBlueMatt · 2021-05-25T21:57:44Z

I'm also unconvinced of the merits of creating our own parallel implementation of the core2 crate

Ugh, yea, after all the work here, it just seems much more complicated than I was anticipating. Maybe if ErrorKind was exposed it could have gone down this path without the crazy amount of work here. Apologies for pushing the wrong way here.

RCasatta · 2021-05-26T08:35:10Z

At least we tried... Somehow relieved to close this...

I remain with an open question though, is "deeper introspection when needed by downcasting the underlying error" really needed somewhere? or one could hypothetically go with just ErrorKind (not ideal losing error information but maybe good enough)?

RCasatta added 2 commits May 6, 2021 11:17

new literacy::{Read, Write} traits to handle std/no_std

f693c4b

use associated type in literacy Read/Write trait

41e04db

RCasatta marked this pull request as draft May 6, 2021 11:34

TheBlueMatt reviewed May 10, 2021

View reviewed changes

src/literacy.rs Show resolved Hide resolved

TheBlueMatt mentioned this pull request May 10, 2021

no_std support, keeping MSRV rust-bitcoin/rust-bitcoin#603

Merged

2 tasks

RCasatta added 2 commits May 11, 2021 13:56

add default impl for vec and slice, remove blanket impl

4291b4b

upgrade CI

73c4aad

RCasatta commented May 11, 2021

View reviewed changes

This was referenced May 13, 2021

new literacy::{Read, Write} traits to handle std/no_std #126

Closed

New alloc feature rust-bitcoin/rust-secp256k1#300

Merged

TheBlueMatt mentioned this pull request May 17, 2021

Initial support for no_std platforms lightningdevkit/rust-lightning#842

Closed

TheBlueMatt reviewed May 18, 2021

View reviewed changes

RCasatta added 2 commits May 19, 2021 11:05

defaults to core2 if both std and core2 features enabled, fixes

62855e6

Docs for literacy module and restore enforcing docs

8bf2f9e

RCasatta marked this pull request as ready for review May 19, 2021 09:42

sgeisler reviewed May 19, 2021

View reviewed changes

use macros to impl literacy::Write

12580fd

Using a macro makes the code more compact and allows us to easily add an impl for &mut HashEngine which could not be achieved through the type system otherwise.

RCasatta added 2 commits May 21, 2021 10:38

test with serde feature

9e49b27

Add take to literacy::Read trait

a37f2be

RCasatta force-pushed the literacy_associated branch from 7b909c9 to a37f2be Compare May 21, 2021 08:56

RCasatta added 2 commits May 21, 2021 12:22

rexport literacy::Error according to the activated feature

a8eb19c

use literacy::Error also for engines

1f500f1

afilini reviewed May 21, 2021

View reviewed changes

add Read trait bound to Take

6b7b28f

sgeisler reviewed May 21, 2021

View reviewed changes

src/literacy.rs Outdated Show resolved Hide resolved

devrandom reviewed May 21, 2021

View reviewed changes

RCasatta added 2 commits May 25, 2021 11:59

remove Take

c6d4ca4

Add literacy:Error with inexpensive ErrorKind and inner error for int…

38bb903

…rospection, adds ErrorTrait to expose kind

RCasatta closed this May 26, 2021

devrandom mentioned this pull request May 28, 2021

Add core2 support #128

Merged

		@@ -0,0 +1,123 @@
		#[cfg(all(feature = "std", feature = "use-core2"))]
		compile_error!("feature \"std\" and \"use-core2\" cannot be enabled together.");

new literacy::{Read, Write} traits to handle std/no_std (alternative) #127

new literacy::{Read, Write} traits to handle std/no_std (alternative) #127

Conversation

RCasatta commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented May 10, 2021

Choose a reason for hiding this comment

RCasatta commented May 11, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom commented May 19, 2021

TheBlueMatt commented May 19, 2021

devrandom commented May 20, 2021

RCasatta commented May 20, 2021

sgeisler commented May 20, 2021

afilini commented May 20, 2021

TheBlueMatt commented May 20, 2021

RCasatta commented May 20, 2021

sgeisler commented May 20, 2021

sgeisler commented May 20, 2021

RCasatta commented May 21, 2021

devrandom commented May 21, 2021

RCasatta commented May 21, 2021

afilini May 21, 2021 • edited Loading

Choose a reason for hiding this comment

afilini May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCasatta commented May 21, 2021 • edited Loading

TheBlueMatt commented May 21, 2021

TheBlueMatt commented May 21, 2021

devrandom commented May 21, 2021 • edited Loading

sgeisler commented May 21, 2021

RCasatta commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devrandom May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler commented May 21, 2021

RCasatta commented May 24, 2021

devrandom commented May 24, 2021

TheBlueMatt commented May 24, 2021

RCasatta commented May 25, 2021

apoelstra commented May 25, 2021

TheBlueMatt commented May 25, 2021 • edited Loading

RCasatta commented May 26, 2021

afilini May 21, 2021 •

edited

Loading

afilini May 21, 2021 •

edited

Loading

RCasatta commented May 21, 2021 •

edited

Loading

devrandom commented May 21, 2021 •

edited

Loading

devrandom May 24, 2021 •

edited

Loading

TheBlueMatt commented May 25, 2021 •

edited

Loading