Skip to content

docs: Clarify that callback can be called more than once #1727

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

docs: Clarify that callback can be called more than once #1727

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b64774
docs: Clarify that callback can be called more than once
real-or-random Aug 20, 2025
d279379
doc: Improve API docs of _context_set_illegal_callback
real-or-random Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions include/secp256k1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ SECP256K1_DEPRECATED("Use secp256k1_context_static instead");
* secp256k1_context_create (or secp256k1_context_preallocated_create), which will
* take care of performing the self tests.
*
* If the tests fail, this function will call the default error handler to abort the
* If the tests fail, this function will call the default error callback to abort the
* program (see secp256k1_context_set_error_callback).
*/
SECP256K1_API void secp256k1_selftest(void);
Expand Down Expand Up @@ -336,34 +336,35 @@ SECP256K1_API void secp256k1_context_destroy(
*
* The philosophy is that these shouldn't be dealt with through a
* specific return value, as calling code should not have branches to deal with
* the case that this code itself is broken.
* the case that this code itself is broken. On the other hand, during debug
* stage, one would want to be informed about such mistakes, and the default
* (crashing) may be inadvisable.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • When this callback is triggered, the API function called is guaranteed not
  • to cause a crash, though its return value and output arguments are
  • undefined.

when I initially read the comments on master, I assumed "API not crashing guarantee" is only for debug stage (no abort call) since they were in 1 para. but after seeing this diff just realised that crashing in a callback is acceptable and still a stable API regardless of debug/production build. so I think the comments made it clearer for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, paragraph breaks matter. :)

Though I think it is relatively clear even on master. When the callback calls abort, then the entire program will crash immediately. And from that point on, any guarantee you get from the API is vacuous.

Copy link
Contributor

@stratospher stratospher Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ok that makes sense. but now I'm confused. It's just a line break difference but I interpreted the meaning before and after as:

  1. before:
*  The philosophy is that these shouldn't be dealt with through a
 *  specific return value, as calling code should not have branches to deal with
 *  the case that this code itself is broken.
 *
 *  On the other hand, during debug stage, one would want to be informed about
 *  such mistakes, and the default (crashing) may be inadvisable.
 *  When this callback is triggered, the API function called is guaranteed not
 *  to cause a crash, though its return value and output arguments are

