docs: Clarify that callback can be called more than once

[real-or-random]

The tests in #1698 reminded me that some functions, e.g., secp256k1_ec_pubkey_cmp, may call the illegal callback more than once (see #1390 (comment) for more context). This PR clarifies the API docs to state explicitly that this is possible.

This is the simplest solution. Any production code should crash anyway if it encounters a callback. And in debug code or in our test code, it doesn't really matter whether you see an error message once or twice.

The alternative is to provide a guarantee that the callback is called only once. But that would make our code more complex for no good reason.

The second commit fixes a few typos, wording, and consistency.

[sipa]

concept ack

[real-or-random]

@stratospher Want to review this? :)

[stratospher]

ACK d279379. verified that illegal callback can be called more than once. and the simple route makes sense.

[stratospher]

• 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!

[real-or-random]

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.

[stratospher]

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)

2. 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?

[real-or-random]

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.

[stratospher]

Thank you for the awesome explanation! This helped me understand what the API is actually guaranteeing.

"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."

I like this wording better!

[real-or-random]

Ok nice, force-pushed to switch to (a variant of) this.

[josibake]

Concept ACK

While working on #1698, I was very confused when I started to see errors stating the illegal callback had been called more than once. Having some documentation to indicated what is allowed/expected would have been very helpful. Haven't had a chance to dig into the exact wording in this PR but generally agree we should have something.

[theStack]

Concept ACK

[theStack]

ACK d279379

[stratospher]

ACK 4d90585.

[theStack]

re-ACK 4d90585