nonce: terminate RFC6979 loop at UINT_MAX

[l0rinc]

Problem: The public nonce callback accepts UINT_MAX, but nonce_function_rfc6979_impl never returns for that attempt.
Its i <= counter loop wraps after the final candidate and starts again.

Fix: Generate the candidate before checking whether it is the requested attempt.
This preserves the result for every unsigned int attempt, including UINT_MAX, and exits before the index can wrap.

[mnfadel]

tested ACK 7b47f1f

Reproduced the hang and confirmed this fixes it.

On master (8c3e6e6), calling the callback directly with UINT_MAX never returns:

ret = secp256k1_nonce_function_rfc6979(nonce, msg32, key32, NULL, NULL, UINT_MAX);
counter=0        -> returned 1
counter=5        -> returned 1
counter=UINT_MAX -> calling...
exit=124        # process killed by timeout(1)
With this PR applied, the same program returns immediately:

counter=UINT_MAX -> returned 0
exit=0

tests and exhaustive_tests both pass.

The mechanism is as described: i is unsigned int, so i <= counter is a tautology when counter == UINT_MAX, and i++ wraps 4294967295 → 0.

One question on the chosen semantics. The public callback type accepts any unsigned int, so returning 0 narrows the documented domain rather than making the loop terminate. Would something along the lines of

for (i = 0; ; i++) {
    secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
    if (i == counter) break;
}

be preferable, since it terminates for every input including UINT_MAX and keeps the callback total? I don't feel strongly — rejecting is simpler, and the value is unreachable through ordinary signing — but it seemed worth asking whether narrowing the API contract is the intent here.

Two minor points:

The early return happens before nonce32 is written, so callers must not read the output when the callback returns 0. That is the existing contract for a 0 return, but it may be worth stating explicitly in the header docs alongside this change.
tests.c now depends on UINT_MAX; worth confirming limits.h is included directly rather than transitively.
No impact on RFC 6979 compliance — no standard test vector approaches that counter value.

[real-or-random]

Nice find. I guess that fix is technically speaking fine. But I wonder if it's the right fix, or if it even matters.

But have you actually tried running it with UINT_MAX - 1 (=2^32 - 2 on a typical machine)? HMAC takes ~0.5 ms on my laptop. With a back of an envelope estimation, this means that, if I'm not mistaken, calling secp256k1_nonce_function_rfc6979 with counter = UINT_MAX - 1 will take 2^32 * 0,5 ms = ~ 600 h on my laptop before it terminates.

Even absurder: As far as I can tell, every call in the loop reperforms the computations of the previous calls. So our implementation of RFC6979 signing is technically quadratic, so we end up making 2^64 HMAC computations in the "worst case".

But none of this is a problem. The probability that we even hit counter = 1 is already negligible.

To me, this sounds like, we should rather restrict the counter value to a very low number or even 0. Noone should notice even though that's technically speaking an API break (because an API user could call the nonce function directly).

[mnfadel]

But have you actually tried running it with UINT_MAX - 1

Measured it rather than estimating. secp256k1_nonce_function_rfc6979 runs its generate loop counter + 1 times, so wall time is linear in counter:

     counter    elapsed (s)  per-iteration (ns)
        1000       0.009762              9752.5
       10000       0.040107              4010.3
      100000       0.325197              3251.9
     1000000       3.083315              3083.3
     2000000       5.843862              2921.9

The falling per-iteration figure is fixed call overhead amortising away; the marginal cost converges to roughly 2.9 us.

So one iteration costs ~3 us rather than ~0.5 ms, which puts counter = UINT_MAX - 1 at approximately 3.5-4 hours, not ~600. This is WSL2 on a laptop against a default ./configure build, so please read it as an order of magnitude rather than a precise figure.

That doesn't change your conclusion, though: nobody is going to wait 3.5 hours to produce a nonce, so large counters are pathological whether the number is 4 hours or 600.

On the quadratic point - confirmed, and in closed form: a signing loop that reached counter n performs

sum_{i=0}^{n} (i + 1)  =  (n+1)(n+2)/2

iterations in total, since each attempt regenerates all of its predecessors. At the measured ~3 us that is ~1.5 s for n = 1000, and ~2^63 iterations for n = UINT_MAX, i.e. on the order of 10^6 years.

