parse external signer master fp as bytes in ExternalSigner::SignTransaction #25019

pull scgbckbone wants to merge 1 commits into bitcoin:master from scgbckbone:lower_master_fp_of_ext_signer_in_SignTransaction changing 1 files +3 −2
  1. scgbckbone commented at 6:02 PM on April 28, 2022: none

    Some external signers scripts may provide master fingerprint in uppercase format. In that case core will fail with Signer fingerprint 00000000 does not match any of the inputs as it only works with lowercase format. Even if the fingerprints match, yet one is lowercase the other uppercase.

    ExternalSigner::SignTransaction is the only place where it is needed IMO, as changing it in other places may break the communication with the external signer (i.e. enumerating with lowercase may not find the device).

  2. scgbckbone cross-referenced this on Apr 28, 2022 from issue master fingerprint should be lowercase by scgbckbone
  3. laanwj added the label Wallet on Apr 28, 2022
  4. prusnak commented at 9:12 PM on April 28, 2022: contributor

    I find it better if HWI corrected this and always normalized to lower-case fingerprints. The other, maybe even better option is that HWI would assert that a fingerprint returned from an external signer script is always lowercase.

    Edit: cross-posted to https://github.com/bitcoin-core/HWI/issues/599#issuecomment-1112659492

  5. scgbckbone commented at 1:26 AM on April 29, 2022: none

    Currently HWI provides all master fingerprints in lowercase (all devices use .hex() on byte sequence). Question is: is HWI the only external signer? and will be?

    IMO when comparing byte sequences, we should normalize as they are per se not upper or lower case...

  6. Sjors commented at 9:23 AM on April 29, 2022: member

    I think normalising to lower case is fine, but doing it only for signtx seems a bit brittle.

    HWI is not the only potential signer. There's a document that describes how external signer scripts / programs should behave. This currently does not specify if the fingerprint MUST / SHOULD be lower / upper case, or that it may be both. We should clarify that.

    Note that BIP32 doesn't specify how a fingerprint should be represented when used as a hex string. The examples it uses are uppercase and use the 0X prefix. We could require all software (including Bitcoin Core, e.g. using ParseHex() in strencodings.h) to cleanly handle parsing of hex strings in any sane format, but I'm tempted to say it's easier to just require lower case without 0x prefix.

  7. scgbckbone cross-referenced this on Apr 29, 2022 from issue normalize PSBT entry instead of external signer master fingerprint by scgbckbone
  8. scgbckbone cross-referenced this on Apr 29, 2022 from issue add explicit condition fo master fingerprint to be lowercase hex by scgbckbone
  9. scgbckbone commented at 3:41 PM on April 29, 2022: none

    I think if external signer(ES) provides you with the uppercase master fp, changing it lowercase can break the communication with ES, as ES itself can implement no normalization and only expect uppercase.

    1. after enumeratesigners you get uppercase from ES
    2. core changes this globaly to lowercase to fit its needs (imo bad)
    3. trying to run other commands on ES with --fingerprint=<normalized_lowercase_fingerprint> can get you no results as ES can have same issue as core here (cmp lowercase and uppercase)

    there are only 2 places where master fp is being compared:

    1. ExternalSigner::SignTransaction --> here it needs to be normalized (on the side of ES or core), core has somehow arbitrarily? chosen to encode lowercase
    2. ExternalSigner::Enumerate line 49 in duplicate check --> here it is not needed to be normalized as we check against what we get from the ES

    Potential solutions I see:

    1. as proposed here --> kind of already got 2 mehs from @prusnak @Sjors
    2. core can instead normalize fingerprint retrieved from PSBT and check both upper and lowercase representation of hex string -> this PR
    3. we do not care and should explicitly state that fingerprint has to be in lower hex --> this PR... this should be imo merged either ways but if 1. or 2. gets merged too language there should be changed from MUST to SHOULD
  10. achow101 commented at 8:39 PM on April 29, 2022: member

    Case-ness shouldn't matter. What we should be doing is parsing the fingerprint string as bytes, not converting the bytes fingerprint to hex.

  11. scgbckbone commented at 11:04 PM on April 29, 2022: none

    @achow101 is right imo - I updated this PR to instead of playing with upper and lower case parsed master fp from external signer as bytes and comapred with bytes from PSBT... my c++ is non-existent so not sure If it is optimal

  12. scgbckbone renamed this:
    normalize (lowercase) master fp in ExternalSigner::SignTransaction
    parse external signer master fp as bytes in ExternalSigner::SignTransaction
    on Apr 29, 2022
  13. scgbckbone cross-referenced this on Apr 29, 2022 from issue normalize master fp to lowercase by scgbckbone
  14. Sjors commented at 5:50 PM on April 30, 2022: member

    That looks better at first glance. Can you squash these three commits?

  15. scgbckbone force-pushed on May 1, 2022
  16. scgbckbone force-pushed on May 1, 2022
  17. scgbckbone commented at 2:50 AM on May 1, 2022: none

    squashed and rebased on top of current master

  18. fanquake requested review from Sjors on May 5, 2022
  19. fanquake requested review from achow101 on May 5, 2022
  20. theStack commented at 9:22 AM on May 5, 2022: contributor

    Concept ACK

    I think converting the bytes into an integer is not needed though, and you could simply compare them directly? E.g. with memcmp or something like

    if (ParseHex(m_fingerprint) == std::vector<uint8_t>(std::begin(entry.second.fingerprint),
                                                    std::end(entry.second.fingerprint))) return true;

    // EDIT

    if (ParseHex(m_fingerprint) == MakeUCharSpan(entry.second.fingerprint)) return true;

    is even shorter and should also work (at least it compiles for me).

  21. scgbckbone force-pushed on May 5, 2022
  22. scgbckbone commented at 11:17 PM on May 5, 2022: none

    updated based on @theStack comments (and squashed and rebased on top of current master)

  23. scgbckbone force-pushed on May 6, 2022
  24. theStack commented at 1:14 PM on May 6, 2022: contributor

    updated based on @theStack comments (and squashed and rebased on top of current master)

    Thanks. Please make sure that you use spaces instead of tabs in your change, the linter is complaining right now:

    This diff appears to have added new lines with tab characters instead of spaces.
