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

scgbckbone · 2022-04-28T18:02:09Z

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

prusnak · 2022-04-28T21:12:13Z

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 bitcoin-core/HWI#599 (comment)

scgbckbone · 2022-04-29T01:26:30Z

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

Sjors · 2022-04-29T09:23:00Z

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.

scgbckbone · 2022-04-29T15:41:17Z

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.

after enumeratesigners you get uppercase from ES
core changes this globaly to lowercase to fit its needs (imo bad)
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:

ExternalSigner::SignTransaction --> here it needs to be normalized (on the side of ES or core), core has somehow arbitrarily? chosen to encode lowercase
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:

as proposed here --> kind of already got 2 mehs from @prusnak @Sjors
core can instead normalize fingerprint retrieved from PSBT and check both upper and lowercase representation of hex string -> this PR
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

achow101 · 2022-04-29T20:39:26Z

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

scgbckbone · 2022-04-29T23:04:44Z

@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

Sjors · 2022-04-30T17:50:29Z

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

scgbckbone · 2022-05-01T02:50:27Z

squashed and rebased on top of current master

theStack

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

scgbckbone · 2022-05-05T23:17:35Z

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

theStack · 2022-05-06T13:14:53Z

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;

scgbckbone · 2022-05-06T16:45:50Z

tab character removed - linter happy

luke-jr · 2022-05-07T01:21:41Z

src/external_signer.cpp

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

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

… caring for lower/upper case in ExternalSigner::SignTransaction

luke-jr

utACK

theStack

Code-review ACK 2a22f03

Sjors

utACK 2a22f03

achow101 · 2022-05-16T19:49:43Z

ACK 2a22f03

… caring for lower/upper case in ExternalSigner::SignTransaction Github-Pull: bitcoin#25019 Rebased-From: 2a22f03

…rnalSigner::SignTransaction 2a22f03 parsing external signer master fingerprint string as bytes instead of caring for lower/upper case in ExternalSigner::SignTransaction (avirgovi) Pull request description: 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). ACKs for top commit: achow101: ACK 2a22f03 theStack: Code-review ACK 2a22f03 Sjors: utACK 2a22f03 Tree-SHA512: f3d84b83fb0b5e06c405eaf9bf20a2fa864bf4172fd4de113b80b9b9a525a76f2f8cf63031b480358b3a7666023a2aef131fb89ff50448c66df3ed541da10f99

scgbckbone mentioned this pull request Apr 28, 2022

master fingerprint should be lowercase bitcoin-core/HWI#599

Closed

laanwj added the Wallet label Apr 28, 2022

This was referenced Apr 29, 2022

normalize PSBT entry instead of external signer master fingerprint #25032

Closed

add explicit condition fo master fingerprint to be lowercase hex #25033

Closed

scgbckbone changed the title ~~normalize (lowercase) master fp in ExternalSigner::SignTransaction~~ parse external signer master fp as bytes in ExternalSigner::SignTransaction Apr 29, 2022

scgbckbone mentioned this pull request Apr 29, 2022

normalize master fp to lowercase bitcoin-core/HWI#600

Closed

scgbckbone force-pushed the lower_master_fp_of_ext_signer_in_SignTransaction branch 2 times, most recently from c79fd30 to 8f6049c Compare May 1, 2022 02:49

fanquake requested review from Sjors and achow101 May 5, 2022 08:35

theStack reviewed May 5, 2022

View reviewed changes

scgbckbone force-pushed the lower_master_fp_of_ext_signer_in_SignTransaction branch from 73de23e to e2a38b6 Compare May 5, 2022 23:16

scgbckbone force-pushed the lower_master_fp_of_ext_signer_in_SignTransaction branch from e2a38b6 to e7d7239 Compare May 6, 2022 07:42

scgbckbone force-pushed the lower_master_fp_of_ext_signer_in_SignTransaction branch from e7d7239 to 99671e5 Compare May 6, 2022 14:04

luke-jr suggested changes May 7, 2022

View reviewed changes

parsing external signer master fingerprint string as bytes instead of…

2a22f03

… caring for lower/upper case in ExternalSigner::SignTransaction

scgbckbone force-pushed the lower_master_fp_of_ext_signer_in_SignTransaction branch from 99671e5 to 2a22f03 Compare May 7, 2022 09:11

luke-jr approved these changes May 9, 2022

View reviewed changes

theStack approved these changes May 9, 2022

View reviewed changes

Sjors approved these changes May 10, 2022

View reviewed changes

Sjors mentioned this pull request May 10, 2022

Add external signer taproot support #23578

Merged

maflcko assigned achow101 May 10, 2022

achow101 merged commit 91a42d6 into bitcoin:master May 16, 2022

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

parsing external signer master fingerprint string as bytes instead of…

e154c68

… caring for lower/upper case in ExternalSigner::SignTransaction Github-Pull: bitcoin#25019 Rebased-From: 2a22f03

bitcoin locked and limited conversation to collaborators May 16, 2023

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

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

Uh oh!

Conversation

scgbckbone commented Apr 28, 2022

Uh oh!

prusnak commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scgbckbone commented Apr 29, 2022

Uh oh!

Sjors commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scgbckbone commented Apr 29, 2022

Uh oh!

achow101 commented Apr 29, 2022

Uh oh!

scgbckbone commented Apr 29, 2022

Uh oh!

Sjors commented Apr 30, 2022

Uh oh!

scgbckbone commented May 1, 2022

Uh oh!

theStack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scgbckbone commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented May 6, 2022

Uh oh!

scgbckbone commented May 6, 2022

Uh oh!

luke-jr May 7, 2022

Choose a reason for hiding this comment

Uh oh!

scgbckbone May 7, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

prusnak commented Apr 28, 2022 •

edited

Loading

Sjors commented Apr 29, 2022 •

edited

Loading

theStack left a comment •

edited

Loading

scgbckbone commented May 5, 2022 •

edited

Loading