Expand the amount of core-only #45

Ericson2314 · 2020-03-12T03:18:50Z

No description provided.

Ericson2314 · 2020-03-12T16:39:24Z

This fails because of missing impls with rust 1.22. How important is rust 1.22?

sgeisler

I haven't looked at all changes yet, the general idea sounds good to me. Discussing the minimum version again would just make this more complicated, so I'd rather use the patch to make the PR 1.22 compatible.

src/lib.rs

reuvenpo · 2020-04-07T08:25:28Z

If you set the minimum version of rust to 1.36 you can use the alloc crate with no_std and make only minimal changes to the crate, because it supports Vecs and other collection types that are used in this crate :)

Ericson2314 · 2020-04-07T15:03:07Z

@reuvenpo I want to use this on embedded systems like hardware wallets where dynamic allocation is highly discouraged. Not using the alloc crate we very intentional.

There is no reason we couldn't do both approaches and have an alloc feature, though.

reuvenpo · 2020-04-14T20:45:54Z

I want to use this on embedded systems like hardware wallets where dynamic allocation is highly discouraged

That's reasonable, if it were solved well this way we wouldn't mind using it in my own project :)

There is no reason we couldn't do both approaches

If it makes sense in the code, sure. My use case required just using bech32 in an environment that has alloc but not std, so i just whipped up #46 (which was the fastest way to achieve that) and we're using that for now. We won't push for this duality if it's a hassle, cause your solution would work well too.

Ericson2314 · 2021-07-15T20:02:05Z

I adapted this in light of the existing std feature.

tcharding · 2022-07-28T01:30:27Z

Hi @Ericson2314, I just put up a PR that aims to do more or less the same thing as this one, I did it without going back through open PRs so I missed this one. I don't want to seem like I was ignoring this so pinging you now. If you want to pick this work up I can close mine since you were first.

Ericson2314 · 2022-09-23T16:09:29Z

@tcharding no worries, I haven't touched this stuff in years. Mine does more things than yours, generalizing some interfaces, so why don't we just do yours first and then mine as follow-up?

Ericson2314 · 2022-09-23T18:50:20Z

@tcharding I rebased this on top as described, and polished it a bit more.

tcharding · 2022-09-30T00:06:57Z

Hey @Ericson2314, I'm not exactly sure what this PR is trying to achieve. "allow doing more things without alloc" doesn't give me quite enough context to evaluate the PR, I like the additional embedded test code but the base N stuff seems orthogonal, or am I missing something? Perhaps you could write a PR description justifying the proposed changes and if I'm correct in thinking the base N stuff is unrelated to the embedded test improvements perhaps split it out into a separate commit. Thanks

Ericson2314 · 2022-09-30T00:32:52Z

To be honest I forget all the details, but see the tests I've enabled, and the new ones I've written.

If your PR is merged first, it will be easier both for you all to read the code, and me to write the expanded description!

tcharding · 2023-03-01T01:40:23Z

Hey @Ericson2314, other PRs merged now so if you are still keen to work on this the path is clear.

apoelstra · 2023-03-01T15:27:49Z

src/lib.rs

-/// Interface to write `u5`s into a sink
-pub trait WriteBase32 {
+/// Interface to write `u(2*n)`s into a sink
+pub trait WriteBaseN<T>


In be4af63:

I find this rename very confusing. It looks like it's generic over the number base, but the write implementation still calls write_u5 ... which now no longer has anything to do with u5s.

I think we should leave this trait alone and make WriteBase256 completely independent (and probably it does not need the single write_u5 method since Rust has decent first-class support for byteslices).

Oops not renaming write_u5 was just an oversight.

I kept it one trait to I can have the polymorphic Vec and ArrayVec implementations, but I suppose I could just copy paste them for each trait/type pair.

I would suggest copy/pasting because I think u8 serialization is "just dumping bytes" and u5 serialization as a nontrivial conversion, so they feel like distinct things to me even though the code is pretty-much the same.

apoelstra · 2023-03-02T17:08:54Z

Looks like we require cargo clippy to pass. Just minor fixes needed.

Ericson2314 · 2023-03-22T03:03:25Z

That is done too. Should be all ready.

tcharding · 2023-03-22T22:09:43Z

Is the point of this PR to enable encoding a slice of u5s to bech32 without an allocator? If so I think this would be better solved by using iterators. The arrayvec dependency breaks MSRV so I don't see how we can use it. (I tried it out with Rust 1.48 as well.)

An update to the PR description and title would be super helpful please - took me a while to work out what was going on.

apoelstra · 2023-03-23T18:19:22Z

The arrayvec msrv looks like it's 1.51, which is not crazy far from our MSRV, and it's an optional dep which looks independent of the other alloc stuff. I think it'd be okay to say "if you enable this you con't use our MSRV".

apoelstra · 2023-03-23T18:31:04Z

So, I'd be ready to ACK this PR, but I find @tcharding idea of replacing these Write traits with an iterator (which maintains only a small amount of state, and a pointer to its input) intriguing. @Ericson2314 does this seem plausible to you?

I'm okay with keeping an optional arrayvec dep but if we could avoid it that'd be even better.

tcharding · 2023-03-23T21:34:11Z

To be totally transparent I got this idea from #60 (posted by @Kixunil) but I have not worked out exactly what it means to implement it. Am happy to learn more about iterators to do it but equally happy to see it done and learn by reviewing it :)

Kixunil · 2023-03-24T06:55:07Z

embedded/no-allocator/Cargo.toml

-bech32 = { path = "../../", default_features = false }
+codegen-units = 1 # better optimizations
+debug = true # symbols are nice and they don't increase the size on Flash
+lto = true # better optimizations


Why are these needed? This is just a test, I expect the compilation time to be longer than the run time.

I think I was just copying what the other test did (or used to do) to try to go with the flow.

Maybe they are intended for benches? Those should have [profile.release] above these settings.

Kixunil · 2023-03-24T06:58:36Z

src/lib.rs

+/// Like `std::io::Writer`, but because the associated type is no_std compatible.
+pub trait WriteBase256 {
+    /// Write error
+    type Err: fmt::Debug;


Most of the Rust ecosystem names associated error types Error, the only exception being ancient FromStr. So I'd prefer that name.

Kixunil · 2023-03-24T06:59:56Z

src/lib.rs

+    }
+
+    /// Write a single `u8`
+    fn write_u8(&mut self, data: u8) -> Result<(), Self::Err> { self.write(&[data]) }


This is infinite recursion by default, I think it's a foot-gun and this method should be required instead.

I think it's fine, it's a pretty common idiom for a trait to have infinite recursion by default, so that you can implement either one of the methods. If you try to run it, it will crash immediately (actually, I think the compiler will even flag it).

I still don't like it :D

But sure, if there is no better way...

Kixunil · 2023-03-24T07:02:20Z

src/lib.rs

+    Bech32Error(Error),
+    /// Error from `arrayvec`.
+    CapacityError(CapacityError),
+}


