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

Expand the amount of core-only #45

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

Ericson2314
Copy link
Contributor

No description provided.

@Ericson2314
Copy link
Contributor Author

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

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@reuvenpo
Copy link

reuvenpo commented Apr 7, 2020

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
Copy link
Contributor Author

Ericson2314 commented Apr 7, 2020

@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
Copy link

reuvenpo commented Apr 14, 2020

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 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
@Ericson2314
Copy link
Contributor Author

I adapted this in light of the existing std feature.

@tcharding
Copy link
Member

tcharding commented Jul 28, 2022

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
Copy link
Contributor Author

@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 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
Copy link
Contributor Author

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

@tcharding
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@Ericson2314 Ericson2314 changed the title Expand the amount of core-only --- contains #43 & #71 Expand the amount of core-only Mar 1, 2023
@Ericson2314 Ericson2314 force-pushed the std-feature branch 2 times, most recently from 99ad259 to be4af63 Compare March 1, 2023 05:10
src/lib.rs Outdated
/// 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>
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@Ericson2314 Ericson2314 Mar 1, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

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

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

@Ericson2314
Copy link
Contributor Author

That is done too. Should be all ready.

@tcharding
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 :)

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
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

src/lib.rs Outdated
/// Like `std::io::Writer`, but because the associated type is no_std compatible.
pub trait WriteBase256 {
/// Write error
type Err: fmt::Debug;
Copy link

Choose a reason for hiding this comment

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

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

src/lib.rs Outdated
}

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

I still don't like it :D

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

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

Choose a reason for hiding this comment

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

I think WriteError would be a clearer name.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

@Ericson2314
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 4b0c640

@Ericson2314
Copy link
Contributor Author

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
Copy link
Contributor Author

Yay OK the last commit did fix everything!

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 42b3446

@tcharding
Copy link
Member

Thanks for sticking with it man.

@apoelstra
Copy link
Member

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.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 42b3446

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 32c87da

@apoelstra apoelstra merged commit d566ec4 into rust-bitcoin:master Mar 31, 2023
@apoelstra
Copy link
Member

Hooray! That time it was accepted.

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

@Ericson2314 Ericson2314 deleted the std-feature branch April 1, 2023 02:59
@Ericson2314
Copy link
Contributor Author

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

@tcharding
Copy link
Member

Nice one man, thank you.

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.

6 participants