One observation that may bear on which fix is the right one: RFC 6979 has no counter. Section 3.2 step (h) specifies that when a candidate k is rejected you

K = HMAC_K(V || 0x00)
V = HMAC_K(V)

and loop to generate a new T. The generator is stateful and resumable - you continue it, rather than re-deriving from an index. So the quadratic behaviour isn't inherent to RFC 6979; it comes from exposing retries through a stateless callback that takes a counter and rebuilds the entire chain on every call.

Which is a further argument for your suggestion of restricting the counter to a very small value, beyond it being unreachable in practice: the counter isn't an RFC 6979 concept to begin with, and it is the thing producing both the non-termination edge case and the quadratic cost. Bound it tightly and both stop existing, rather than needing to be special-cased.

[real-or-random]

Let's ignore the long running times for a moment. This PR is pedantic (and that's good). But I think if we want to be pedantic, then this is the right fix:

    i = 0;
    do {
        secp256k1_rfc6979_hmac_sha256_generate(hash_ctx, &rng, nonce32, 32);
        i++;
        if (i == 0) {
            break;  /* overflow */
        }
    } while (i <= counter);

The i == 0 could be added to the while of course, but I think this one is more readable.

(This is hard to get right; I hope I got it right. :D)

edit: Oh, I had not noticed that the AI slop above suggested a similar thing.

[mnfadel]

(This is hard to get right; I hope I got it right. :D)

Verified rather than eyeballed - it's correct.

Checking it at 32 bits means 2^32 iterations, so I modeled the identical logic with a uint8_t counter instead, where the wrap boundary is 256 iterations away and the unsigned overflow semantics are the same:

  counter       want        current      term?       proposed    term?
---------------------------------------------------------------------
        0          1              1        yes              1      yes
        1          2              2        yes              2      yes
        5          6              6        yes              6      yes
      128        129            129        yes            129      yes
      254        255            255        yes            255      yes
      255        256         100000         NO            256      yes

(current reaching 100000 at counter = 255 is a safety cap I added so non-termination could be observed rather than hung on.)

Your version performs exactly counter + 1 generate calls for every counter including the maximum, and terminates. The one prerequisite is that i remains unsigned int, since the fix relies on defined wraparound - which it is today, but it becomes load-bearing in a way it wasn't before, so it may be worth a word in the comment beyond /* overflow */.

One thing worth weighing before choosing between this and bounding the counter. At the ~3 us per iteration measured above, counter = UINT_MAX under this fix terminates after 2^32 iterations - about 3.7 hours. So it converts "never returns" into "returns in roughly four hours", which by your own earlier argument isn't a difference a caller can perceive.

Which suggests the two aren't really competing fixes:

• Bounding the counter fixes termination implicitly - with any bound below UINT_MAX, i <= counter can no longer be a tautology, so even the current for loop terminates - and bounding it to something small additionally removes the running time and the quadratic re-derivation.
• The do/while fixes termination but leaves the running time and the quadratic behaviour exactly as they are.

So if the counter does get bounded, this loop change becomes defensive rather than necessary; if it doesn't, the loop change is necessary but not sufficient. Given that RFC 6979 has no counter at all, bounding it still looks like the change that makes all three problems disappear at once - with the do/while as belt-and-braces if you'd rather the loop be locally correct regardless of what the API permits.

[real-or-random]

Approach NACK

It's true that we should fix the infinite loop here. But given that this can be easily done in a way that yields the correct result for UINT_MAX, let's do this instead.

And then we can still see whether we want to bound counter. My current thinking is "no" because it raises API compatibility questions and the question what the bound should be. None of these are essential, and I feel we should not waste our limited brain capacity on them. If the caller wants 2^32 HMAC calls, let's give them 2^32 HMAC calls. If we ever introduce a modern ECDSA API, this can be dealt with.

[mnfadel]

Makes sense - "if the caller wants 2^32 HMAC calls, give them 2^32 HMAC calls" is a cleaner contract than picking a bound nobody can justify, and it avoids the compatibility question entirely.

One small implementation note and then I'll leave it with you: breaking on equality before the increment

for (i = 0; ; i++) {
    secp256k1_rfc6979_hmac_sha256_generate(hash_ctx, &rng, nonce32, 32);
    if (i == counter) break;
}

