contrib: exclude inactive keys from the binary quorum by default #36293

l0rinc commented at 11:21 PM on September 17, 2026: contributor

Problem: The binary verifier counts signatures from expired and revoked keys toward --min-good-sigs. Enough of these signatures can satisfy the threshold without any active signing key, and they are returned as good signatures.

Fix: Warn about expired and revoked signatures and exclude them from threshold counting by default. For historical verification, expired signatures can count toward the threshold with --allow-expired or BINVERIFY_ALLOW_EXPIRED, including signatures created after key expiry. Revoked signatures remain excluded. Expired signatures are reported separately, including in the expired_sigs field in successful JSON output.

The decision uses GnuPG’s current key status from the local keyring, even if the signature was created while the key was active.

This was found and disclosed responsibly by the Red Team 🟥.

DrahtBot added the label Scripts and tools on Sep 17, 2026

DrahtBot commented at 11:21 PM on September 17, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36293.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	willcl-ark

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

willcl-ark commented at 7:06 AM on September 18, 2026: member

i get revoked, but are we sure we want to exclude expired keys too?

will this break verifying old releases or current releases in the future?

i’ve noticed GPG keys lapse (in general, not only in this project) reasonably frequently and this doesn’t mean they’re compromised or bad.

sedited commented at 7:59 AM on September 18, 2026: contributor

will this break verifying old releases or current releases in the future?

I think that is the correct thing to do for revoked keys. What to do with expired keys has been a debate since ~forever. How about categorizing sigs from expired keys separately and adding a flag that allows the user to treat them as good?

willcl-ark commented at 8:10 AM on September 18, 2026: member

How about categorizing sigs from expired keys separately and adding a flag that allows the user to treat them as good?

I could see that as reasonable. With that optionality a user of "current software, today" should ideally see majority non-expired keys verified by default. Somebody targeting older software (perhaps with automated build tools) could set the optional flag to include expired keys.

We should prioritise "verifying current software today" in the most robust way possible. That's the less interesting/important use-case vs "historical verifications" where keys may have expired...

l0rinc renamed this:
~~contrib: prevent inactive keys from satisfying the binary verification quorum~~
contrib: exclude inactive keys from the binary quorum by default
on Sep 18, 2026

l0rinc force-pushed on Sep 18, 2026

l0rinc commented at 9:51 PM on September 18, 2026: contributor

Added --allow-expired for historical verification, with expired signatures reported separately and revoked signatures always excluded - also simplified the tests and reorganized the commits, thanks for the quick reviews.

willcl-ark commented at 10:12 AM on September 23, 2026: member

The end result looks decent, but the development sequence seems to change the same lines as the behavior changes through its flow.

Not a blocker for me, especially given the scope of this code, but I wonder whether a more "squashed" sequence, such as this, would reduce the churn (and make the history easier to follow and bisect if ever needed)?

If you prefer your more "natural development" sequence, I can probably be persuaded it's fine too, though.

l0rinc commented at 6:01 PM on September 23, 2026: contributor

I wonder whether a more "squashed" sequence, such as this, would reduce the churn

Your version is also a good way to solve it, but it has bigger conceptual jumps. I deliberately structured it so that I cover the behavior first before modifying it, adding the safety net before the jump. We get to the solution in trivial steps. It's like solving an equation while only doing one simplification at a time, to guarantee we're not skipping any steps. I document what the current behavior is to be able to prove that the fix changes that behavior, and to show exactly what the extent of the change is (keeping the test setup the same and only adjusting the assertions, so the diff shows both the before and after states and proving that both are passing CI).

l0rinc force-pushed on Sep 24, 2026

l0rinc commented at 12:42 AM on September 24, 2026: contributor

Thanks @willcl-ark for prompting another look. I combined your version and kept my structure expressed above (and inverted the revoked/expired commit order, which reduced a lot of TODO churn): I like this version more, let me know what you think. The review also uncovered a case where a key can be both revoked and expired: revocation now takes precedence.

in contrib/verify-binaries/verify.py:548 in a984c3efc6 outdated

 544 | @@ -544,6 +545,7 @@ def cleanup():
 545 |              'good_untrusted_sigs': [str(s) for s in good_untrusted],
 546 |              'unknown_sigs': [str(s) for s in unknown],
 547 |              'bad_sigs': [str(s) for s in bad],
 548 | +            'expired_sigs': [str(s) for s in expired],

