wallet: Fix for duplicate external signers case #35251

optout21 commented at 9:17 AM on May 9, 2026: contributor

A straightforward fix for a minor bug in a low-probability case; when there are multiple signers with the same fingerprint, the signers following the duplicate are also thrown away (due to a break instead of continue).

Background. Bitcoin core can work with external signers. They can be configured using the -signer=<cmd> argument. The enumeratesigners RPC can be used to retrieve the available signers.

Precondition A minor bug was found in a special case:

multiple external signers are detected, and
at least two of them are duplicate, i.e., have identical fingerprint, and
the duplicates are followed by at least one additional signer (that is, the last one is not a duplicate),

Current behavior: Only one of the duplicates is kept, however all subsequent signers are also ignored. This last part is probably unintended, thus it's a minor bug. To put it in another way, the set of fingerprints returned depends on the actual order in which the devices are returned by the external tool, which may be arbitrary.

Expected behavior: De-duplicate the found signers by fingerprint:

keep one signer for each fingerprint
keep all non-duplicates.

Fix: The early break of the loop of signers upon duplication has been changed to continue.

Test added/extended: RPCSignerTest in test/functional/rpc_signer.py has been extended with a case of multiple duplicates.

DrahtBot added the label RPC/REST/ZMQ on May 9, 2026

DrahtBot commented at 9:17 AM on May 9, 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/35251.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, polespinasa, naiyoma, achow101
Stale ACK	kevkevinpal

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

optout21 force-pushed on May 9, 2026

DrahtBot added the label CI failed on May 9, 2026

sedited commented at 9:43 AM on May 9, 2026: contributor

Can you rewrite the pull request description in your own words? It reads very LLM formulaic. Not following a canned format and being concise is an indication for reviewers that you understand the change, making it more likely to be reviewed.

DrahtBot removed the label CI failed on May 9, 2026

optout21 commented at 2:56 PM on May 9, 2026: contributor

Well, @sedited, I have to admit that no LLM was involved in the text of this PR in any shape or form, this is my own style I use for many decades by now. These are my own words, including the manually typed header triple-hashes. I can remove those if it makes it less LLM-looking :D Following your feedback, I've added a 2-sentence summary at the top, hopefully helpful for reviewers.

Frankly, I can't decide to take this as a critique or a compliment... (I did use an LLM to help me finding the test code location, though.) I even forgot to run the description through an LLM/spellchecker, which I did now, and found some embarassing mistakes, fixing them now ("was fund", "one of the duplicates are kept").

optout21 marked this as ready for review on May 9, 2026

l0rinc commented at 3:09 PM on May 9, 2026: contributor

I have opened a similar one, I'll close it to allow you to take over: #35233. Please see the comments there.

optout21 commented at 3:20 PM on May 9, 2026: contributor

I have opened a similar one, I'll close it

Duplicate work happened, following some prior communication. Clarified now, thanks.