I think WriteError would be a clearer name.

Kixunil · 2023-03-24T07:08:00Z

src/lib.rs

+            .iter()
+            .map(|x| u5::try_from(*x).map(|_| ()).map_err(Error::TryFrom))
+            .find(|r| r.is_err())
+            .unwrap_or(Ok(()))


This looks like a really convoluted way of checking validity. Where is this needed?

I think it's exactly as complex as you'd expect for a function that merely checks whether a string contains only valid bech32 characters.

But I agree, the utility of a function that checks this without actually doing the conversion seems unclear to me.

Ericson2314 · 2023-03-29T13:40:05Z

Would it be possible to merge this and then take a stab at the iterators approach? Would like to do it, but I am worried about finding the time for another "creative" investigation, vs the more tasks that are the other review items. I agree it does sound like a good idea.

apoelstra · 2023-03-29T14:27:35Z

@Ericson2314 for sure, let's just merge this for now. I don't think we have a release of this crate coming up anytime soon, and this PR is blocking some other things.

But could you first fix the following minor comments from Kix?:

remove the new lto = true stuff from embedded/no-allocator/Cargo.toml
rename CapacityError to WriteError

Then later we can explore the iterator thing and revisit the check_base32 API.

Ericson2314 · 2023-03-29T17:42:33Z

No problem @apoelstra, happy to do all those things. Thank you for agreeing to defer the iterator exploration.

Ericson2314 · 2023-03-29T18:08:53Z

I renamed CapacityError as requested, as to the lto stuff. C.f. with-allocator. I tried to make it more like before (not just the lto thing but in general), but that didn't work so well when the goal is to actually run the no-std binary.

Right now, with-allocator and no-allocator should be set up exactly the same, and I recommend if we do get rid of the lto thing for one of them, we should get rid of it for both of them.

apoelstra · 2023-03-29T18:15:35Z

Lol, CI is failing, something to do with LTO ..... maybe there was a reason for those flags.

tcharding

ACK 4b0c640

Ericson2314 · 2023-03-30T01:44:18Z

Yeah CI wasn't actually running the thing as I intended before the previous push.

We'll see if I fixed it. Also added a new readme for both weird ARM tests.

Ericson2314 · 2023-03-30T17:03:32Z

Yay OK the last commit did fix everything!

tcharding

ACK 42b3446

tcharding · 2023-03-30T22:02:48Z

Thanks for sticking with it man.

apoelstra · 2023-03-31T14:04:35Z

This is super cool! I didn't know how to use qemu in CI before. We should use this to get a BE system to test rust-secp against.

apoelstra

ACK 42b3446

apoelstra · 2023-03-31T14:34:15Z

embedded/no-allocator/memory.x

@@ -0,0 +1 @@
+../with-allocator/memory.x


Sorry, we can't accept symlinks. Our merge script (which we borrow from Bitcoin Core) will not accept them, even if they point within the repo.

OK I'll just copy the file then no problem.

Co-Authored-By: Sebastian <geisler.sebastian@googlemail.com>

apoelstra

ACK 32c87da

apoelstra · 2023-03-31T20:37:01Z

Hooray! That time it was accepted.

Thanks for your contribution @Ericson2314 and glad to have you poking at these projects!

