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.

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, 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

test: Add duplicate external signers test

Extend the `rpc_signer.py` test with duplicate signers case.

eaa62f9978

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. Test updated.

1d9e4252d2

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

  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

  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.

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