rest: Reject truncated hex txid early in getutxos parsing #30482

nit: or just initialize directly?

<details> <summary>git diff on 0d57f058e4</summary>

diff --git a/src/test/pow_tests.cpp b/src/test/pow_tests.cpp
index efca540240..0a9137636a 100644
--- a/src/test/pow_tests.cpp
+++ b/src/test/pow_tests.cpp
@@ -83,31 +83,28 @@ BOOST_AUTO_TEST_CASE(get_next_work_upper_limit_actual)
 BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_negative_target)
 {
     const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus();
-    uint256 hash;
+    uint256 hash{1};
     unsigned int nBits;
     nBits = UintToArith256(consensus.powLimit).GetCompact(true);
-    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 
 BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_overflow_target)
 {
     const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus();
-    uint256 hash;
+    uint256 hash{1};
     unsigned int nBits{~0x00800000U};
-    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 
 BOOST_AUTO_TEST_CASE(CheckProofOfWork_test_too_easy_target)
 {
     const auto consensus = CreateChainParams(*m_node.args, ChainType::MAIN)->GetConsensus();
-    uint256 hash;
+    uint256 hash{1};
     unsigned int nBits;
     arith_uint256 nBits_arith = UintToArith256(consensus.powLimit);
     nBits_arith *= 2;
     nBits = nBits_arith.GetCompact();
-    hash = uint256{1};
     BOOST_CHECK(!CheckProofOfWork(hash, nBits, consensus));
 }
 

</details>

Thanks, but I'll leave it as-is for now.

Man, I was worried for a moment that this modification of switching to the uint8_t ctor was setting the other end of the base_blob array, but it's the same. Will be glad to see the fragile function go. :+1:

review note nit: perhaps worth adding in the commit message that this implementation is ~identical to ParseHashStr (even though it's later deleted)?

Thanks, edited commit message

Perhaps worth it to introduce the FromHex method you mentioned here already, as it's not much extra LoC and probably makes for a more final interface? We can just do it for transaction_identifier now and do base_blob later?

<details> <summary>git diff on 5ac06abeb9</summary>

diff --git a/src/rest.cpp b/src/rest.cpp
index b0d0d928f6..ff06c4191f 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -792,14 +792,14 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
             if (txid_out.size() != 2) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
             }
-            Txid txid;
             auto output{ToIntegral<uint32_t>(txid_out.at(1))};
 
-            if (!txid.SetHex(txid_out.at(0)) || !output ) {
+            auto txid {Txid::FromHex(txid_out.at(0))};
+            if (!txid || !output ) {
                 return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
             }
 
-            vOutPoints.emplace_back(txid, *output);
+            vOutPoints.emplace_back(txid.value(), *output);
         }
 
         if (vOutPoints.size() > 0)
diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h
index c1b46ea39c..68182b6332 100644
--- a/src/util/transaction_identifier.h
+++ b/src/util/transaction_identifier.h
@@ -37,6 +37,11 @@ public:
     bool operator<(const Other& other) const { return Compare(other) < 0; }
 
     const uint256& ToUint256() const LIFETIMEBOUND { return m_wrapped; }
+    static std::optional<transaction_identifier> FromHex(std::string_view hex)
+    {
+        transaction_identifier result;
+        return result.SetHex(hex) ? std::make_optional(result) : std::nullopt;
+    }
     static transaction_identifier FromUint256(const uint256& id) { return {id}; }
 
     /** Wrapped `uint256` methods. */

</details>

On a second thought, I think the SetHex is preferable in this case, because the code using std::optional requires more typing (minimally more) when parsing and when unwrapping. Given the [[nodiscard]] approach, both should be equally safe, so I picked the approach using less code.

(In response to #30482 (review)): To see how FromHex would look, I implemented a commit on top of this PR that does that transformation: 404c115ea0f53785a640818c2058ef0591c8c6ac

It turned out well enough that I hope you will at least consider it. Could use more auto to reduce typing. In my mind it's worth sacrificing some typing and a slightly larger diff to get clarity and RAII.

Ok, done, went with my original suggestion :sweat_smile:

We might as well use the [[deprecated]] attribute instead:

[[deprecated("Missing length-check and hex-check, please use the safer SetHex, or FromUint256")]]

Resulting in compilation warnings for the remaining usages:

bitcoin/src/qt/coincontroldialog.cpp:345:25: warning: 'TxidFromString' is deprecated: Missing length-check and hex-check, please use the safer SetHex, or FromUint256 [-Wdeprecated-declarations] COutPoint outpt(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt());

The build logs may be too verbose then, but I'll reconsider it in a follow-up, when more work has landed.

I don't see the point of bloating the build logs with this, a tracking issue seems more appropriate if we're worried it'll be forgotten.

I don't see why silencing warnings and putting them to dead comments that cannot bite back is better than "bloating the logs"

Again, I may fix this in a follow-up. Right now the focus is on the bugfix explained in the pull request description.

👍, I follow-up PR makes sense here

When a method both mutates and returns, we often extract it, to clarify that it's not just a simple pure check.

    bool isValid = hash.SetHex(hashStr);
    if (!isValid) {

isValid

For new code, that'd need to be snake_case. Other than that, I think it is clear that a mutable method called Set...(...) mutates, so there is no need to change the code here. Also, the dev guide doesn't mention it.

What's the reason for inverting the clauses, previously if !output we short-circuited the check, now we're attempting to set the hex value even when output is empty.

This shouldn't make a difference in the happy path. Optimizing for the non-happy path seems useless, as no user should be using it (outside of a few test cases).

Sure, but you did change it, so unless there's any particular reason, I think the original order makes more sense

The format string is txid_num, so I kept the order.

+1, trims the same whitespace values as https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L169 before

We shouldn't need zeroing, if we're processing the values exhaustively - but if we do, we could use

    SetNull();

I am not touching this line of code, so it seems unrelated to refactor it.

posted to #30436, please resolve it here

previously we've only removed leading whitespaces, but TrimStringView removed trailing ones as well. Not sure if this changes the overall behavior, though.

It does not, because trailing non-hex was always skipped.

I find it quite confusing that we're sneaking in a mutation of the copy, while previously we just returned a copy directly (like TrimStringView). We could const the parameters if we did the same as before:

        return str.substr(prefix.size());

i.e.

[[nodiscard]] inline std::string_view RemovePrefixView(const std::string_view str, const std::string_view prefix)

Could you please add a test case that checks the integrity of the original:

    auto original = "0x123";
    BOOST_CHECK_EQUAL(RemovePrefixView(original, "0x"), "123");
    BOOST_CHECK_EQUAL(original, "0x123"); // Ensure original string is intact

Same for TrimStringView, just to be sure.

Should probably be done in #30482 (comment), or in a separate pull reqeust. (The method already exists, and if the tests do not exists, one can add them today on master).

posted to #30436, please resolve it here

    [[deprecated("Accepting any invalid input, thus fragile, please use the safer SetHex instead")]]

(Same as above)

SetHex sounds like we're setting the field hex to the given parameter. But this isn't a simple setter, it parses and makes decisions (e.g. trim, skip values after an invalid char), so it's more like a factory method, right? In other cases we prefix these with From instead, see from_hex

No, it is not a factory method, because it does not return an object. I kept the previous name, but I am happy to change it, if there is a reason.

The reason is that it's not a simple setter, but a complex method that changes the internal state.

it parses and makes decisions (e.g. trim, skip values after an invalid char),

No, it does not trim or skip values. Either is sets the internal state, or it does not.

will leave it up to you, but if I see a Set.* I assume it's a property setter (which has an equivalent Getter), not that something is calculated and internal state (maybe) modified accordingly.

will leave it up to you, but if I see a Set.* I assume it's a property setter (which has an equivalent Getter)

Yes, there is a corresponding GetHex.

You mean it's a "virtual property" - I can work with that, thanks, resolve it please. (though it's weird that the getter seems to reverse the hex back to big-endian, weird, but I guess that's for another PR)

This isn't actually fragile (i.e. breaks easily), quite the opposite, it's rather lenient, i.e. accepts unvalidated input. Could we name it as FromHexUnchecked or FromHexLenient instead?

With "fragile" I mean that it breaks code that uses it and leads to bugs. See for example the bug that was fixed in this pull request.

I understood, that's why I recommended the name change. We don't say trim is fragile, just because more values become valid after they're trimmed, we call it lenient, right?

because more values become valid

Those values are not "valid". For example, why would turning aa into 00aa, or aa00 be valid? Both should be rejected and code using it is fragile.

Happy to rename it to SetHexUnchecked, but I don't think it matters much, as the code is deprecated anyway.

I think it would help a bit, but I'm more concerned about the setter name, when in fact it's a complex method, changing internal state, it's why I recommended something like FromHexUnchecked (the naming suggestion also applies to the non-deprected method calling this, of course).

-Fragile feels slightly off to me as well. -Unchecked seems to imply that the function itself may crash. -Lenient feels closer to the truth but sounds safe to use.

I recently came across MakeRandDeterministicDANGEROUS(), maybe something like that suffix would work?

DANGEROUS

That may also imply the function is crashing or has some internal hidden danger, which seems odd because it simply lacks a return value to indicate success or failure? Maybe just ...Deprecated()?

It is dangerous because input isn't length-checked, meaning extra hex chars may be ignored, and an accidentally short copy-paste of hex chars will be silently padded with zeroes.

Maybe just ...Deprecated()?

As in #30482?notification_referrer_id=NT_kwDOCkdNarUxMTU3NjYyODE4MzoxNzI0NDUwMzQ#discussion_r1685726873, I wonder what the policy around deprecation is. Are there prior examples of deprecation on a non-RPC level you can point to?

Only src/util/time.h, but those are just type-related; And src/logging.h, but those are just aliases; So I don't think they apply here.

Happy to rename it to SetHexDeprecated, but I don't think it matters much, as the code is deprecated anyway.

But production code calls it indirectly, so it's not exactly deprecated, just direct access is discouraged, right?

Yes, happy to rename it to SetHexDiscouraged, but I don't think it matters much, as the code is deprecated to be called directly anyway.

I'm fine with that if you are

Only src/util/time.h, but those are just type-related; And src/logging.h, but those are just aliases; So I don't think they apply here.

Thanks, those are good examples. I guess they are/were used more widely and directly compared to SetHex(), making deprecation a more obvious approach. If you deem this sufficiently big to break up into multiple PRs, I trust your judgement.

SetHexDeprecated or SetHexDiscouraged work for me.

shouldn't this remain SetHex here?

Thanks, fixed the comment in this test!

you may want to revert it in the commit that introduced it instead

you may want to revert it in the commit that introduced it instead

It is a scripted-diff, so it is correct and self-consistent, no?

ah, got it, sure!

I see that BOOST_CHECK was use throughout, but we could use BOOST_CHECK_EQUAL in a few places for better error messages:

    BOOST_CHECK_EQUAL(uint256S("\t \n  \n \f\n\r\t\v\t   0x"+R1L.ToString()+"  \t \n  \n \f\n\r\t\v\t "), R1L);

Happy to review a pull request doing this, but I won't be doing it myself.

posted to #30436, please resolve it here

I see this uses little-endian encoding, while e.g. TryParseHex uses big-endian, right? Do you happen to know what the reason is? Should we maybe document this?

I am not changing the endian behavior in this instance, so maybe a separate pull can improve the documentation?

posted to #30436, please resolve it here

previous version was cleaner in my opinion. Since str is random access, we could simplify to:

    size_t digits = 0;
    while (digits < trimmed.size() && ::HexDigit(trimmed[digits]) != -1) ++digits;

See #30482 (comment)

posted to #30436, please resolve it here

applying the suggestions below would look like this:

template <unsigned int BITS>
void base_blob<BITS>::SetHexFragile(const std::string_view str)
{
    const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");

    size_t digits = 0;
    while (digits < trimmed.size() && ::HexDigit(trimmed[digits]) != -1) ++digits;

    for (auto it = m_data.begin(); it < m_data.end(); ++it) {
        unsigned char temp = (digits > 0) ? ::HexDigit(trimmed[--digits]) : 0;
        *it = (digits > 0) ? temp | (::HexDigit(trimmed[--digits]) << 4) : temp;
    }
}

See #30482 (comment)

posted to #30436, please resolve it here

let's add a test where TryParseHash would behave differently, e.g. "0123456789 ABCDEF" where TryParseHex continues, SetHexFragile stops:

TryParseHex: 0123456789abcdef
SetHexFragile: efcdab8967452301000000000000000000000000000000000000000000000000

TryParseHex

This pull request doesn't modify (or mention) this helper, so I think it is unrelated. Also, the test already exist in:

src/test/util_tests.cpp:    result = ParseHex("12 34 56 78");
src/test/util_tests.cpp:    result = ParseHex(" 89 34 56 78");
src/test/util_tests.cpp:    result = ParseHex("     Ff        aA    ");
src/test/util_tests.cpp:    result = ParseHex("");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(ParseHex("AAF F").size(), 0);
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(ParseHex(with_embedded_null).size(), 0);
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(ParseHex("1234 invalid 1234").size(), 0);
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(ParseHex("12 3").size(), 0);

posted to #30436, please resolve it here

What is your reasoning for deprecating rather than removing and correcting call-sites to not use TxidFromString - is that part of why this PR is a draft? I can't see many other deprecated things in the codebase beyond RPCs.

If you keep it, TxidFromString should probably be renamed TxidFromStringFragile to follow SetHexFragile, but I understand you want to avoid touching src/qt/ source files as much as possible. Maybe one could at least change the implementation to make it clearer that it is "fragile" there?

    uint256 wrapped;
    wrapped.SetHexFragile(str);
    return Txid::FromUint256(wrapped);

What is your reasoning for deprecating rather than removing and correcting call-sites

I don't think fixing all bugs in all places in a single pull request makes sense. Often, context specific fixes are needed, so separate pull requests can be used.

is that part of why this PR is a draft?

It is draft, because it includes a merge commit of other commits.

If you keep it, TxidFromString should probably be renamed TxidFromStringFragile to follow SetHexFragile

Sure, but this will be done in a follow-up.

Makes sense to do the deprecation removal in a separate PR

Noticed ParseHashV(hash_str, ...) is needlessly called a few lines below instead of using the already verified hash value.

Nice, removed!

is this exactly the same as before?

Why not?

I was asking because ParseHashStr returned a bool before and FromHex returns an optional, was wondering if it had some weirdness that I didn't know about (like how JavaScript's truthy has), e.g. take the internal value into consideration in some cases or whatever. I checked it now (was reviewing the other one from my phone), it seems to behave in the same way indeed.

hmmm, what's the point of the Assert inside?

To avoid UB on a nullopt deref. But happy to remove it, if you think there is a benefit in removing it.

But happy to remove it, if you think

ok, but seems a bit passive-aggressive to resolve all such comments in that case.

Wouldn't this achieve the same in a more idiomatic way?

        auto prev_filter_header_basic{uint256::FromHex(test[pos++].get_str()).value()};

This will fail for

static std::optional<uint256> FromHex(std::string_view str) { return std::nullopt; }

with

std::bad_optional_access: bad_optional_access

which is more specific than

signal: SIGABRT (application abort requested)

ok, but seems a bit passive-aggressive to resolve all such comments in that case.

I am sorry you feel that way, but I don't see value in review comments or questions that can be answered by the person asking the question. (Assert is pretty well documented, see git grep Assert doc/developer-notes.md)

Regardless, I did answer the question and then resolved the conversation, which seems reasonable for a question.

which is more specific than

I think both are fine. As long as the test fails in some way, an error is visible and finding it should be trivial in either case.

So I consider it a style-nit.

not sure I get the SetHexFragile/FromHex naming duality here :/

The duality will be removed in a follow-up, when the deprecated on is removed

after we get rid of the deprecation, these checks should ideally be done while iterating over the values to avoid multiple iterations (e.g. how Base58 parsing is done)

Maybe. Seems best to leave for the follow-up that does it.

Yes, definitely different PR

nit: Missed opportunity to convert to curly braces? :)

Same below for COutPoint.

Heh, may do if I retouch for other reasons.

No, see:

bitcoin-tx.cpp:288:23: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'uint32_t' (aka 'unsigned int') in initializer list [-Wc++11-narrowing]
  288 |     CTxIn txin{*txid, vout, CScript(), nSequenceIn};
      |                       ^~~~
bitcoin-tx.cpp:288:23: note: insert an explicit cast to silence this issue
  288 |     CTxIn txin{*txid, vout, CScript(), nSequenceIn};
      |                       ^~~~
      |                       static_cast<uint32_t>( )
bitcoin-tx.cpp:637:34: error: non-constant-expression cannot be narrowed from type 'int' to 'uint32_t' (aka 'unsigned int') in initializer list [-Wc++11-narrowing]
  637 |             COutPoint out{*txid, nOut};
      |                                  ^~~~
bitcoin-tx.cpp:637:34: note: insert an explicit cast to silence this issue
  637 |             COutPoint out{*txid, nOut};
      |                                  ^~~~
      |                                  static_cast<uint32_t>( )
2 errors generated.
make[2]: *** [Makefile:16088: bitcoin_tx-bitcoin-tx.o] Error 1
make[2]: *** Waiting for unfinished jobs....

Attempted cleaning that up, but didn't uncover anything useful to the end users, unlike #30444, so probably not worth following up on:

diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 89c03c1647..dab8b81128 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -274,8 +274,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
 
     // extract and validate vout
     const std::string& strVout = vStrInputParts[1];
-    int64_t vout;
-    if (!ParseInt64(strVout, &vout) || vout < 0 || vout > static_cast<int64_t>(maxVout))
+    const auto vout{ToIntegral<uint32_t>(strVout)};
+    if (!vout || *vout > maxVout)
         throw std::runtime_error("invalid TX input vout '" + strVout + "'");
 
     // extract the optional sequence number
@@ -285,7 +285,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
     }
 
     // append to transaction input list
