Introduce callback functions for dealing with errors

sipa commented at 8:30 pm on July 18, 2015: contributor

sipa force-pushed on Jul 18, 2015

sipa commented at 9:54 pm on July 18, 2015: contributor

Made some changes.

sipa force-pushed on Jul 19, 2015

apoelstra commented at 6:59 pm on July 21, 2015: contributor

Maybe when VERIFY is defined, the callback should be called even when the condition passes (with some sort of “success” flag, or a different callback altogether). What I’d like is for the test binary to be able to detect if an ARG_CHECK is defined but never gets tripped.

sipa commented at 9:47 pm on July 24, 2015: contributor

Will rebase/update this after #282.

Introduce callback functions for dealing with errors. 995c548771

sipa force-pushed on Jul 26, 2015

sipa commented at 4:11 pm on July 26, 2015: contributor

Rebased.

I haven’t incorporated @apoelstra’s change. I like the idea (I think an extra callback entry in the context is best), but I’m not exactly sure about the implementation. How would you identify a particular ARG_CHECK? Would they need a number you can test for?

apoelstra commented at 4:48 pm on July 26, 2015: contributor

I’m not sure. I don’t want to require a number for each one because that’d be awful to maintain and result in a lot of boilerplate code.

I think using the stringification of the ARG_CHECK should be sufficient. This is also a maintenance burden if specific checks are ever referred to, but I don’t think this will happen. To detect an untriggered check it suffices to maintain an array (hash table, I guess :/, if the checks are identified by string) of number-of-times-triggered counts for each check.

sipa commented at 2:47 pm on July 28, 2015: contributor

@apoelstra I think all of that can wait. Being able to catch errors (rather than their absence) from the tests is much easier and useful on itself.

in include/secp256k1.h: in 995c548771

82+ *  such mistakes, and the default (crashing) may be inadvisable.
83+ *  When this callback is triggered, the API function called is guaranteed not
84+ *  to cause a crash, though its return value and output arguments are
85+ *  undefined.
86+ */
87+void secp256k1_context_set_illegal_callback(

apoelstra commented at 8:59 pm on July 28, 2015:

I think you should comment that these two functions are not threadsafe, since everything else in the library is (assuming you lock your contexts appropriately).

Alternately, we could gain threadsafety by storing these in the secp256k1 contexts. I think this is a bad idea since it’d clutter up the API and almost nobody will be using these functions, just throwing it out there.

Edit: Oh, I see, these things are part of the context. I think they should appear in secp256k1_context_create (with NULL being mapped to the defaults).

sipa commented at 9:40 pm on July 28, 2015:

Adding all properties of a context to their constructor is not really feasible…

apoelstra commented at 10:00 pm on July 28, 2015:

My comment was based on a misunderstanding anyway. Sorry.

apoelstra commented at 10:00 pm on July 28, 2015: contributor

Tested ACK, bearing in mind that the new functionality is not actually (yet) tested.

sipa merged this on Jul 29, 2015

sipa closed this on Jul 29, 2015

sipa referenced this in commit ae4f0c6eec on Jul 29, 2015

Introduce callback functions for dealing with errors #278