key: validate BIP32 seed length in CExtKey::SetSeed #35320

BIP32 specifies that the seed must be between 128 and 512 bits (16 to 64 bytes). CExtKey::SetSeed currently accepts any length, which could result in weak master keys being generated. Add an Assert at the start of SetSeed to enforce the valid seed length range as a programming invariant. The existing BIP32 test vectors already provide sufficient coverage of valid seed lengths.

To test:

cmake --build build -j --target test_bitcoin ./build/bin/test_bitcoin --run_test=bip32_tests

Fixes #35308

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35320.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Two things:

1. I believe return is the wrong failure handling. Silently returning leaves the CExtKey in an uninitialized/partial state with no way for the caller to detect the rejection. Since passing an out-of-spec seed is a programming error with no safe recovery, Assert(seed.size() >= 16 && seed.size() <= 64) is more appropriate because it makes the contract explicit and terminates loudly on violation rather than silently producing a broken object.

2. The tests belongs in src/test/bip32_tests.cpp. That file already exercises CExtKey::SetSeed directly and is the natural home for BIP32 seed validation tests.

Two things:

1. I believe return is the wrong failure handling. Silently returning leaves the CExtKey in an uninitialized/partial state with no way for the caller to detect the rejection. Since passing an out-of-spec seed is a programming error with no safe recovery, Assert(seed.size() >= 16 && seed.size() <= 64) is more appropriate because it makes the contract explicit and terminates loudly on violation rather than silently producing a broken object.
2. The tests belongs in src/test/bip32_tests.cpp. That file already exercises CExtKey::SetSeed directly and is the natural home for BIP32 seed validation tests.

Yeah, very much agree!.

Checked it out - I'll move the tests.

1. nit: I believe that swapping the ordering would improve readability, Assert(16 <= seed.size() && seed.size() <= 64)

2. The commit message needs to be updated as well to match the proposed functionality, it still mentions the early return.

3. The tests are lacking, especially since they do not test arguably the most problematic part which are short seeds. The BOOST_CHECK_THROW (or its variants, I am not familiar with Boost) could be used to test too short and too long seeds.

Thanks for the feedback @quapka, @achow101 The added test cases were handled already with the test vectors. For invalid test cases, Assert calls assertion_fail which aborts, seems BOOST_CHECK_THROW and its variant can't catch it. I don't know if adding a fuzz test seem like a good option

Assertions enforce invariants and are there to reveal programming bugs outside of tests. This does not need any new tests.

Assertions enforce invariants and are there to reveal programming bugs outside of tests. This does not need any new tests.

Got it

Removed the test case per @achow101 feedback, the existing BIP32 test vectors provide sufficient coverage and Assert is there to enforce the invariant, not to be tested directly. Updated the commit to only include the Assert in key.cpp.