-    CTxIn txin(*txid, vout, CScript(), nSequenceIn);
+    CTxIn txin{*txid, *vout, CScript(), nSequenceIn};
     tx.vin.push_back(txin);
 }
 
@@ -634,7 +634,7 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
             if (nOut < 0)
                 throw std::runtime_error("vout cannot be negative");
 
-            COutPoint out(*txid, nOut);
+            COutPoint out{*txid, static_cast<uint32_t>(nOut)};
             std::vector<unsigned char> pkData(ParseHexUV(prevOut["scriptPubKey"], "scriptPubKey"));
             CScript scriptPubKey(pkData.begin(), pkData.end());

+            COutPoint out{*txid, static_cast<uint32_t>(nOut)};

Thanks for looking again. For reference, getInt has a built in check, so you could use getInt<uint32_t> and avoid this cast. That'd be a behavior change for out-of-range values, but the values are out of range anyway, so should be fine.

Not sure whether we should only do FromHex testing or do it in addition to SetHexDeprecated testing.

Sure, we want to remove SetHexDeprecated but it is unclear when or how easily it could be done, do you have an exploratory branch doing the removal?

It is still tested in this test case (see above), and it other test cases (indirectly). Am I missing something, or is there other missing test coverage?

There's only 1 remaining explicit call to SetHexDeprecated in this test case, before there were 7 calls to SetHex. But I guess almost the same tests are done indirectly in the basics test case above where SetHexDeprecated is being called via uint<N>S.

