Implementing pre allocation context creation #116

elichai · 2019-05-28T17:52:49Z

Hi,
I want to implement bitcoin-core/secp256k1#566

this PR build on top of #115.

I see 2 ways to do it:

expose get_preallocated_size(), new_preallocated_context() functions to the user and add the reference to that buffer into the Context struct so rust won't allow him to write into it anymore.
just assume that if he can allocate data on the fly then he will Implement the https://doc.rust-lang.org/beta/std/alloc/trait.GlobalAlloc.html and just use liballoc instead. (and allocate ourself with Vec).

This is a first try on how 1 will look like (didn't write function docs and I hope I can find better function names haha)

elichai · 2019-05-28T17:55:43Z

I hoped I could leverage Verification and Signing and add a "parent" trait called Context that has the flags in it according to all the traits and then just use rust generics to identify if it's all/verification/signing and act accordingly.
Put this might be impossible in rust current state.

(tried combining traits: https://play.rust-lang.org/?gist=1c23c763ea2438eb620f51fbfb985515,
And also tried with Specialization(https://github.com/rust-lang/rfcs/blob/master/text/1210-impl-specialization.md) which didn't really work either and that's not yet stable anyway.)

elichai · 2019-06-11T14:12:33Z

Ok, I thought about it and I think the best way to do this is by testing how much memory each flag requires and then putting that on the stack and panic if the result of new_preallocated_context is bigger than that.
then every time we update the underlying libsecp the tests will crash if the context is now bigger and we can adjust the stack allocation accordingly.

(that way we can preserve the API)

What do you guys think? @apoelstra

real-or-random · 2019-06-12T10:42:44Z

Hm, the problem with that approach is that the size of the underlying context depends on the (C) platform for which secp256k1 has been compiled and the configuration settings (endo, etc.). Given what's currently in the context, we could reasonably guess an upper bound depending on configuration settings, etc. But that seems overly complicated.

What's the downside of adding get_preallocated_size()?

Is the underlying problem that you want to avoid the heap for no_std?

elichai · 2019-06-12T12:17:57Z

Yes. The underlying problem is to avoid heap allocation and give a fully no-std support.

In the current implementation I wrote there's a get_preallocated_size and then the user of the lib should give a mutable reference to a big enough buffer which is stored in the context struct (this prevents the user from mutating it while it's part of the context)

I just thought that it would be nicer to preserve API if we can put an upper bound and then put it on the stack, but if it's platform specific then it'll be overly complicated as you said.

real-or-random · 2019-06-12T13:07:12Z

Hm I see. Offering get_preallocated_size is simply pushing the problem towards the user who then can't use the heap either. The user may have a better understanding of the target platform though. But I think this really depends on your setting.

Maybe we can convince upstream to make secp256k1_context_preallocated_size a macro that expands to a compile time constant.

elichai · 2019-06-12T14:11:11Z

yeah, so the user might have a big scratch space available (either heap or stack) which he could use, or he might have the ability to do some weird allocations himself(see #119 for example) but it would be easier if we could solve it on the stack for the user.

if the whole size thing could've been constant it would be nice but I think there's some implementation difficulties and they're hoping to just make the Context itself constant and then they could get rid of it completely(and replace it with compilation flags for the size of the context)

apoelstra · 2019-06-12T15:26:10Z

For the Context trait you can do like
https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=4e65e1fdeb47bb1dce225589ab00228f

elichai · 2019-06-12T15:30:31Z

@apoelstra So you're suggesting to implement the trait directly on the structs/enums and not through the already existing traits, that's too bad but I guess that's all we can do.

Maybe i'll make another PR that replaces the structs with enums and adds this trait, I think it should be in a separate PR

apoelstra · 2019-06-12T15:31:27Z

Yeah, it kinda sucks but I think it's the correct thing to do actually - you can imagine some future context type has different flags but still implements the ordinary Signing and Verification traits.

real-or-random · 2019-06-12T23:33:08Z

yeah, so the user might have a big scratch space available (either heap or stack) which he could use, or he might have the ability to do some weird allocations himself(see #119 for example) but it would be easier if we could solve it on the stack for the user.

OTOH this would assumes that we have enough stack space and the user wants to do this on the stack. One typical use of no_std will be embedded devices where the user may indeed want to decide where in "which" memory the context is created. So I guess exporting the size function is okay for now.

vhnatyk · 2019-06-18T11:17:39Z

Hi! @real-or-random - given latest changes and my yesterday's "research" 😄 I had impression that all I need is to define lower 'ECMULT_WINDOW_SIZE' for static precompute tables. So I set in build.rs:58 .define("ECMULT_WINDOW_SIZE", Some("4")); but seems I'm still getting precompiled tables of 1.35 MiB size.

Then I even refreshed secp256k1 depend sources with latest and even tried to merge#337 -variable sized precomputed table for signing (where you are also reviewer) on top.

That's after both #115 and #116. And everywhere behavior is virtually the same - SEGFAULT at manual_alloc as I see in debug it tries to allocate too much mem. Probably I'm missing something obvious - but still unable launch it on mcu 😢

Here is the diff between this pr and my experimental branch

vhnatyk · 2019-06-18T11:20:13Z

@real-or-random -oh btw - in build.rs there is line .debug(true) in base_config . Is it ok?

real-or-random · 2019-06-18T12:42:29Z

@vhnatyk Hm weird, can you open a separate issue with more background?

How much RAM do you have?
Do you need verification, signing or both?
How do you know that manual_alloc is still really allocating of 1.35 MB? Have you checked the output of the size function? (Maybe the reason for the segfault is entirely different).

The debug(true) is a deliberate decision.

vhnatyk · 2019-06-18T19:06:43Z

I have 256Kb RAM and when I hardcode smaller values being passed to checked_malloc in the current master branch (though it fails few tests of course) - I'm able to pass checked_malloc (but it anyway fails on later steps pretty expected)
Not verification nor signing - just point and scalar math
With hardcoded values when I run the test in my app and analyze mem consumption in vlagrind massif (linux) and mtuner (on windows) it shows something like 2mb ram on master and ~500kb with hardcoded dummy array sizes (like secp256k1_ge prec[64]; instead of secp256k1_ge prec[1024])

I admit that it's a bit of black magik, but my current understanding of the subject doesn't let me generate any more useful info, unfortunately 😊

.gitignore

laanwj · 2019-06-23T20:03:32Z

build.rs

-               .define("ENABLE_MODULE_ECDH", Some("1"));
+               .define("ENABLE_MODULE_ECDH", Some("1"))
+               .define("USE_EXTERNAL_DEFAULT_CALLBACKS", Some("1"))
+               .define("ECMULT_WINDOW_SIZE", Some("15")); // This is the default in the configure file (`auto`)


ECmult window size needs to be configurable, 15 is definitely too large for smaller platforms
(though I don't know if rust has any way to pass values instead of feature bits?)

I don't think there is a way in rust to pass variables as compile time arguments. but i'll look into it, maybe there's some hack we can use (maybe environment variable?)

elichai · 2019-07-03T22:20:09Z

So after talking with apoelstra, it seems like the right way would be to split this PR to 2.

Remove all free/malloc and manage the memory ourself.
Enable a way for no-std users to supply their own memory buffer.

I'll implement that after #115 will get merged.
(maybe even another PR before to redo the trait flags stuff)

apoelstra · 2019-07-04T18:56:03Z

src/lib.rs

    ctx: *mut ffi::Context,
-    phantom: PhantomData<C>
+    phantom: PhantomData<C>,
+    _buf: Option<&'buf [u8]>,


I think unconditionally adding 'buf to the public API will make things too unergonomic for users who aren't providing their own buffer.

I think it'd be better to add a new set of context types SignOnlyBuffer<'buf> VerifyOnlyBuffer<'buf> AllBuffer<'buf> which tie the lifetime of the passed-in buffer to the context. Then let phantom deal with the borrowck and change the _buf field into buf: Option<*mut u8>.

hmm you prefer that no-std users will have a different Context struct? (If you provide no buffer you shouldn't feel the new lifetime at all(as a user, for the library maintainers you're right that it's unergonomic))

If we do go in that direction we'll need to abstract out the Context, so that sign/verify functions won't be implemented on the struct itself but on a generic of the trait (adding something like get_ptr() to the traits)

the nice thing about having &[u8] is that it's a fat pointer which makes it easier to clone (no need to recheck the size)

At the very least having 'buf in the API makes the documentation confusing, since users will see a lifetime but have no idea what it's for or how to use it.

I'm pretty sure sign/verify are already abstracted over the context, since e.g. both SignOnly and All have signing functionality.

apoelstra · 2019-07-04T18:56:52Z

src/lib.rs

        Secp256k1 {
            ctx: unsafe { ffi::secp256k1_context_clone(self.ctx) },
-            phantom: self.phantom
+            phantom: self.phantom,
+            _buf: None,
        }
    }
 }


...then I'd implement Clone only for the non-Buffer versions of Secp256k1.

apoelstra · 2019-07-04T18:57:51Z

src/lib.rs

-    pub fn new() -> Secp256k1<All> {
-        Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(ffi::SECP256K1_START_SIGN | ffi::SECP256K1_START_VERIFY) }, phantom: PhantomData }
+    pub fn new() -> Secp256k1<'buf, All> {
+        Secp256k1 { ctx: unsafe { ffi::secp256k1_context_create(Self::FLAGS) }, phantom: PhantomData, _buf: None }


As discussed on IRC, I'd like to allocate our own memory in Rust and then call the prealloc functions.

elichai · 2019-07-04T19:44:35Z

So A couple of design decisions we need to make:

how do we differentiate our allocations from user allocations? different structs or having some lifetime inclusion(by PhantomData or by slice) to the regular Context struct?
If we do separate the structs, how do we abstract out the sign/verify functions? have a trait to return a ffi::Context pointer?
If we need to clone ourself, how do we measure the size that we need to allocate? I think this can be solved with something like Implementing pre allocation context creation #116 (comment).

If we move the flags into a trait, and create a different struct for no-std, that's 2 new traits and a struct, are we ok with this? @apoelstra what are your thoughts?

apoelstra · 2019-07-04T19:50:43Z

I'd like to use different structs; it makes things more robust against future changes.
Yeah, we can use Self::get_preallocated_size() (so we'd need to rename that function to have the same name for all types).

I don't understand what you mean by "move the flags into a trait". Can you be more explicit?

Also while you're messing with structs can you make them all empty enums? (#117)

elichai · 2019-07-04T23:06:51Z

Closed in favor of #125

elichai mentioned this pull request Jun 10, 2019

missing symbols for no_std target #119

Closed

vhnatyk mentioned this pull request Jun 18, 2019

Dev/vhnat/new master elichai/rust-secp256k1#1

Closed

laanwj reviewed Jun 23, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

laanwj reviewed Jun 23, 2019

View reviewed changes

apoelstra reviewed Jul 4, 2019

View reviewed changes

elichai added 2 commits July 4, 2019 15:02

Added the preallocated FFI

2e782eb

Implementing the pre allocated context

776da69

elichai force-pushed the preallocated branch from 571c8eb to 776da69 Compare July 4, 2019 19:03

elichai marked this pull request as ready for review July 4, 2019 19:11

apoelstra mentioned this pull request Jul 4, 2019

Building for wasm doesn't work (well, building works but running does not) #124

Closed

elichai mentioned this pull request Jul 4, 2019

Implement pre allocation context creation #125

Merged

elichai closed this Jul 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing pre allocation context creation #116

Implementing pre allocation context creation #116

elichai commented May 28, 2019

elichai commented May 28, 2019

elichai commented Jun 11, 2019 •

edited

Loading

real-or-random commented Jun 12, 2019

elichai commented Jun 12, 2019

real-or-random commented Jun 12, 2019

elichai commented Jun 12, 2019

apoelstra commented Jun 12, 2019

elichai commented Jun 12, 2019

apoelstra commented Jun 12, 2019

real-or-random commented Jun 12, 2019

vhnatyk commented Jun 18, 2019 •

edited

Loading

vhnatyk commented Jun 18, 2019

real-or-random commented Jun 18, 2019

vhnatyk commented Jun 18, 2019 •

edited

Loading

laanwj Jun 23, 2019

elichai Jun 24, 2019

elichai commented Jul 3, 2019 •

edited

Loading

apoelstra Jul 4, 2019

elichai Jul 4, 2019

elichai Jul 4, 2019

elichai Jul 4, 2019

apoelstra Jul 4, 2019

apoelstra Jul 4, 2019

apoelstra Jul 4, 2019

elichai commented Jul 4, 2019 •

edited

Loading

apoelstra commented Jul 4, 2019

elichai commented Jul 4, 2019 •

edited

Loading

Implementing pre allocation context creation #116

Implementing pre allocation context creation #116

Conversation

elichai commented May 28, 2019

elichai commented May 28, 2019

elichai commented Jun 11, 2019 • edited Loading

real-or-random commented Jun 12, 2019

elichai commented Jun 12, 2019

real-or-random commented Jun 12, 2019

elichai commented Jun 12, 2019

apoelstra commented Jun 12, 2019

elichai commented Jun 12, 2019

apoelstra commented Jun 12, 2019

real-or-random commented Jun 12, 2019

vhnatyk commented Jun 18, 2019 • edited Loading

vhnatyk commented Jun 18, 2019

real-or-random commented Jun 18, 2019

vhnatyk commented Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Jul 3, 2019 • edited Loading

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

elichai commented Jul 4, 2019 • edited Loading

apoelstra commented Jul 4, 2019

elichai commented Jul 4, 2019 • edited Loading

elichai commented Jun 11, 2019 •

edited

Loading

vhnatyk commented Jun 18, 2019 •

edited

Loading

vhnatyk commented Jun 18, 2019 •

edited

Loading

elichai commented Jul 3, 2019 •

edited

Loading

elichai commented Jul 4, 2019 •

edited

Loading

elichai commented Jul 4, 2019 •

edited

Loading