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

Support no-std #137

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Support no-std #137

merged 1 commit into from
Sep 26, 2023

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Aug 11, 2023

Should allow for no-std libraries to use rust-dlc

@benthecarman benthecarman changed the title Update ldk to v0.0.116 Support no-std Aug 11, 2023
@benthecarman
Copy link
Contributor Author

Doesn't seem to be fully working yet..

@benthecarman
Copy link
Contributor Author

building works correctly, can't seem to get it to work correctly with the correct dev-dependecies, not too sure what I am doing wrong

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Aug 15, 2023

building works correctly, can't seem to get it to work correctly with the correct dev-dependecies, not too sure what I am doing wrong

The dlc crate doesn't build for me (running cargo build --features=no-std). I get an error on miniscript.

@benthecarman
Copy link
Contributor Author

benthecarman commented Aug 15, 2023

Should be a lot closer now, just need to get dlc-manager working. There are a lot of test dependencies that rely on bitcoin/std so will need to figure out a way around that.

Looks like also unit test CI fails because we can't enable std and no-std together so the all-features flag doesn't work

@benthecarman
Copy link
Contributor Author

So seems the only way to get dlc-manager working with no-std would be to move the tests into another crate, which is kinda ugly.

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Aug 16, 2023

So seems the only way to get dlc-manager working with no-std would be to move the tests into another crate, which is kinda ugly.

I'm still having the same issue with miniscript in the dlc crate (which also seem to be the error that occur on CI). Is there more to it than running cargo build --features=no-std?

@benthecarman
Copy link
Contributor Author

I'm still having the same issue with miniscript in the dlc crate (which also seem to be the error that occur on CI). Is there more to it than running cargo build --features=no-std?

you need to disable default features as well

cargo build --no-default-features --features no-std

if that still doesn't work you might need to do a cargo clean

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Aug 16, 2023

you need to disable default features as well

cargo build --no-default-features --features no-std

Thanks that worked! But yeah not sure what's the best solution, having tests in a separate crate is definitely not great. Maybe some weird feature flag fu could do it?

@benthecarman benthecarman marked this pull request as ready for review September 3, 2023 15:41
@benthecarman
Copy link
Contributor Author

This should be ready after #145

dlc-manager doesn't have explicit support for no-std but not the end of the world, it should still work when used as a library.

Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Great work thanks! Just a couple of nits and to understand the reasons for some features.

dlc-trie/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +3 to +4
#[cfg(all(feature = "no-std", not(feature = "std")))]
extern crate hashbrown;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've never had to use this before but I couldn't get it to work without it. Seems like rust-miniscript does it as well so maybe it is just required for this crate?

dlc/Cargo.toml Outdated Show resolved Hide resolved
dlc-manager/Cargo.toml Outdated Show resolved Hide resolved
@benthecarman
Copy link
Contributor Author

Fixed the unnecessary features, good call out!

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Sep 6, 2023

Fixed the unnecessary features, good call out!

Actually seems like the rand-std feature of secp256k1-zkp is required to access the EcdsaAdaptorSignature::encrypt function because it uses thread_rng. Solution is either use an prng that works with no-std and use encrypt_with_rng, or use encrypt_no_aux_rand (ideally it'd be best to do that only for the no-std feature). I'm confused that this did not show up before.

@benthecarman
Copy link
Contributor Author

@Tibo-lg sorry totally missed your comment. I made it so when we are not std that it uses encrypt_no_aux_rand!

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Sep 22, 2023

@Tibo-lg sorry totally missed your comment. I made it so when we are not std that it uses encrypt_no_aux_rand!

Perfect! One last thing, could you apply this patch to cleanup unnecessary features? (I couldn't push to your branch)
8f5fe5e

Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Thanks!

@Tibo-lg Tibo-lg merged commit 7a45182 into p2pderivatives:master Sep 26, 2023
54 checks passed
@benthecarman benthecarman deleted the no-std branch September 26, 2023 08:29
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.

2 participants