Replace custom hex parsing with hex-conservative crate #259

Velnbur · 2025-11-01T20:13:54Z

Overview

Add hex-conservative as a dependency and refactor hex parsing code to use it instead of the custom implementation. Update FromHex trait to use associated error types and standardize hex decoding across the codebase.

Closes: #258

Velnbur · 2025-11-02T09:17:29Z

src/encode.rs

+    /// Hex fixed parsing errors
+    HexFixedError(DecodeFixedLengthBytesError),
+    /// Hex variable parsing errors
+    HexVariableError(DecodeVariableLengthBytesError),


Not really sure if it's okay to keep both this way. We can alternatively keep only one variant as before HexError(), add hex::Error which is merge of DecodeFixedLengthBytesError and DecodeVariableLengthBytesError and add From implementations

delta1 · 2025-11-03T07:51:44Z

Cargo.toml

 actual-serde = { package = "serde", version = "1.0.103", features = [
    "derive",
 ], optional = true }
+hex-conservative = "1.0.0"


Hi @Velnbur thanks for the PR, could you please regenerate the Cargo.lock file and include it as Cargo-latest.lock ?

delta1 · 2025-11-03T13:21:00Z

Please run cargo test, I'm seeing 2 test failures

failures:

---- pset::tests::single_blinded_output_pset stdout ----

thread 'pset::tests::single_blinded_output_pset' panicked at src/pset/mod.rs:863:54:
called `Result::unwrap()` on an `Err` value: SurjectionProofVerificationError(0)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- blind::tests::test_blind_tx stdout ----

thread 'blind::tests::test_blind_tx' panicked at src/blind.rs:1474:55:
called `Result::unwrap()` on an `Err` value: SurjectionProofVerificationError(0)

apoelstra · 2025-11-03T13:29:18Z

This is clearly a LLM and it's not even running the tests. Let's not waste our time on this.

Velnbur · 2025-11-03T18:40:25Z

Fixed tests, missed a part that blinding factors have reverse order of bytes. And yeah, I forgot to run tests as in corner of my mind I relied on CI which is the real source of truth that checks different versions of Rust, and was expecting to receive a notification from it where I broke something (also the fact that you store binaries of Elements and Bitcoin nodes in repo is really confusing). So, totally my fault

This is clearly a LLM

What made you think so? I made this changes without any AI tools. Except commit messages, I use automation tools to write them, shouldn't be a sin nowadays . May be some words like "for more robust hexadecimal encoding/decoding" sound AI-ish, kind of understand that

Also that's really mean that you just closed this PR without any time for me to response. I'm in different timezone, doing this in my free time and have relatively often electricity (and so network) shortages.

apoelstra · 2025-11-03T21:09:28Z

What made you think so? I made this changes without any AI tools. Except commit messages, I use automation tools to write them, shouldn't be a sin nowadays . May be some words like "for more robust hexadecimal encoding/decoding" sound AI-ish, kind of understand that

I apologize -- and for what it's worth, I don't mind the use of AI tools or even majority-LLM-authored PRs. What's a problem is that Github is now full of unsupervised agents which open broken PRs and then respond to any comments or requests by closing their PR and moving on. It's extremely irksome to be scammed by a machine into doing pointless work. (There are some technical limitations keeping us on Github but I hope to abandon it entirely within the next year or two because of this.)

For future reference -- what triggered me was mostly the "Overview" heading in your PR comment, which looks like LLM output (humans don't usually write PR descriptions in structured markdown), combined with the unit tests not passing (though humans do this all the time :P).

Velnbur · 2025-11-04T09:02:13Z

It's fine, I understand the issue now. Thanks for feedback.

"Overview" heading in your PR comment

Seems that I've learned that from LLMs :) . Originally I planned to describe there alternative approaches how to handle different errors from hex_conservative crate but from rust-bitcoin/bitcoin_hashes#124 it seems that to add associated Error type was actually the goal.

Also, what we would do with this PR? Do I need to recreate it or you can do that?

delta1 · 2025-11-04T09:27:06Z

Hi @Velnbur reopened, please address feedback

Velnbur · 2025-11-04T09:34:02Z

@delta1 Check please latest commits, I added Cargo-latest.lock and tests are passing (at least those that start with cargo test) as you asked. Am I missing something? My apologies if so

Velnbur · 2025-11-04T09:37:51Z

Okay, I see tests failing, working on it

Velnbur · 2025-11-04T10:01:29Z

src/serde_utils.rs

+        where
+            <B as FromHex>::Err: std::fmt::Display,


Added bounds here for associated Err type, as compiler suggested

Cargo-latest.lock

Velnbur · 2025-11-04T13:20:39Z

@delta1 I finally regenerated it correctly, sorry for inconveniences :)

apoelstra · 2025-11-04T13:23:06Z

Can you please squash/rebase these commits so that they compile by themselves? You need to update the lockfile in the same commit as you change Cargo.toml. Also, please don't make changes in one commit which are reverted in another commit.