API function called is guaranteed not to cause a crash in debug stage (in tests where it's mostly the callback function counting_callback_fn that just counts and no abort)

  1. Now:
*  The philosophy is that these shouldn't be dealt with through a
 *  specific return value, as calling code should not have branches to deal with
 *  the case that this code itself is broken. On the other hand, during debug
 *  stage, one would want to be informed about such mistakes, and the default
 *  (crashing) may be inadvisable.
 *
 *  When this callback is triggered, the API function called is guaranteed not
 *  to cause a crash, though its return value and output arguments are
 *  undefined. A single API call may trigger the callback more than once.

API function called is guaranteed not to cause a crash in any stage (tests or IRL)
=> because it's the callback doing the crash + handling invalid user inputs + user responsibility and so API is stable?

do we want 1 or 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, I think the guarantee we want to provide is:
"An API function called is guaranteed not to crash after the callback returns."

This guarantee holds no matter what the callback function is set to, which corresponds to what you called "in any "stage". Now, of course, this guarantee doesn't say anything if the callback itself causes a crash (and the default callback does) because then the callback never returns, so there's no "after the callback returns".

Now that I've written this, I'm not sure what this actually means. In simple cases, the only thing the API function does after the callback returns is to return 0. This shouldn't crash. But there are more complex cases:

  • Some functions try to read more arguments from memory, e.g., secp256k1_ec_pubkey_cmp tries to read the memory at pubkey1 even after an invalid pubkey0 triggered the callback. This may crash if pubkey1 points to an invalid memory region.
  • Some functions write to their output arguments after the callback has returned (see Initialization of output args #1736 for another rabbit hole...).

Perhaps what we should want to say here is something like this: "An API function called is guaranteed not to crash after the callback returns (unless the crash is unrelated to the violation that triggered the callback)". But this is imprecise and ugly.

Perhaps we should drop this sentence entirely. It creates more confusion than it resolves. What about this?

"Should the callback return, the return value and output arguments of the API function call are undefined. Moreover, the same API call may trigger the callback again in this case."

Note that all of this is about API guarantees, so the audience is API users. As a result, "debug stage" means "debug stage of a program using libsecp256k1". (And as a side remark, note that the callbacks can be set at runtime, so strictly speaking, this is not (necessarily) about debug vs. release builds.) So if you develop a program (an application or another library) that uses the libsecp256k1 API, you may want to set a callback that does not crash for debugging purposes. I'm not entirely sure how useful this functionality is to API users, but this is what the docs here describe.

In practice, where this is useful is exactly in our internal tests, as you correctly point out. But what I'm trying to say is that these are our internal tests, where we may do everything because we control the implementation of the API functions. So we could also have an internal way of setting a counting callback that is not exposed through the public API.

If you ask me, the main purpose of setting callback functions is that you can control how the library crashes in production code. Maybe in your application, you don't want abort() but a more graceful termination. Or you can't use the default callback because it writes an error message to stdout but you don't have stdout because you're developing for a hardware wallet. (In the latter case, setting callbacks at runtime won't help you because the program won't compile in the first place. We've added compile-time overrides for this case.)

And on a last note, I don't think all of this is optimally designed. I believe we'd do some things differently if we had a chance to start from scratch.

*
* On the other hand, during debug stage, one would want to be informed about
* such mistakes, and the default (crashing) may be inadvisable.
* When this callback is triggered, the API function called is guaranteed not
* to cause a crash, though its return value and output arguments are
* undefined.
* undefined. A single API call may trigger the callback more than once.
*
* When this function has not been called (or called with fn==NULL), then the
* default handler will be used. The library provides a default handler which
* writes the message to stderr and calls abort. This default handler can be
* When this function has not been called (or called with fun==NULL), then the
* default callback will be used. The library provides a default callback which
* writes the message to stderr and calls abort. This default callback can be
* replaced at link time if the preprocessor macro
* USE_EXTERNAL_DEFAULT_CALLBACKS is defined, which is the case if the build
* has been configured with --enable-external-default-callbacks. Then the
* following two symbols must be provided to link against:
* - void secp256k1_default_illegal_callback_fn(const char *message, void *data);
* - void secp256k1_default_error_callback_fn(const char *message, void *data);
* The library can call these default handlers even before a proper callback data
* The library may call a default callback even before a proper callback data
* pointer could have been set using secp256k1_context_set_illegal_callback or
* secp256k1_context_set_error_callback, e.g., when the creation of a context
* fails. In this case, the corresponding default handler will be called with
* fails. In this case, the corresponding default callback will be called with
* the data pointer argument set to NULL.
*
* Args: ctx: pointer to a context object.
* In: fun: pointer to a function to call when an illegal argument is
* passed to the API, taking a message and an opaque pointer.
* (NULL restores the default handler.)
* data: the opaque pointer to pass to fun above, must be NULL for the default handler.
* (NULL restores the default callback.)
* data: the opaque pointer to pass to fun above, must be NULL for the
* default callback.
*
* See also secp256k1_context_set_error_callback.
*/
Expand All @@ -380,18 +381,19 @@ SECP256K1_API void secp256k1_context_set_illegal_callback(
* to abort the program.
*
* This can only trigger in case of a hardware failure, miscompilation,
* memory corruption, serious bug in the library, or other error would can
* otherwise result in undefined behaviour. It will not trigger due to mere
* memory corruption, serious bug in the library, or other error that would
* result in undefined behaviour. It will not trigger due to mere
* incorrect usage of the API (see secp256k1_context_set_illegal_callback
* for that). After this callback returns, anything may happen, including
* crashing.
*
* Args: ctx: pointer to a context object.
* In: fun: pointer to a function to call when an internal error occurs,
* taking a message and an opaque pointer (NULL restores the
* default handler, see secp256k1_context_set_illegal_callback
* default callback, see secp256k1_context_set_illegal_callback
* for details).
* data: the opaque pointer to pass to fun above, must be NULL for the default handler.
* data: the opaque pointer to pass to fun above, must be NULL for the
* default callback.
*
* See also secp256k1_context_set_illegal_callback.
*/
Expand Down