wallet: keep enumerating after duplicate signers #35233

pull l0rinc wants to merge 2 commits into bitcoin:master from l0rinc:l0rinc/external-signer-skip-canceled-duplicates changing 2 files +12 −1

l0rinc commented at 9:33 AM on May 7, 2026: contributor

Problem

ExternalSigner::Enumerate() says it skips duplicate signers, but the current control flow stops enumeration when it finds the first duplicate fingerprint.

This can hide later unique signer entries returned by the same enumerate call.

Fix

Use continue instead of break for duplicate signer entries, so only the duplicate entry is skipped and later entries are still processed. This does not change error handling: if an enumerate entry reports an error before it reaches the duplicate check, the RPC still reports that error.

Test

The functional test now covers an enumerate result containing one signer, a duplicate of that signer, and a later unique signer.

test: cover duplicate signer enumeration

External signer enumeration currently stops when it sees a duplicate fingerprint, so later unique signer entries are not reported.
Add coverage for that behavior before changing duplicate handling from stopping enumeration to skipping one entry.

773f305e51

wallet: continue after duplicate signers

The code says duplicate signer entries are skipped, but it currently breaks out of enumeration at the first duplicate and can hide later unique signers.
Use `continue`, so the implementation matches the comment: only the duplicate entry is skipped and later entries still reach the caller.
This does not change error handling. A duplicate entry that reports an error before the duplicate check is still reported as an error.

d543f8ba1d

DrahtBot added the label Wallet on May 7, 2026
DrahtBot commented at 9:33 AM on May 7, 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/35233.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ViniciusCestarii, kevkevinpal

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
ViniciusCestarii commented at 6:57 PM on May 7, 2026: contributor
tACK d543f8ba1d05b5c159adb76ff1cd6b3428db0e32

Ran the test and did some manual mutation testing to confirm it catches regressions:
- Dropping {"fingerprint": "00000002", "name": "trezor_one"} from the expected list -> test fails (assertion catches the missing signer).
- Swapping continue for break in the wallet enumeration -> test fails (only the first signer gets returned).
Sjors commented at 7:29 AM on May 8, 2026: member

This probably makes sense. @achow101 do you remember under what circumstances HWI returns duplicate signers?
achow101 commented at 8:04 AM on May 8, 2026: member

do you remember under what circumstances HWI returns duplicate signers?

It should never have duplicates.
Sjors commented at 9:23 AM on May 8, 2026: member

@achow101 I vaguely remember something like finding the same device via two different transports.
kevkevinpal commented at 3:14 PM on May 8, 2026: contributor

ACK d543f8b

I'm confused why, in the first commit, you added a broken test to fix it in the next commit instead of just a single commit. But I guess it does make sense to have some coverage for enumerating duplicate signers if we ever end up rolling back this change.
l0rinc commented at 3:17 PM on May 8, 2026: contributor

you added a broken test to fix it

To document the current behavior, so that the next commit shows exactly what changed compared to the current behavior. A single commit would only show the behavior after the fix.
l0rinc commented at 3:11 PM on May 9, 2026: contributor

I'll close this one to allow new contributors to continue: https://github.com/bitcoin/bitcoin/pull/35251
l0rinc closed this on May 9, 2026