The following changes were suspected:

diff --git a/src/external_signer.cpp b/src/external_signer.cpp
@@ -81 +81 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
+	    if (ParseHex(m_fingerprint) == MakeUCharSpan(entry.second.fingerprint)) return true;
  25. scgbckbone force-pushed on May 6, 2022
  26. scgbckbone commented at 4:45 PM on May 6, 2022: none

    tab character removed - linter happy

  27. in src/external_signer.cpp:81 in 99671e5663 outdated 
      77 | @@ -78,7 +78,7 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
  78 |      // Check if signer fingerprint matches any input master key fingerprint
  79 |      auto matches_signer_fingerprint = [&](const PSBTInput& input) {
  80 |          for (const auto& entry : input.hd_keypaths) {
  81 | -            if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) return true;
  82 | +            if (ParseHex(m_fingerprint) == MakeUCharSpan(entry.second.fingerprint)) return true;
    luke-jr commented at 1:21 AM on May 7, 2022:

    IMO, ParseHex(m_fingerprint) should be done once, outside this lambda.

    scgbckbone commented at 9:12 AM on May 7, 2022:

    very good point - thanks (fixed - only parse m_fingerprint once as it does not change)

  28. luke-jr changes_requested
  29. parsing external signer master fingerprint string as bytes instead of caring for lower/upper case in ExternalSigner::SignTransaction 2a22f034ca
  30. scgbckbone force-pushed on May 7, 2022
  31. luke-jr approved
  32. luke-jr commented at 6:17 AM on May 9, 2022: member

    utACK

  33. theStack approved
  34. theStack commented at 11:24 AM on May 9, 2022: contributor

    Code-review ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

  35. Sjors approved
  36. Sjors commented at 4:05 PM on May 10, 2022: member

    utACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

  37. Sjors cross-referenced this on May 10, 2022 from issue Add external signer taproot support by Sjors
  38. MarcoFalke assigned achow101 on May 10, 2022
  39. achow101 commented at 7:49 PM on May 16, 2022: member

    ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

  40. achow101 merged this on May 16, 2022
  41. achow101 closed this on May 16, 2022
  42. luke-jr referenced this in commit e154c6894d on May 21, 2022
  43. sidhujag referenced this in commit 6c70aa9df0 on May 28, 2022
  44. bitcoin locked this on May 16, 2023
Contributors
scgbckboneprusnakSjorsachow101theStackluke-jr


achow101

Labels
Wallet
Linked (view graph)
#23578 Add external signer taproot support #25032 normalize PSBT entry instead of external signer master fingerprint #25033 add explicit condition fo master fingerprint to be lowercase hex
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC