Test for `lineage.depth` overflow of the extended keys #1624

quapka · 2025-03-31T10:55:13Z

Hi, based on the discussion in #1623, I've decided to create a few test cases. I've manually created an extended private key with the depth set to 254 and then incremented it twice, checking that the first derivation passes and the second one fails (for various combinations of hd_private, hd_public and derive_private or derive_public).

I've split the tests into two commits due to, surprisingly, some variants failing. In particular, it seems to be possible to hit the overflow without and when deriving a public key of depth 256 from a private key of depth 255. Unfortunately, the tests are quite some copy-paste (I am happy to be transparent about this), but I could not see where the test (for the failing ones) would be incorrect.

quapka · 2025-03-31T20:28:54Z

If I read the output of the CI correctly, it runs all tests together as a single test as part of the installation in install-cmake.sh and thus the failure that I talked about is hidden :-/

evoskuil · 2025-03-31T20:31:59Z

https://github.com/libbitcoin/libbitcoin-system/actions/runs/14169924138/job/39725696803?pr=1624#step:10:958

cmake:
https://github.com/libbitcoin/libbitcoin-system/actions/runs/14169924138/job/39725696825#step:23:31

quapka · 2025-03-31T20:38:25Z

https://github.com/libbitcoin/libbitcoin-system/actions/runs/14169924138/job/39725696803?pr=1624#step:10:958

cmake: https://github.com/libbitcoin/libbitcoin-system/actions/runs/14169924138/job/39725696825#step:23:1

My bad, thanks! The two failing tests are indeed the ones I was talking about. The offending lines is this and this one.

The idea of the tests was to hit the overflow when going from 255 xpriv to 256/0 xpub. This code.

Oh, wait a minute. Isn't the derive_private(index) return value left unchecked and simple the to_public() is what gets returned, which succeeds even for the empty key?

evoskuil · 2025-03-31T20:39:48Z

No worries, in some of the CI outputs (CMake I think) you have to go to a different section to see the individual test failures.

quapka · 2025-03-31T20:56:33Z

Oh, wait a minute. Isn't the derive_private(index) return value left unchecked and simple the to_public() is what gets returned, which succeeds even for the empty key?

I am starting to believe that this is the case. The tests pass with the following patch:

diff --git a/src/wallet/keys/hd_private.cpp b/src/wallet/keys/hd_private.cpp
index fa9d7dc9c..0b4201c74 100644
--- a/src/wallet/keys/hd_private.cpp
+++ b/src/wallet/keys/hd_private.cpp
@@ -266,7 +266,12 @@ hd_private hd_private::derive_private(uint32_t index) const NOEXCEPT

 hd_public hd_private::derive_public(uint32_t index) const NOEXCEPT
 {
-    return derive_private(index).to_public();
+    const auto tmp = derive_private(index);
+    if (tmp) {
+        return tmp.to_public();
+    }
+
+    return {};
 }

 // Operators.

I am no C/C++, so the code is just something quickly stitched together. Wdyt?

evoskuil · 2025-03-31T21:15:20Z

That makes sense, I'll give it a look in a bit.

quapka · 2025-03-31T21:17:32Z

If my suggestion is indeed the problem, I wonder what a proper fix should be. Maybe verifying this within to_public() method? Which makes me wonder, how the C/C++ API of HD keys work and what are possibly all the other places, where operation are done on invalid keys.

evoskuil · 2025-03-31T21:22:15Z

I think people generally ignore secp invalidity and guard against deriving past the limit (since it's a guaranteed failure).

quapka · 2025-03-31T21:33:31Z

Do you also backport bug-fixes? I see the same thing in verson3.

evoskuil · 2025-03-31T21:48:20Z

Sure, if they're simple.

evoskuil · 2025-03-31T22:16:02Z

It's either:

hd_public hd_private::derive_public(uint32_t index) const NOEXCEPT
{
    const auto derived = derive_private(index);
    return derived ? derived.to_public() : hd_public{};
}

or:

hd_public hd_private::to_public() const NOEXCEPT
{
    if (!valid_)
        return {};
        ...
 }

as you suggested. I prefer the latter because it explicitly guards both scenarios which also benefits the case where the caller has not guarded a private key before conversion (as opposed to derivation) to public.

evoskuil · 2025-03-31T22:16:51Z

Feel free to make the change into both repos.

In case `hd_private` is invalid, the `to_public()` derivation should fail. The `hd_private` might be invalid from various reasons, such as deserializing invalid key or derived, using `derive_private` into an invalid one. Finally, nitpick: `return {};` is enclosed in curly brackets to make the if statement scope explicit.

quapka · 2025-04-01T09:22:09Z

hd_public hd_private::to_public() const NOEXCEPT
{
    if (!valid_)
        return {};
        ...
 }

Thanks for the code examples. I also like the second one more. I've added a couple more tests to also:

test explicitly for decoding null keys, both hd_private and hd_public
test to_public() called on invalid hd_private arising from other reasons, as you mentioned.
Fixed in master
Backport fix to version3

evoskuil

Could you please squash these to a single commit?

evoskuil · 2025-04-02T01:38:45Z

src/wallet/keys/hd_private.cpp

+
+    if (!valid_) {
+        return {};
+    }


Avoid bracing for single line (also braces would be balanced).

I like to be explicit about the if scope, as I have seen a new code added (maybe under the influence of Python-like, whitespace aware languages), without adding the braces. Tests should hopefully catch that though.

https://github.com/libbitcoin/libbitcoin-system/wiki/Style-Guide

evoskuil · 2025-04-02T01:41:02Z

test/wallet/keys/hd_private.cpp

@@ -128,4 +128,112 @@ BOOST_AUTO_TEST_CASE(hd_private__derive_public__long_seed__expected)
    BOOST_REQUIRE_EQUAL(m0xH1yH2_pub.encoded(), "xpub6FnCn6nSzZAw5Tw7cgR9bi15UV96gLZhjDstkXXxvCLsUXBGXPdSnLFbdpq8p9HmGsApME5hQTZ3emM2rnY5agb9rXpVGyy3bdW6EEgAtqt");
 }

