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

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. laanwj added the label Wallet on Apr 28, 2022
3. 
   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

4. 
   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...

5. 
   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.

6. 
   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
7. 
   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.

8. 
   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

9. scgbckbone renamed this:
   normalize (lowercase) master fp in ExternalSigner::SignTransaction
   parse external signer master fp as bytes in ExternalSigner::SignTransaction
   on Apr 29, 2022
10. 
    Sjors commented at 5:50 PM on April 30, 2022: member

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

11. scgbckbone force-pushed on May 1, 2022
12. scgbckbone force-pushed on May 1, 2022
13. 
    scgbckbone commented at 2:50 AM on May 1, 2022: none

    squashed and rebased on top of current master

14. fanquake requested review from Sjors on May 5, 2022
15. fanquake requested review from achow101 on May 5, 2022
16. 
    theStack commented at 9:22 AM on May 5, 2022: member

    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).

17. scgbckbone force-pushed on May 5, 2022
18. 
    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)

19. scgbckbone force-pushed on May 6, 2022
20. 
    theStack commented at 1:14 PM on May 6, 2022: member

    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;
    

21. scgbckbone force-pushed on May 6, 2022
22. 
    scgbckbone commented at 4:45 PM on May 6, 2022: none

    tab character removed - linter happy

23. 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)

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

    utACK

29. theStack approved
30. 
    theStack commented at 11:24 AM on May 9, 2022: member

    Code-review ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

31. Sjors approved
32. 
    Sjors commented at 4:05 PM on May 10, 2022: member

    utACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

33. MarcoFalke assigned achow101 on May 10, 2022
34. 
    achow101 commented at 7:49 PM on May 16, 2022: member

    ACK 2a22f034ca3298c9f86d1edd4283a0bea18dfbbe

35. achow101 merged this on May 16, 2022
36. achow101 closed this on May 16, 2022

    ---

37. luke-jr referenced this in commit e154c6894d on May 21, 2022
38. sidhujag referenced this in commit 6c70aa9df0 on May 28, 2022
39. DrahtBot locked this on May 16, 2023

Contributors

scgbckboneprusnakSjorsachow101theStackluke-jr

Review Requested
achow101