willcl-ark commented at 7:43 AM on September 24, 2026:

In a984c3efc659296670df86c4b99e06d913c77f4e

Do you think the test should have an assertion for these expired_sigs fields?

l0rinc commented at 1:03 AM on September 25, 2026:

Sure, the pub --json test now checks that expired_sigs is a list, and the quorum test checks the expired signature’s full representation.

in contrib/verify-binaries/verify.py:386 in a984c3efc6 outdated

 383 |          else:
 384 | -            log.warning(f"INACTIVE SIGNATURE: {sig}")
 385 | +            assert sig.status == 'expired', sig
 386 | +            expired.append(sig)
 387 | +            log.warning(f"EXPIRED SIGNATURE: {sig}")
 388 |      num_trusted = len(good_trusted) + len(good_untrusted)

willcl-ark commented at 7:48 AM on September 24, 2026:

In a984c3efc659296670df86c4b99e06d913c77f4e

Not introduced in this PR, but in reading the changes above I notice we just sum the lengths here for a count. I haven't tested in GPG, but I'm not wondering whether I could provide 10 signatures from one key, and have this verify successfully; should we be de-duplicating by key or fingerprint perhaps?

Anyway, probably for a followup...

l0rinc commented at 1:05 AM on September 25, 2026:

I also got suspicious of this one and agree, we should address this in a follow-up. I confirmed with a disposable GPG key that repeating one signature can satisfy a threshold of two. For the default path, len({sig.key for sig in good_trusted + good_untrusted}) would catch that case, though counting distinct primary keys probably needs more care.

willcl-ark commented at 7:52 AM on September 24, 2026: member

As I said, I can be persuaded that a totally squashed commit series is not necessary here. I feel OK with this current one, thanks for updating.

One other side-thought I had thinking about this PR is that we are relying on local gpg (revocation) data/statuses being up-to-date for this to work. (Well, for revoked keys at least, which are perhaps the most important).

I wonder if we should prompt/remind users that they should ideally update keys periodically. I don't think we want to initiate that ourselves, as fetching keys is not a private process unless done carefully, so we might otherwise "dox our users" as bitcoiners...

test: characterize inactive signature quorum

Extract the parser for tests and add an empty expired-signature result to keep the characterization's return shape stable through the fixes.

Record that expired and revoked signatures currently count toward the quorum and can appear among good signatures.

431cac7273

contrib: exclude revoked keys from binary quorum

Revoked signatures currently count toward the quorum and can appear among good signatures.
Exclude them and give revocation precedence when GnuPG reports both statuses for a key.

Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>

20e15a5fdc

contrib: exclude and report expired signatures

Exclude expired keys from the quorum regardless of when they signed.
Return their signatures separately on quorum failure and in successful JSON output from both commands.

Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>

02939e424d

contrib: allow expired keys for old releases

Old releases may have signatures from keys that expired later.
Allow these to count with `--allow-expired` or `BINVERIFY_ALLOW_EXPIRED`, while continuing to exclude revoked keys and report expired signatures separately.

eff64984f5

doc: explain inactive binary signature handling 034271d4e1

l0rinc force-pushed on Sep 25, 2026

l0rinc commented at 1:11 AM on September 25, 2026: contributor

I wonder if we should prompt/remind users that they should ideally update keys periodically.

The latest push adds a short README note that stale local keyrings may miss revocations and keyserver updates can reveal requested keys - in a non-patronizing way. It also checks expired_sigs in the JSON test, covers both orders of the expired and revoked GPG statuses.

willcl-ark approved

willcl-ark commented at 9:31 AM on September 25, 2026: member

ACK 034271d4e1739347a31c64dbc4bfef8aa133ad1d

I verified recent releases with and without --allow-expired:

Release	Counted by default	Expired	Counted with `--allow-expired`	Result
22.1	9	5	14	Pass
23.2	7	5	12	Pass
24.2	8	4	12	Pass
25.2	5	2	7	Pass
26.2	6	4	10	Pass
27.2	8	3	11	Pass
28.4	8	2	10	Pass
29.4	9	3	12	Pass
30.3	8	2	10	Pass
31.1	8	2	10	Pass

All releases passed the default (three-signature) quorum with my local keyring. None raised an exception.

All seems well.

I think distinct keys could be an interesting belt-and-braces followup still, if it's reasonable/tractable.