Also, please don't update unrelated dependencies. You can do that in a separate PR if you want but it will require separate review.

Add `hex-conservative` crate as a dependency for more robust hexadecimal encoding/decoding. Replace custom hex parsing logic with the library's implementation across the codebase. Update error types to use `hex-conservative`'s error variants instead of custom `hex::Error`. Implement `TweakHexDecodeError` to handle hex and tweak validation errors for blinding factors. Add Display bound to FromHex error type in serde_util.

Velnbur · 2025-11-04T13:32:09Z

Got it, done! Squashed commits

Also, please don't update unrelated dependencies

My bad, created Cargo.lock from scratch, not from from existing Cargo-latest.lock

apoelstra · 2025-11-04T13:58:29Z

src/confidential.rs

+}
+
+#[doc(hidden)]
+impl From<hex_conservative::DecodeFixedLengthBytesError> for TweakHexDecodeError {


In 775d7cd:

We don't have to fix this here (it's a problem throughout the crate) but I would like to get rid of these From impls since they increase the API surface and encourage unthinking error promotion.

apoelstra · 2025-11-04T13:59:06Z

src/confidential.rs

+impl std::error::Error for TweakHexDecodeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            TweakHexDecodeError::InvalidHex(err) => Some(err),


In 775d7cd:

Also don't need to fix it here but we should use Self:: instead of writing TweakHexDecodeError:: all over the place.

apoelstra · 2025-11-04T14:00:13Z

src/confidential.rs

    }
 }

 impl hex::FromHex for AssetBlindingFactor {


In 775d7cd:

I don't think we should keep the FromHex trait. Where is it used? Can we replace it with FromStr or with a special-purpose utility trait?

I see, it's used for the serde hex_bytes module.

It seems to me that we can replace it with FromStr, introduce a sealed trait if we really want to limit its scope (but do we? I don't think so), and then we can drop hex-conservative 0.2 entirely.

Shouldn't than all fmt::Display implementations use hex as well? Asking cause it isn't so for Script at least:

rust-elements/src/script.rs

Line 58 in f6ffc78

fmt::Debug::fmt(self, f)

apoelstra · 2025-11-04T14:56:40Z

Shouldn't than all fmt::Display implementations use hex as well? Asking cause it isn't so for Script at least:

Ohh, right, hex-conservative 1.0 doesn't have encoding yet.

Ok, I guess we're stuck with 0.x for now. This PR is good as-is, given the circumstance.

Thanks for your patience and continued iterating!

apoelstra

ACK 775d7cd; successfully ran local tests

Velnbur marked this pull request as draft November 1, 2025 20:21

Velnbur force-pushed the master branch from 3536c2d to 33582ab Compare November 2, 2025 09:14

Velnbur commented Nov 2, 2025

View reviewed changes

Velnbur marked this pull request as ready for review November 2, 2025 09:17

Velnbur mentioned this pull request Nov 2, 2025

std::error::Error implementation for hex::Error #258

Closed

delta1 reviewed Nov 3, 2025

View reviewed changes

apoelstra closed this Nov 3, 2025

delta1 reopened this Nov 4, 2025

Velnbur commented Nov 4, 2025

View reviewed changes

delta1 reviewed Nov 4, 2025

View reviewed changes

Cargo-latest.lock Outdated Show resolved Hide resolved

Velnbur force-pushed the master branch from 2c337bc to 232ef1c Compare November 4, 2025 13:16

Velnbur force-pushed the master branch from 232ef1c to 775d7cd Compare November 4, 2025 13:29

apoelstra reviewed Nov 4, 2025

View reviewed changes

apoelstra approved these changes Nov 4, 2025

View reviewed changes

apoelstra merged commit 646f317 into ElementsProject:master Nov 4, 2025
8 checks passed

Replace custom hex parsing with hex-conservative crate #259

Replace custom hex parsing with hex-conservative crate #259

Conversation

Velnbur commented Nov 1, 2025

Overview

Uh oh!

Velnbur Nov 2, 2025 • 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!

delta1 Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

delta1 commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Nov 3, 2025

Uh oh!

Velnbur commented Nov 3, 2025

Uh oh!

apoelstra commented Nov 3, 2025

Uh oh!

Velnbur commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

delta1 commented Nov 4, 2025

Uh oh!

Velnbur commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Velnbur commented Nov 4, 2025

Uh oh!

Velnbur Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Velnbur commented Nov 4, 2025

Uh oh!

apoelstra commented Nov 4, 2025

Uh oh!

Velnbur commented Nov 4, 2025

Uh oh!

apoelstra Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Velnbur Nov 4, 2025 • 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!

apoelstra commented Nov 4, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Velnbur Nov 2, 2025 •

edited

Loading

delta1 commented Nov 3, 2025 •

edited

Loading

Velnbur commented Nov 4, 2025 •

edited

Loading

Velnbur commented Nov 4, 2025 •

edited

Loading

Velnbur Nov 4, 2025 •

edited

Loading