in test/functional/rpc_signer.py:83 in 24e93f7f9e

  78 | +        # With the current likely incorrect behavior, the first duplicate and ALL subsequent signers are ignored.
  79 | +        self.set_mock_result(self.nodes[2],
  80 | +            '0 ['
  81 | +            '{"fingerprint": "00000001", "model": "signer_1"}, '
  82 | +            '{"fingerprint": "00000002", "model": "signer_2"}, ' # duplicate 1/2
  83 | +            '{"fingerprint": "00000002", "model": "signer_3"}, ' # duplicate 2/2

l0rinc commented at 3:32 PM on May 9, 2026:

24e93f7 test: Add duplicate external signers test:

Given how unlikely these are I would prefer simple, minimal reproducer, e.g. https://github.com/bitcoin/bitcoin/pull/35233/changes/773f305e514743ec332b1751b36834df5b23dedd#diff-76d56c3936515f4a5618805c6c902fe0cd5b4d3ed356af78917dd7ba80372aa9R75-R83

optout21 commented at 3:54 PM on May 9, 2026:

Done

l0rinc approved

l0rinc commented at 3:33 PM on May 9, 2026: contributor

Concept ACK, please simplify the tests to make the proportional to their importace

optout21 force-pushed on May 9, 2026

optout21 commented at 3:50 PM on May 9, 2026: contributor

Simplified the test (3 identical signers removed, not really necessary).

optout21 force-pushed on May 9, 2026

DrahtBot added the label CI failed on May 9, 2026

l0rinc commented at 4:15 PM on May 9, 2026: contributor

tested ACK 1d9e4252d25d38e93e776440b8eaf231bc49467b

Tests pass locally for for both commits. First one documenting current behavior, second one the fixed behavior. Second test correctly fails without the fix with:

AssertionError: not({'signers': [{'fingerprint': '00000001', 'name': 'trezor_t'}]} == {'signers': [{'fingerprint': '00000001', 'name': 'trezor_t'}, {'fingerprint': '00000002', 'name': 'trezor_one'}]})

DrahtBot removed the label CI failed on May 9, 2026

kevkevinpal commented at 6:14 PM on May 9, 2026: contributor

ACK 1d9e425

I previously ACK'd the closed PR, so ACKing here as well #35233 (comment)

sedited commented at 8:16 PM on May 9, 2026: contributor

Frankly, I can't decide to take this as a critique or a compliment... (I did use an LLM to help me finding the test code location, though.)

My apologies, I was too primed on the format after a bunch of slop PRs were opened. Clearly this is starting to be flawed, if contributors prefer this more formulaic description.

optout21 renamed this:
~~rpc: Fix for duplicate external signers case~~
wallet: Fix for duplicate external signers case
on May 9, 2026

in src/external_signer.cpp:56 in 1d9e4252d2 outdated

  52 | @@ -53,7 +53,7 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
  53 |          for (const ExternalSigner& signer : signers) {
  54 |              if (signer.m_fingerprint.compare(fingerprintStr) == 0) duplicate = true;
  55 |          }
  56 | -        if (duplicate) break;
  57 | +        if (duplicate) continue;

polespinasa commented at 9:09 AM on May 11, 2026:

in 1d9e4252d25d38e93e776440b8eaf231bc49467b rpc: Fix for duplicate external signers case

nit-feel-free-to-ignore: I would like to have this line commented to explain it. I think the commit message text could be used.

l0rinc commented at 9:16 AM on May 11, 2026:

https://github.com/bitcoin/bitcoin/pull/35251/changes#diff-75def83348cbe1eb61d67f3b629638f5fa1763945051c061c1c523e54627696bR51 already states we're duplicating

polespinasa commented at 9:24 AM on May 11, 2026:

true, didn't see that line

optout21 commented at 9:48 AM on May 11, 2026:

I feel the code is self-documenting well (literally "if (duplicate) ..."), and the change is also rather obvious. I don't feel a comment would add much value here.

in test/functional/rpc_signer.py:83 in eaa62f9978 outdated

  78 | +            '{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, '
  79 | +            '{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}, '
  80 | +            '{"fingerprint": "00000002", "type": "trezor", "model": "trezor_one"}'
  81 | +            ']')
  82 | +        assert_equal(self.nodes[1].enumeratesigners(), {"signers": [
  83 | +            {"fingerprint": "00000001", "name": "trezor_t"},

polespinasa commented at 9:13 AM on May 11, 2026:

in eaa62f9978abcf2b58723666d678edf6b71686e2 test: Add duplicate external signers test

The test is wrong we should not add a commit with a wrong test. As you did in the second commit it should be checking that the non-duplicate is also included in the response:

assert_equal(self.nodes[1].enumeratesigners(), {"signers": [
            {"fingerprint": "00000001", "name": "trezor_t"},
            {"fingerprint": "00000002", "name": "trezor_one"},
        ]})

You could drop the first commit or drop the first commit and split the second one in two, first commit for the fix and second for the test.

l0rinc commented at 9:18 AM on May 11, 2026:

Not sure what you mean, the first commit documents the current behavior, the next commit changes behavior and documents the effect it has by adjusting the tests.

polespinasa commented at 9:30 AM on May 11, 2026:

I don't think introducing a wrong test is how current behavior should be documented. If it was a correct behavior change (by correct I mean two expected behaviors not a bug) maybe yes, but adding a test that says "x should happen" when x is a bug doesn't lgtm.

Behavior change can be tested by just running the new test with the pre-patched code and checking that it fails.

l0rinc commented at 9:50 AM on May 11, 2026:

Behavior change can be tested by just running the new test

How would that demonstrate that the test is actually exercising the proposed change? How would we assess with a single commit what exactly the effect of the fix is? By documenting the existing behavior the CI demonstrates what the current behavior is exactly and it's trivial to test the fix against the old assumptions, pinpointing exactly what the effects of the fix are on our assumptions - instead of a lot of assertions added with the fix (most of which were already true before the fix), you only see the exact change in assumptions.

This is a standard technique called characterization testing (see also Feathers' original write-up: the first commit pins the existing (buggy) behavior so the second commit's diff isolates exactly what the fix changes in our assumptions, rather than mixing fix + new assertions in one commit. As Feathers puts it: "When we write characterization tests we build up our knowledge of what the code actually does. This is particularly useful when we want to refactor or rewrite. We can run our tests and find out immediately whether we've changed behavior."

optout21 commented at 10:04 AM on May 11, 2026:

True, the effect can be seen by running the old code vs. the new test. However, this requires manually combining code into a version which is not in the PR. With the other approach, the check for both commits passing tests verifies this implicitly.

In general, I do see the value of a diff on the test illustrating the effect of a behavior change (regardless of whether it's a bug or changed requirement -- the distinction can be blurry).

Having said that, I will remove the pre-patch test to simplify things here. (Note: @kevkevinpal also had a similar comment).

l0rinc commented at 10:14 AM on May 11, 2026:

I will remove the pre-patch test to simplify things here

If users don't like that, they can just view the change in the squashed view, I prefer the current structure - it's trivial to squash but not to take apart.

l0rinc commented at 10:33 AM on May 11, 2026:

This came up in multiple reviews, so I opened a PR to document the rationale: https://github.com/bitcoin/bitcoin/pull/35260

polespinasa commented at 10:44 AM on May 11, 2026:

Okay, got it. Still I don't like it, but with what @l0rinc said I could understand it and agree to it, concept~0. I will not oppose to it if other contributors like this type of commit structure.

But imho this makes sense for long PRs with some complexity to review. This is a lot of noise for a single word change.

achow101 commented at 11:21 PM on May 20, 2026:

I agree with @polespinasa, a test should be introduced for the correct behavior only, and if that doesn't pass by itself, it should be introduced after the fix. Such tests can be run against the old code to verify that it fails. This is a common pattern used for many PRs in this codebase, and is my preferred pattern for adding tests for bug fixes.

Even with an incorrect test being introduced first, as a reviewer, I would still want to execute the new test on the old code to verify that it catches the bug. Splitting the test across commits like this makes it more annoying to do that.

optout21 commented at 4:08 AM on May 21, 2026:

This is a coding style question, subjective, and apparently controversial. I respond in general and in particular for this PR.

"I would still want to execute the new test on the old code to verify that it catches the bug". The combination of new test with old code is a "manual" step that may be complicated and error prone (in more complex cases). It is harder to reproduce, as it involves a code combination that is not present in git. However, with the 2-commit approach, if the change in the test is exclusive (meaning that both test versions cannot pass with the same code), e.g. because an expected value is changed or an expected condition is reverted, that in itself establishes the fact that the new test fails with the old code. I think this approach is much clearer and more robust. I'd suggest you check the carved out doc issue #35260, and maybe chime in there.

For this particular PR the change is trivial (it's easy to verify by changing the code). I observe the opinion of those who were willing to review and are experienced devs (though handling contradictory opinions can be a challenge). I will simplify this PR as you prefer.

l0rinc commented at 6:20 AM on May 21, 2026:

Even with an incorrect test being introduced first, as a reviewer, I would still want to execute the new test on the old code to verify that it catches the bug. Splitting the test across commits like this makes it more annoying to do that.

Please see #35260 for details on why characterization testing is actually superior to adding the fix and test for uncovered code. The commits already document the test changes needed for the test to pass - which the CI (or your local env) already check. The diff is exactly the reason why it won't pass if you revert. So it favors reviewers: the author already does the separation to ease reverting the feature (which isn't always as trivial as it is here).

achow101 commented at 12:22 AM on May 26, 2026:

The combination of new test with old code is a "manual" step that may be complicated and error prone (in more complex cases). It is harder to reproduce, as it involves a code combination that is not present in git.

The approach I describe is still 2 commits, and often trivial to apply the test the master during review. The change goes as the first commit, and the test after in the second commit. Checking the test on master is as simple as cherry-picking the second commit and requires little to no manual intervention.

Squashing everything into 1 commit was not my suggestion.

optout21 commented at 4:11 AM on May 26, 2026:

Ah, OK, I misunderstood then. Noted.

polespinasa changes_requested

polespinasa commented at 9:15 AM on May 11, 2026: member

will ack after the test is fixed

Sjors commented at 9:17 AM on May 11, 2026: member

I'd still like to know how the original issues (having a duplicate signer) can be reproduced: #35233 (comment)

optout21 commented at 9:42 AM on May 11, 2026: contributor

I imagine the obvious case with two duplicate physical devices, with the same seedphrase loaded, is possible. It's unlikely, but it can happen realistically e.g. during migration from one HW device to another one. (For the bug to manifest, there should be a third device as well, which makes it more unlikely to happen.)

The case you mentioned -- one device showing up duplicated is also plausible, e.g. due to some USB subsystem magic, but for me that's just a guess.

Is this question very relevant for this PR? There is code for handling duplicates that has a bug, with an easy fix, so it should be fixed. Or, if there is no need for handling duplicates, the whole logic could be removed.

BTW, the wallet seems to assume a single signer, e.g. GetExternalSigner() takes the first signer. There is a TODO that in case of multiple signers it should choose based on the fingerprint. So more work could be done there in the future.

https://github.com/bitcoin/bitcoin/blob/ccbd00ab87c09779e9edc589af6df5c967687a2f/src/wallet/external_signer_scriptpubkeyman.cpp#L54

fanquake commented at 1:18 PM on May 20, 2026: member

cc @cobratbq @naiyoma

rpc: Fix for duplicate external signers case

In case of multiple external signers, with some duplicates,
de-duplicate them: keep one signer per fingerprint, and
keep all non-duplicates as well. Add a new test check.

f05b1a3532

optout21 force-pushed on May 21, 2026

optout21 commented at 4:29 AM on May 21, 2026: contributor

Change applied: simplified PR, first commit with test-only dropped; there is now a single commit with code change and new test check (as suggested by reviewers). Rebased as well once touched.

l0rinc commented at 6:24 AM on May 21, 2026: contributor

ACK f05b1a3532bd8ddcfb6a8b7a6f9f5eb4aeee68ef

Squashed commits since the last review; the characterization testing angle was debatable here, as the change is very simple.

polespinasa approved

polespinasa commented at 6:27 AM on May 21, 2026: member

code review ACK f05b1a3532bd8ddcfb6a8b7a6f9f5eb4aeee68ef

Sjors commented at 6:42 AM on May 21, 2026: member

two duplicate physical devices, with the same seedphrase loaded, is possible.

But this PR doesn't help in that scenario, since the user can't choose between the duplicates. Arguably we should show the duplicate in that case, and then have any operation referencing the duplicate fingerprint fail, since we don't know which one to pick.

Perhaps a better title for this PR is: "ignore duplicate external signers"

It fixes the case where HWI or an equivalent application unintentionally returns a duplicate result, not the case where there actually are duplicate signers. Leaving that to a followup is fine.

l0rinc commented at 6:48 AM on May 21, 2026: contributor

Arguably we should show the duplicate in that case

But we're not just omitting duplicates currently - we don't even show any signers after them. Do you think we should remove all duplicate handling instead?

Sjors commented at 6:50 AM on May 21, 2026: member

I think the current approach is fine, but the commit message is over selling it. This helps in a scenario where you have two duplicate signers somehow (either a bug in HWI / equivalent, or an actual duplicate) and you don't want to use it, because there's a third device.

optout21 commented at 7:29 AM on May 21, 2026: contributor

Sorry @Sjors , I'm not sure I understand your point. Are you suggesting to remove the duplicate check completely, or keep the current change with different commit message, or keep the existing legacy code and drop this PR?

I can repeat my point mentioned previously: Currently there is code for handling duplicates, and to me the logic has a bug. The bug is relevant only in improbable cases, but has an easy fix. It seems logical to me to fix it. Alternatively, if there is no need for de-duplication, the whole logic should be removed (simplify the code, get rid of the bug; probably in another PR). Assuming that we don't want to show multiple same-fingerprint devices (in the RPC call result), I can see no logical justification for keeping the current behavior, as it affects other (unrelated) signers. Specifically, the set of fingerprints returned depends on the actual order the devices are returned by the external tool, which may be arbitrary.

l0rinc commented at 7:58 AM on May 21, 2026: contributor

I think the current approach is fine, but the commit message is over selling it.

Are you suggesting to remove the duplicate check completely, or keep the current change with different commit message

My understanding is that this was basically a concept ack from him ("current" is a bit confusing in this context), asking the PR description and commit message to be updated.

Sjors commented at 7:14 AM on May 22, 2026: member

keep the current change with different commit message

This. The change is fine for now, but shouldn't be presented as fixing duplicate signer handling.

optout21 commented at 12:54 PM on May 22, 2026: contributor

The change is fine for now, but shouldn't be presented as fixing duplicate signer handling.

OK, that's clear now, thanks. You are right, the commit message is misleading: this change does not attempt to fix the handling of duplicated signers as such, it just fixes a small issue with the existing handling. I will reformulate it to be more clear.

naiyoma commented at 9:42 PM on May 25, 2026: contributor

ACK f05b1a3532bd8ddcfb6a8b7a6f9f5eb4aeee68ef

test fails on master

raise AssertionError(f"not({thing1!s} == {thing2!s})")
AssertionError: not({'signers': [{'fingerprint': '00000001', 'name': 'trezor_t'}]} == {'signers': [{'fingerprint': '00000001', 'name': 'trezor_t'}, {'fingerprint': '00000002', 'name': 'trezor_one'}]})

as a follow-up, it would be nice to have a LogWarning or an rpc warning when duplicates are found.

achow101 commented at 12:23 AM on May 26, 2026: member

ACK f05b1a3532bd8ddcfb6a8b7a6f9f5eb4aeee68ef

achow101 merged this on May 26, 2026

achow101 closed this on May 26, 2026

optout21 deleted the branch on May 26, 2026