Ericson2314 · 2023-04-01T03:00:16Z

And thanks everyone too for helping me get this over the finish line!

tcharding · 2023-04-03T01:36:22Z

Nice one man, thank you.

sgeisler reviewed Mar 13, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

reuvenpo mentioned this pull request Apr 7, 2020

Add no_std support for crates using compiler versions >1.36 #46

Closed

Ericson2314 changed the title ~~Create std feature for core-only users --- contains #43~~ Create alloc feature for core-only users --- contains #43 Jul 15, 2021

tcharding mentioned this pull request Jul 28, 2022

Add "alloc" feature #71

Merged

Ericson2314 force-pushed the std-feature branch from f2d8845 to 24d76fc Compare September 23, 2022 16:08

Ericson2314 force-pushed the std-feature branch from 24d76fc to bdf7f70 Compare September 23, 2022 18:46

Ericson2314 changed the title ~~Create alloc feature for core-only users --- contains #43~~ Expand the amount of core-only --- contains #43 & #71 Sep 23, 2022

Ericson2314 force-pushed the std-feature branch from bdf7f70 to 79cb499 Compare September 24, 2022 16:59

Ericson2314 force-pushed the std-feature branch from 79cb499 to 321a674 Compare March 1, 2023 05:02

Ericson2314 changed the title ~~Expand the amount of core-only --- contains #43 & #71~~ Expand the amount of core-only Mar 1, 2023

Ericson2314 force-pushed the std-feature branch 2 times, most recently from 99ad259 to be4af63 Compare March 1, 2023 05:10

apoelstra reviewed Mar 1, 2023

View reviewed changes

Ericson2314 force-pushed the std-feature branch 3 times, most recently from a254de1 to 30028bf Compare March 1, 2023 21:03

tcharding mentioned this pull request Mar 3, 2023

Improve "infrastructure" crate wide #88

Merged

Ericson2314 force-pushed the std-feature branch from 30028bf to 5299cc1 Compare March 8, 2023 17:10

Ericson2314 force-pushed the std-feature branch from cd68bae to 5021694 Compare March 22, 2023 02:46

Kixunil suggested changes Mar 24, 2023

View reviewed changes

Ericson2314 force-pushed the std-feature branch from 5021694 to 4b0c640 Compare March 29, 2023 18:04

tcharding approved these changes Mar 29, 2023

View reviewed changes

Ericson2314 force-pushed the std-feature branch from 4b0c640 to 42b3446 Compare March 30, 2023 01:43

tcharding approved these changes Mar 30, 2023

View reviewed changes

apoelstra approved these changes Mar 31, 2023

View reviewed changes

apoelstra reviewed Mar 31, 2023

View reviewed changes

Allow doing more things without alloc

32c87da

Co-Authored-By: Sebastian <geisler.sebastian@googlemail.com>

Ericson2314 force-pushed the std-feature branch from 42b3446 to 32c87da Compare March 31, 2023 19:53

apoelstra approved these changes Mar 31, 2023

View reviewed changes

apoelstra merged commit d566ec4 into rust-bitcoin:master Mar 31, 2023

Ericson2314 deleted the std-feature branch April 1, 2023 02:59

		@@ -0,0 +1 @@
		../with-allocator/memory.x

Expand the amount of core-only #45

Expand the amount of core-only #45

Conversation

Ericson2314 commented Mar 12, 2020

Ericson2314 commented Mar 12, 2020

sgeisler left a comment • edited Loading

Choose a reason for hiding this comment

reuvenpo commented Apr 7, 2020

Ericson2314 commented Apr 7, 2020 • edited Loading

reuvenpo commented Apr 14, 2020 • edited Loading

Ericson2314 commented Jul 15, 2021

tcharding commented Jul 28, 2022 • edited Loading

Ericson2314 commented Sep 23, 2022

Ericson2314 commented Sep 23, 2022

tcharding commented Sep 30, 2022

Ericson2314 commented Sep 30, 2022

tcharding commented Mar 1, 2023

Choose a reason for hiding this comment

Ericson2314 Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Mar 2, 2023

Ericson2314 commented Mar 22, 2023

tcharding commented Mar 22, 2023

apoelstra commented Mar 23, 2023

apoelstra commented Mar 23, 2023

tcharding commented Mar 23, 2023

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

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

Ericson2314 commented Mar 29, 2023

apoelstra commented Mar 29, 2023

Ericson2314 commented Mar 29, 2023

Ericson2314 commented Mar 29, 2023

apoelstra commented Mar 29, 2023

tcharding left a comment

Choose a reason for hiding this comment

Ericson2314 commented Mar 30, 2023

Ericson2314 commented Mar 30, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Mar 30, 2023

apoelstra commented Mar 31, 2023

apoelstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 31, 2023

Ericson2314 commented Apr 1, 2023

tcharding commented Apr 3, 2023

sgeisler left a comment •

edited

Loading

Ericson2314 commented Apr 7, 2020 •

edited

Loading

reuvenpo commented Apr 14, 2020 •

edited

Loading

tcharding commented Jul 28, 2022 •

edited

Loading

Ericson2314 Mar 1, 2023 •

edited

Loading