--- Day changed Tue Jul 24 2018 01:19 -!- jtimon [~quassel@213.28.134.37.dynamic.jazztel.es] has quit [Ping timeout: 240 seconds] 04:13 -!- jtimon [~quassel@213.28.134.37.dynamic.jazztel.es] has joined #secp256k1 10:21 < andytoshi> in rust-bitcoin i'm getting "defined but not used" warnings on a bunch of ecmult_multi stuff ... and can confirm that this code is not actually used except in tests and benchmarks 10:21 < andytoshi> they're all static functions, not exported 10:22 < andytoshi> should i just live with it? add -Wunused-function ? 10:22 < andytoshi> erm, in rust-secp256k1, during the part of the build process that compiles libsecp256k1 10:24 < andytoshi> -Wno-unused-function i mean 10:25 < gmaxwell> there should be nothing defined and not used. 10:26 < gmaxwell> oh multi 10:26 < gmaxwell> well that shoudl get fixed. 10:26 < andytoshi> i think we merged the ecmult_multi stuff in anticipation of aggsig 10:26 < andytoshi> yeah 10:26 < gmaxwell> I think it can get moved under an ifdef verify. 10:27 < andytoshi> ifndef SECP256k1_BUILD maybe? 12:32 < andytoshi> https://github.com/trezor/trezor-core/issues/282 i'm working with trezor on libsecp integration, with the goal of having hw support for liquid. it sounds like they're interested in using libsecp in general 12:32 < andytoshi> but they're stuck on the use of malloc on secp256k1_context_create 12:32 < andytoshi> what do you think the best way forward on that is? 12:34 < gmaxwell> andytoshi: you replace the internal malloc function with something that just returns a pointer to a constant buffer. 12:34 < gmaxwell> I thought we'd had a comment explaining how to do that already, but I guess not. 12:36 < gmaxwell> e.g. char g_buffer[enough]; void *checked_malloc(...){return g_buffer;}. 12:36 < gmaxwell> All the memory 'allocation' is intended to be functionally static. 12:36 < andytoshi> well, we call checked_malloc a couple times during context_create, we'd need to increment g_buffer each time 12:36 < andytoshi> but sure 12:36 < gmaxwell> right. 12:37 < gmaxwell> probably we should change things so it's more obvious. 12:37 < andytoshi> we could support this directly with a compile flag 12:38 < gmaxwell> sure, in libopus it's -DNONTHREADSAFE_PSEUDOSTACK 12:39 < gmaxwell> https://github.com/xiph/opus/blob/master/celt/stack_alloc.h#L118 12:39 < gmaxwell> though an alternative would be to split the create calls into two steps -- a function that tells you the size you need, and the actual create.. and then make the caller do the allocation. But I think for non-embedded usage thats kinda lame. 12:40 < gmaxwell> For Opus we had constant problems with non-embedded people managing to enable the allocation free stuff, and then suffering concurrency problems. I think those problems stopped after we added the NONTHREADSAFE prefix. :P 12:41 < gmaxwell> but I think thats because for opus you need that to get rid of C99 var arrays, and MSVC users were getting warned about the var arrays so they flipped the switch. 12:43 < gmaxwell> andytoshi: it would simplify things further if we changed the create to just do one allocation for everything needed. 12:44 < andytoshi> yep, we could certainly do that 12:44 < gmaxwell> and an ifdef NOMALLOC_ONECONTEXT_NOTTHREADSAFE around that one line. 12:44 < andytoshi> though this won't really be extensible to the scratch space API 12:44 < gmaxwell> (and I suppose around the free) 12:44 < gmaxwell> well scratchspace API might be better off with the two step context creation where the caller finds the memory for us... 12:44 < gmaxwell> (I don't recall if thats already how it works) 12:45 < gmaxwell> in which case the trezor folks would know what to do. 12:45 < andytoshi> it doesn't work that way right now 12:45 < gmaxwell> Hm. Alternatively, we could have secp.._create_with_memory that does no alloc, and just ifdefs to compile out the versions that can malloc. 12:45 < andytoshi> right now it's actually super un-ergonomic for embedded use because i needed multiple frames that get allocated and deallocated multiple times 12:45 < gmaxwell> I kinda like that better because it avoids hiding mysterious global state in the library. 12:46 < andytoshi> my intention was to replace all of scratch_impl.h with something that used a stack pointer into a fixed space 12:46 < andytoshi> for embedded apps 12:46 < gmaxwell> but wouldn't force programmers for RealComputer to do two step context creation. 12:47 < gmaxwell> so e.g. you'd call secp256k1_context_size(flags) and get a size, then call secp256k1_context_create_mem() giving it a pointer to at least that much memory. 12:47 < andytoshi> yeah, that works 12:47 < gmaxwell> and we could still have the existing API too. 12:47 < andytoshi> yep 12:48 < gmaxwell> (and then to make sure no embedded deves are confused, wrap the existing create in a ifndef SECP256K1_NOMALLOC 12:48 < gmaxwell> ) 12:49 < gmaxwell> and the existing impl just becomes a wrapper on the two step. 12:50 < andytoshi> ok, i could give implementing that a shot 12:50 < andytoshi> hm, i wish matt was in the channel 12:51 < gmaxwell> From opus development I know that a lot of embedded developers would litterally grep the codebase, find malloc, then report back that opus can't work on their platform. 12:51 < andytoshi> yes, if you read the github htread i linked, trezor did exactly that to me 12:51 < andytoshi> "uses malloc, won't work" 12:52 < gmaxwell> Oh missed the link 12:52 < andytoshi> if you're willing to add some insight that'd be very appreciated, ideally we can nudge them toward using libsecp everywhere 12:53 < andytoshi> i asked them to replace checked_malloc, i assume they'll balk at modifying the lib, but if they had the 2-step API i bet that'd be totally fine (And it'd be easy to add a 2-step API to the scratch space as well) 12:54 < gmaxwell> I think it's probably better to just make the patch, rather than discuss it. 12:54 < gmaxwell> I feel kinda dumb also because I"m sure at one point I intended to write a comment near the actual malloc call to basically tell embedded developers how to handle it (how to do their job...), having gone through this with opus... and it seems I just never did it. 12:55 < gmaxwell> Yea, I didn't previously push for the 2-step API because its more error prone for desktop users. 12:55 < gmaxwell> but I don't think it occured to me that we could just have both. 12:55 < andytoshi> nice 12:56 < gmaxwell> (e.g. more error prone: someone doesn't make the size call, codes in a constant, and then things blow up on a system with different struct padding or type sizes) 12:58 < gmaxwell> yea, so the steps would be refactor to a single malloc for creation, then split the sizing into its own function and init into its own funcion, turn the create into a wrapper around the sizing/malloc/init. And export the sizing/init APIs. 12:59 < gmaxwell> maybe reasonable function names would be secp256k1_context_size(flags) and secp256k1_context_init(ctx, flags). 13:04 < gmaxwell> aside, another related change I wanted to add was canires on the context. E.g. have a void* at the beginning and end of the context that gets set to the aaddress of secp256k1_context_init xor 0xdeadbeef or something on init. and set to 0 on free. and have every entry that uses the context check the canary. .. to make use after free or other stupidity fail safer. 13:08 < andytoshi> yeah, definitely worth doing for use-after-free protection 13:43 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Excess Flood] 13:43 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1 15:06 -!- Cory [~Cory@unaffiliated/cory] has quit [Ping timeout: 264 seconds] 15:13 -!- Pasha [~Cory@unaffiliated/cory] has joined #secp256k1 15:15 -!- Pasha is now known as Cory 15:22 -!- molz [~IRCIdent@unaffiliated/molly] has joined #secp256k1 17:14 -!- jtimon [~quassel@213.28.134.37.dynamic.jazztel.es] has quit [Remote host closed the connection]