never overflows at all - the exit happens before the increment that would wrap - so correctness doesn't depend on i being unsigned, and neither the /* overflow */ case nor the implicit requirement on the type is needed. Both versions are correct and produce the same counter + 1 calls; purely your call which reads better.

Happy to re-run the reproduction and the test suite against whichever version lands.

[real-or-random]

the exit happens before the increment that would wrap - so correctness doesn't depend on i being unsigned, and neither the /* overflow */ case nor the implicit requirement on the type is needed.

Yeah, I agree that this code is better. Want to open a PR?

[l0rinc]

Thanks @real-or-random, took your simplified version (as a for loop though, to minimize the diff.

@mnfadel, thanks for the review, but I have to ask: is there a human behind the account? It's fine to use AI assistance, but we prefer talking to humans.

[mnfadel]

Hey! Yeah for sure, Mohamad here. I'm an engineer with a background in ERP suites - for now. I've been getting into secp256k1 and Bitcoin for over 3 years now. Was focusing on the Bitcoin puzzle mainly. Yes, i do use an AI assistant to help me organize my words, not now though:) I do enjoy good feedback especially after a bunch of blocked walls..

[real-or-random]

utACK b1bc6f3

[real-or-random]

Hey! Yeah for sure, Mohamad here.

Hey Mohamad, thanks for your contribution here!

I think it makes sense to make your mode of using AI transparent, in particular when you're new to a repo, e.g., write something like this: "I asked Claude x.y whether ... and it tells me that .... I have (not) verified the claim. See here for full prompt and response: ... ". Or: "I came up with this idea and I asked GPT 5.4 to confirm it , ...".

When I see text from a new contributor generated by an LLM, it's very hard to me to assign a level of trustworthiness to it (which is one of my main tasks as a maintainer). LLMs and human both make mistakes, but they have distinct strengths and weakness. Moreover, LLMs tend to sound confident even if they're not. For an LLM text, all I can do can read it and agree or disagree with the arguments brought up, but that's a time-consuming process, even more so because LLMs tend to be verbose. Worse, when people tell an LLM to write a comment, the LLM will write a comment, no matter if the comment adds anything at all. In the worst case, human brain bandwidth has been wasted on reading an LLM summary of the issue but no insights were generated.

That's why I tend to skip text when I don't know if a human was involved at all or not. It's hard to take it serious, and it's also less useful. If I want an LLM review, I can get myself one immediately after all. So giving your interaction a human touch is more useful. And since we're all (still) humands, it's also just a nicer experience to talk to a human.

[mnfadel]

Hello again Tim,

Yeah already with you on that one. However, I'm actually quite into LLMs for a while now, and i do know how to safeguard responses. I am at a point where i triple-check responses before even posting. I distinguish fabricated and hallucinated responses when they occur. I've built my profile on those standards. As a ground truth, my strength is in my analysis skills specifically when it comes to Artificial Intelligence Engineering. I'm actually working on an engine for that specific purpose, applying guardrails for ever-correct answers.

I can discuss any possible ways to contribute further if needed.

M.

[real-or-random]

I am at a point where i triple-check responses before even posting. I distinguish fabricated and hallucinated responses when they occur. I've built my profile on those standards.

All good! :) My main point is now I that I'm aware of this, I can judge the value of your comments much better, so I'm looking forward to more contributions if you're interested.

[mnfadel]

Of course! Would love to contribute in any way possible.. let me know..

[theStack]

Chiming in to agree what was already stated in detail, I also can't imagine a scenario where a user would ever want to set such a high counter value (if even hitting 1 is negligible), even less so considering the multiple-hours waiting time, so this fix is certainly a very theoretical one. Still worthwhile to get rid of the endless loop without disallowing UINT_MAX, so

ACK b1bc6f3

And then we can still see whether we want to bound counter. My current thinking is "no" because it raises API compatibility questions and the question what the bound should be. None of these are essential, and I feel we should not waste our limited brain capacity on them. If the caller wants 2^32 HMAC calls, let's give them 2^32 HMAC calls. If we ever introduce a modern ECDSA API, this can be dealt with.

I very much agree.

@mnfadel: Welcome to the project! 🎉