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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

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.