Curious: Why introduce this namespace instead of making a protected method in base_blob as I did in my exploration (404c115ea0f53785a640818c2058ef0591c8c6ac)?

I don't think it matters much either way, because it is an internal implementation detail. It doesn't need access to private or protected members, so standalone seems better. Otherwise, there could be confusion when calling base_blob<160>::FromHex<uint256>(hex), as to what values are used. Also, if the function is changed to access the private or protected members, it may be moved to the cpp file, or otherwise "optimized", so spending too much time here now is probably not worth it.

Happy to change, if there is a reason.

Hm.. yeah, to me the namespace itself is a cognitive speed-bump but I see your point about keeping to the public members.

Ok, resolving for now. Happy to change if there is a convincing reason.

Why not have all FromHex methods be [[nodiscard]]? Seems bad to throw away work.

The attribute should only be added to functions where ignoring the return value will lead to an error later on in the code. See #27774 (comment)

Otherwise, if it was added to all functions that return something, it would instead be better to add it to no function and enforce the behavior via a language plugin, for example via clang-tidy.

In principle all pure functions like FromHex (that at most mutate input-parameters and don't have asserts/throw) should be [[nodiscard]]. One could at least apply that rule to new code?

I guess it's easy for the compiler to eliminate the call if it's indeed pure and the result isn't used - but even better if there's a warning as well

In principle all pure functions like FromHex (that at most mutate input-parameters and don't have asserts/throw) should be [[nodiscard]]. One could at least apply that rule to new code?

If there is a convincing reason to do this, the rule should be added to the dev notes. However, absent a convincing reason and absent the style rule being in the dev notes, I will leave this as-is for now.

Should use BOOST_CHECK_EQUAL_COLLECTIONS().

Why? This blob of code is untouched in this pull request, so if there is a reason to change it, it should be done in a separate pull request.

Ok, previously it was BOOST_CHECK_EQUAL(TmpL, uint256());. I may change it to that, if I have to re-touch for other reasons. Otherwise, it can be done in one of the follow-ups.

Yeah, sorry, BOOST_CHECK_EQUAL(). Seems to be a mis-merge on your part when rebasing. Considering I took the time to add a whole extra commit to convert away from BOOST_CHECK in this file recently, I would be quicker to ACK if you fixed the merge.

+1, gives way better errors on failure

done

nit: we're not using SetHexDeprecated here, right?

BOOST_AUTO_TEST_CASE(methods) // GetHex FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize

It is

nit: the conditions in the code are actually reversed

Reordered, because I changed the comment anyway in another push.

as hitned before, this seems like a slight behavior change: previously if txid_out.at(1) was invalid, txid_out.at(0) wasn't checked. But I guess it's just extra work at most, no side-effects should follow from running IsHex, right?

no side-effects should follow from running IsHex, right?

Yes, IsHex is pure.

aren't we mixing big and little endian here? Is this by design? As mentioned in the other PR, I think these unintuitive differences should definitely be documented...

I am not changing this line of code, or the behavior of this test, so a separate pull request seems more appropriate.

Would be more robust to use .value() here. Suggestion that also skips unnecessary assignment to TmpL:

<details> <summary>git diff on fa479be4d8</summary>

diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index 52a8dbc200..430fc14892 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -168,12 +168,10 @@ BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() s
     // Verify previous values don't persist when setting to truncated string.
     TmpL.SetHexDeprecated("21");
     BOOST_CHECK_EQUAL(TmpL.ToString(), "0000000000000000000000000000000000000000000000000000000000000021");
-    TmpL = *uint256::FromHex(R2L.ToString());
-    BOOST_CHECK_EQUAL(TmpL, R2L);
-    TmpL = *uint256::FromHex(ZeroL.ToString());
-    BOOST_CHECK_EQUAL(TmpL, uint256());
+    BOOST_CHECK_EQUAL(uint256::FromHex(R2L.ToString()).value(), R2L);
+    BOOST_CHECK_EQUAL(uint256::FromHex(ZeroL.ToString()).value(), uint256());
 
-    TmpL = *uint256::FromHex(R1L.ToString());
+    TmpL = uint256::FromHex(R1L.ToString()).value();
     BOOST_CHECK_EQUAL_COLLECTIONS(R1L.begin(), R1L.end(), R1Array, R1Array + R1L.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(TmpL.begin(), TmpL.end(), R1Array, R1Array + TmpL.size());
     BOOST_CHECK_EQUAL_COLLECTIONS(R2L.begin(), R2L.end(), R2Array, R2Array + R2L.size());

</details>

Ok, done in both places

in eeeeb0474d439c8d7e5ea9a680119245a455b5a0: should now be "please use the safer FromHex" since recent force push

Good catch. Fixed the typo!

nit: should we add this docstring to uint256S too?

Yes, done.

Since this is a potentially breaking interface change for downstream applications, adding release notes might be appropriate?

edit: perhaps interface change is not the right description, since hex txids really always have required 64 hex chars by definition. Not sure where we draw the line here. I'd be okay with not having release notes for this too, but doesn't hurt to have it either. Up to you.

<details> <summary>git diff on fa2d94a01c</summary>

diff --git a/doc/release-notes-30482.md b/doc/release-notes-30482.md
new file mode 100644
index 0000000000..6eb8af3f69
--- /dev/null
+++ b/doc/release-notes-30482.md
@@ -0,0 +1,6 @@
+Updated REST APIs
+-----------------
+- Parameter validation for `/rest/getutxos` has been improved by
+  rejecting truncated or overly large txids by raising an
+  HTTP_BAD_REQUEST "Parse error". Previously, these malformed txids
+  would be silently ignored.

</details>

Sure, done for both pulls.

If someone is looking for follow-up work, a test-only change could be to replace TxidFromString(a) with Txid::FromHex(a).value() in src/test.

git grep TxidFromString ./src/test/
git grep WtxidFromString

I've started working on a branch that removes (W)TxidFromString entirely, it seems the qt callsites would work well with FromHex too.

Done in #30532. Also started working on a similar branch to remove uint256S.