--- Log opened Wed Mar 13 00:00:14 2019
00:08 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
00:17 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Excess Flood]
00:37 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
00:42 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 252 seconds]
02:07 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
03:11 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Excess Flood]
03:12 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
03:18 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Ping timeout: 256 seconds]
03:18 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined #secp256k1
05:16 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Ping timeout: 256 seconds]
05:23 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined #secp256k1
05:45 < real_or_random> hm I think there is a bug in the scratch API. when you create a scratch space, the error_callback is copied in the scratch. if you call set_error_callback later with a new callback, then call into ecmult and it fails to allocate, then the old callback is called
09:59 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Read error: Connection reset by peer]
10:08 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
10:12 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Read error: Connection reset by peer]
10:31 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
10:38 < andytoshi> gmaxwell: i'd like that, but i think we do still need to do a validity check before conversion to secp256k1_pubkey
10:38 < andytoshi> real_or_random: hmm, gross
10:38 < andytoshi> the lifetime of the scratch space is independent of the lifetime of the context object used to create it
10:39 < andytoshi> so it makes sense that you'd have to set both callbacks separately .. though there is no facility in the scratch space API for this .. and it's surprising/annoying from a user perspective
10:40 < andytoshi> i guess the scratch space should not be storing this callback at all
10:40 < andytoshi> the scratch space functions should take the callback as an arg, and the caller (which has a secp256k1_context* available in every case AFAIK) can give the callback then
10:42 < andytoshi> yeah, i see, from the POV of the user, the current situation is really surprising. it's unexpected that the scratch space would be maintaining its own callback.
10:42 < andytoshi> we can change this. it'd be user-invisible (other than fixing the 'bug')
11:00 < real_or_random> "the scratch space functions should take the callback as an arg, and the caller (which has a secp256k1_context* available in every case AFAIK) can give the callback then" yes I think that's how it should be
11:32 < real_or_random> argh no, so ... currently scratch spaces are only used for ecmult_multi, which is used nowhere. (so technically yes, the downstream caller has a secp256k1_context available in every case)
11:33 < real_or_random> but the ecmult_multi functions don't have a secp256k1_context but just the secp256k1_ecmult_context of course. that's somewhat annoying
11:49 < real_or_random> we could store a pointer to the secp256k1_context in the secp256k1_ecmult_pointer ... I'm not sure that this is the best thing to do though
11:49 < real_or_random> s/secp256k1_ecmult_pointer/secp256k1_ecmult_context
11:50 < real_or_random> this will set in stone that a secp256k1_ecmult_context cannot exist without a parent secp256k1_context. but given the callback issues, this seems to be the case anyway
11:53 < real_or_random> another option is to ignore that problem for now and first change the scratch API to be compatible with preallocated memory, too. this may make things easier. I think the annoying part currently is that the secp256k1_scratch_allocate_frame() allocates more memory if the current scratch space won't suffice. if we need to work with preallocated memory, this is not possible anyway.
11:54 < real_or_random> though it may still be useful for people who have malloc(). but still, there are certainly interdependencies between fixing the bug and extending the scratch spaces to preallocated memory
12:29 < real_or_random> andytoshi: anyway it seems somewhat strange atm because the scratch space keeps calling malloc() and free() for every multimultiplication (in allocate_frame and deallocate_frame). or is there a reason for this?
12:32 < real_or_random> I think it's cleaner to do the malloc() once when the scratch space is created, not only for efficiency. this is the point where the user commits to have memory of a certain size available. otherwise you can get random OOM failures when doing multiplications. I think it's better to get them only at startup time
12:37 < gmaxwell> +1 I assume that was actually the goal there, otherwise why even have a scratch space.
12:43 < real_or_random> indeed
12:54 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Read error: Connection reset by peer]
12:54 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
14:51 < andytoshi> it was the goal but there was some difficulty related to resizing that made this impossible
14:51 < andytoshi> real_or_random: ecmult_multi could take a callback as a parameter
14:52 < andytoshi> but rather, let's think again about changing the API to use a fixed slab
14:54 < andytoshi> oh, i see, this looks like we took a series of wrong turns, and actually we can use a fixed slab..
14:55 < andytoshi> initially we had an API that did some sort of resizing, on the theory that we maybe couldn't predict in advance how big an ecmult we might want to do
14:55 < andytoshi> then in https://github.com/bitcoin-core/secp256k1/pull/523 we replaced that with the current frame-based approach
14:56 < andytoshi> which was needed because there's a tension between ecmult_multi (which can use more-or-less arbitrary amounts of scratch space, but always wants to have more) and every other uses of the stack space (which are fixed, and if there's not enough memory then we fail)
14:56 < andytoshi> and as i recall there was a bunch of iteration on the size estimation functions
14:58 < andytoshi> but istm that the current API can easily be changed to use a fixed slab (actually i'll try to PR to do that now)
15:20 < andytoshi> done: https://github.com/bitcoin-core/secp256k1/pull/600
15:20 < andytoshi> i have no idea what i was thinking not doing this initially .. but i definitely did try and ran into some amount of trouble
15:21 < andytoshi> real_or_random: what do you think about this? should i actually change it to just take a slab to begin with, rather than mallocking?
15:22 < gmaxwell> Avoiding mallocs is good, but I do feel a bit iffy about forcing users into an interface where they have to manage the memory needed by a context object we create.
15:22 < gmaxwell> in embedded land thats mostly unavoidable.
15:23 < gmaxwell> But in desktop land dynamic language idiots will pass in the buffer then free it when they get the context. :P
15:23 < andytoshi> lol
15:23 < gmaxwell> (as an aside, add cookies to these data structures when you make slabs)
15:23 < andytoshi> it's actually nontrivial in micropython to not do this ... the fscking GC will go behind your back and reclaim any scratch space that you don't explicitly anchor to something it knows about
15:24 < gmaxwell> (e.g. just some 32/64 bit value at the beginning that you set when its initialize (value should be non-zero and odd ideally), zero when its freed, ... and check on entry before use)
15:24 < gmaxwell> Yeah, I really don't blame the users (much).
15:25 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection]
15:25 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined #secp256k1
15:26 < andytoshi> i can do that, though bear in mind this is an opaque data structure so it's pretty hard for the user to mess up its memory
15:27 < gmaxwell> andytoshi: well for example if you make it so the caller passes in the buffer instead of mallocing ourselves then it is REALLY easy to mess it up.
15:28 < andytoshi> yeah, definitely .. so ok, i see the value in always doing that, even if we add another API call for embedded users
15:32 < andytoshi> hmm, in these functions i don't have error callbacks available
15:32 < andytoshi> wonder how i should do this check
15:35 < gmaxwell> This is an ongoing question about how the failure callbacks should work.
15:35 < andytoshi> added a commit that just returns failure, in whatever way makes sense for each of the functions
15:36 < andytoshi> which isn't great but is probably better than the UB that wolud otherwise be caused
15:36 < andytoshi> (well, except that a cuople functions are assumed to succeed, and now they're no-ops, and this may itself cause UB..)
15:40 < gmaxwell> andytoshi: we should avoid creating cases where things like malloc failure return signature validation fails though.
15:41 < gmaxwell> (we don't want to promote programming gafs into consensus faults)
15:41 < gmaxwell> "I realize I lost my mind" and "this signature is false" are distinct failure conditions.
15:44 < andytoshi> yeah .. russell had this problem with simplicity
15:44 < andytoshi> but since we're not targeting embedded systems there (yet) i told him to just abort()
15:44 < andytoshi> abort() frustratingly is not available on embedded systems and will cause compile failures if you try to use it
15:45 < gmaxwell> yea, well on embedded systems you usually infinite loop or so some other similar platform specific thing (reset it) when you need an abort.
15:45 < gmaxwell> which was sort of the purpose of our callbacks: let the user do whatever makes sense in their enviroment.
15:47 < andytoshi> hm, so, actually here i can thread the error callbacks into these functions
15:47 < andytoshi> which is not too bad, it'll mostly just be annoying to change the unit tests
15:49 < andytoshi> gr, derp, `secp256k1_scratch_space_destroy` is in the public API and does not take a contex object
15:51 < sipa> i doubt anyone is using the scratch api yet
15:53 < andytoshi> we use it for bulletproofs in an open PR in secp-zkp .. and by extension the grin people are using it
15:53 < andytoshi> i'm not too worried about breaking this API tho
15:53 < andytoshi> it's pretty experimental
15:54 < gmaxwell> We shouldn't feel too worried about changing the libsecp256k1 api.
15:54 < gmaxwell> If the change will make it clearly better, do it.
15:55 < andytoshi> agreed
16:31 < andytoshi> heh, this was actually a bit invasive. had to also thread the error callback through ecmult_multi and its dispatch functions
16:31 < andytoshi>  9 files changed, 119 insertions(+), 102 deletions(-)
16:32 < andytoshi> but i'm happy with the result. pushed to #600
19:29 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 252 seconds]
19:34 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
20:17 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 252 seconds]
20:56 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
22:04 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 252 seconds]
22:06 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
22:10 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Excess Flood]
22:10 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #secp256k1
--- Log closed Thu Mar 14 00:00:15 2019