+BOOST_AUTO_TEST_CASE(hd_private__constructor__null_key_decodes_to_invalid__expected)


This:

hd_private__constructor__null_key_decodes_to_invalid__expected

would be better as:

hd_private__constructor__null_key__decodes_to_invalid

Others as well. Third part is the thing being tested and fourth part is the expectation.

Thanks, will improve.

evoskuil · 2025-04-02T01:44:37Z

test/wallet/keys/hd_public.cpp

+    BOOST_REQUIRE_EQUAL(xpub_255.lineage().depth, 255);
+    BOOST_REQUIRE(xpub_255);
+
+    // depth overflows uint from 255 to 0


I don't think it's actually overflowing here, it should be the default (invalid/null) key, and the depth value for that happens to be zero.

evoskuil · 2025-04-13T20:25:26Z

Moved to #1637

quapka added 2 commits March 31, 2025 12:47

Test for extended key depth overflow

188609e

Add failing tests for extended key depth overflow

b290f4f

quapka mentioned this pull request Mar 31, 2025

Extended key lineage.depth overflows and produces garbage key #1623

Closed

quapka added 3 commits April 1, 2025 11:14

Test for deriving public from invalid extended private key

1db5f42

Test decoded null keys yields invalid keys

44807d2

Remove accidentally commited dev env files

46ecf36

quapka mentioned this pull request Apr 1, 2025

Backport fix for deriving public keys from private ones to version3 #1627

Open

Put tests into the correct files

77aab49

evoskuil requested changes Apr 2, 2025

View reviewed changes

quapka mentioned this pull request Apr 2, 2025

BIP32: Maximal length of derivation paths and depth of extended keys bitcoin/bitcoin#32201

Open

evoskuil closed this Apr 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test for `lineage.depth` overflow of the extended keys #1624

Test for `lineage.depth` overflow of the extended keys #1624

quapka commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025 •

edited

Loading

quapka commented Mar 31, 2025 •

edited

Loading

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

evoskuil commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Apr 1, 2025 •

edited

Loading

evoskuil left a comment

evoskuil Apr 2, 2025

quapka Apr 2, 2025

evoskuil Apr 2, 2025

evoskuil Apr 2, 2025

quapka Apr 2, 2025

evoskuil Apr 2, 2025

evoskuil commented Apr 13, 2025

Test for lineage.depth overflow of the extended keys #1624

Test for lineage.depth overflow of the extended keys #1624

Conversation

quapka commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025 • edited Loading

quapka commented Mar 31, 2025 • edited Loading

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Mar 31, 2025

evoskuil commented Mar 31, 2025

evoskuil commented Mar 31, 2025

evoskuil commented Mar 31, 2025

quapka commented Apr 1, 2025 • edited Loading

evoskuil left a comment

Choose a reason for hiding this comment

evoskuil Apr 2, 2025

Choose a reason for hiding this comment

quapka Apr 2, 2025

Choose a reason for hiding this comment

evoskuil Apr 2, 2025

Choose a reason for hiding this comment

evoskuil Apr 2, 2025

Choose a reason for hiding this comment

quapka Apr 2, 2025

Choose a reason for hiding this comment

evoskuil Apr 2, 2025

Choose a reason for hiding this comment

evoskuil commented Apr 13, 2025

Test for `lineage.depth` overflow of the extended keys #1624

Test for `lineage.depth` overflow of the extended keys #1624

evoskuil commented Mar 31, 2025 •

edited

Loading

quapka commented Mar 31, 2025 •

edited

Loading

quapka commented Apr 1, 2025 •

edited

Loading