adjust hex conversion on sv1 to use bitcoin::hashes::hex#2205
Conversation
lucasbalieiro
force-pushed
the
update-hex-conversion-sv1
branch
from
fbfd92f to
a75874c
Compare
| let json = r#"{ | ||
| "id": 34560326215225189, | ||
| "error": null, | ||
| "result": [ | ||
| [["", [-32088499539804252, {"": null}]], null, false], | ||
| "55555ʛ", | ||
| -1.0900715865557387e-175 | ||
| ] | ||
| }"#; |
There was a problem hiding this comment.
non-blocking clanker review:
This regression test includes multiple invalid fields (subscriptions shape and extra_nonce2_size as a negative float), so it may pass without exercising the non-ASCII extranonce decode path.
Suggestion: keep non-target fields valid (e.g. valid subscriptions + extra_nonce2_size: 4) so the test isolates the non-ASCII hex behavior.
| pub fn hex_decode_with_padding(s: &str) -> Result<Vec<u8>, Error<'static>> { | ||
| let padded; | ||
|
|
||
| let s = if s.len() % 2 != 0 { | ||
| padded = format!("0{s}"); | ||
| &padded | ||
| } else { | ||
| Vec::<u8>::from_hex(s).map_err(Error::HexError) | ||
| } | ||
| s | ||
| }; | ||
|
|
||
| hex_decode(s) | ||
| } | ||
|
|
||
| impl<'a> TryFrom<&str> for Extranonce<'a> { | ||
| type Error = error::Error<'a>; | ||
|
|
||
| fn try_from(value: &str) -> Result<Self, Error<'a>> { | ||
| Ok(Extranonce(B032::try_from(hex_decode(value)?)?)) | ||
| Ok(Extranonce(B032::try_from(hex_decode_with_padding(value)?)?)) |
There was a problem hiding this comment.
does any such scenario even happen, odd length hex string? discussion is too long, some tldr would be helpful.
And considering decode_with_padding is used on once, maybe inlining would be ok.
There was a problem hiding this comment.
TL;DR: At the time of the original issue, a faulty firmware was sending an odd-length hex extranonce, which caused errors in tproxy.
I agree that this scenario should not occur, and ideally we should reject invalid extranonce values instead of trying to "fix" them via left-padding (IMO).
however, following the history of the codebase, this behavior was intentionally carried over into the New TProxy work:
f0a69aa
If I remove the left-padding behavior now, it will break an existing unit test (which tells me that this is intentional). Since changing the semantics of Extranonce parsing is not the goal of this PR, I opted to preserve the current behavior. I also don't know whether any downstream users are relying on this behavior today, so keeping it seemed like the safer option for a targeted fix.
Regarding inlining hex_decode_with_padding, I wouldn't oppose that. My main concern is that, if we decide to keep left-padding in specific scenarios, we should avoid ending up in the same situation we had before, where hex decoding logic was scattered across the codebase with slightly different implementations.
One of the goals of this PR was to centralize hex decoding behind a small set of helpers so that any future changes to validation or decoding behavior can be made in a single place.
lucasbalieiro
force-pushed
the
update-hex-conversion-sv1
branch
from
6b9160b to
974696c
Compare
lucasbalieiro
force-pushed
the
update-hex-conversion-sv1
branch
from
974696c to
d501a1a
Compare
4 participants
closes #2201.
This PR replaces the use of
bitcoin_hashes@0.3.2, which can panic when processing certain multibyte characters, withbitcoin::hashes::hex.It also centralizes hex decoding behind a single utility function,
crate::utils::hex_decode. Prior to this change, there were multiple hex conversion implementations scattered throughout the codebase, making behavior harder to reason about and maintain consistently.While working on this issue, I also noticed inconsistent handling of odd-length hex strings. Some call sites would pad the input before decoding, while others would not.
After investigating the historical context (see #896), I introduced a dedicated wrapper named
hex_decode_with_padding. The goal is to make padding an explicit and deliberate choice rather than an implicit side effect hidden within the decoding logic.At the moment,
hex_decode_with_paddingis only used in theTryFromimplementation forExtranonce, which is the use case discussed in #896. The padding behavior forExtranoncealso appears to be required to satisfy the unit test introduced in #1791.