external_signer: improve fingerprint matching logic (stop on first match) #22789

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202108-external_signer-improve_fingerprint_matching changing 1 files +6 −6

theStack commented at 10:42 AM on August 24, 2021: member

The fingerprint matching logic in ExternalSigner::SignTransaction currently always iterates all inputs of a PSBT, even after a match has already been found. I guess the reason for that is not that it was not thought of, but rather the fact that breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables). This PR fixes this by using std::any_of from C++'s standard library, see http://www.cplusplus.com/reference/algorithm/any_of/
external_signer: improve fingerprint matching logic (stop on first match) d047ed729f
fanquake requested review from Sjors on Aug 24, 2021

meshcollider commented at 12:17 PM on August 24, 2021: contributor

Concept ACK

breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables)

You could do this though:

-for (unsigned int i = 0; i < psbtx.inputs.size(); ++i) {
+for (unsigned int i = 0; !match && i < psbtx.inputs.size(); ++i) {
    const PSBTInput& input = psbtx.inputs[i];
    for (const auto& entry : input.hd_keypaths) {
-        if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true;
+        if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) { 
+            match = true;
+            break;
+        }
    }
}

lsilva01 approved
lsilva01 commented at 11:09 PM on August 24, 2021: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/22789/commits/d047ed729f1d4732d23324fc76849f3c657cdbe4
Sjors approved
Sjors commented at 1:38 PM on August 25, 2021: member

utACK d047ed7

I don't think there was a deep reason I didn't do this.
Zero-1729 approved
Zero-1729 commented at 12:09 PM on August 27, 2021: contributor

crACK d047ed729f1d4732d23324fc76849f3c657cdbe4
mjdietzx commented at 3:13 PM on August 30, 2021: contributor

Code review ACK d047ed729f1d4732d23324fc76849f3c657cdbe4

in src/external_signer.cpp:84 in d047ed729f

  84 | -            if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) match = true;
  85 | +            if (m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint))) return true;
  86 |          }
  87 | -    }
  88 | +        return false;
  89 | +    };

mjdietzx commented at 3:22 PM on August 30, 2021:

I'm curious, could you get rid of the other for loop as well with the same approach? Would that be cleaner? Something like:

return std::any_of(input.hd_keypaths.begin(), input.hd_keypaths.end(), [](auto& entry){ return m_fingerprint == strprintf("%08x", ReadBE32(entry.second.fingerprint)); });

hebasto commented at 3:00 PM on October 8, 2021: member

Concept ACK.

in src/external_signer.cpp:86 in d047ed729f

  87 | -    }
  88 | +        return false;
  89 | +    };
  90 |  
  91 | -    if (!match) {
  92 | +    if (!std::any_of(psbtx.inputs.begin(), psbtx.inputs.end(), matches_signer_fingerprint)) {

hebasto commented at 3:10 PM on October 8, 2021:

nit:

    if (!std::any_of(psbtx.inputs.cbegin(), psbtx.inputs.cend(), matches_signer_fingerprint)) {

hebasto approved
hebasto commented at 3:11 PM on October 8, 2021: member

ACK d047ed729f1d4732d23324fc76849f3c657cdbe4, I have reviewed the code and it looks OK, I agree it can be merged.
luke-jr referenced this in commit 87126b3be1 on Oct 10, 2021
laanwj merged this on Oct 22, 2021
laanwj closed this on Oct 22, 2021
theStack deleted the branch on Oct 22, 2021
sidhujag referenced this in commit 6709f42f0a on Oct 22, 2021
DrahtBot locked this on Oct 30, 2022

Contributors