kernel: expose btck_block_tree_entry_get_ancestor #34885

It seems like passing a height < 0 or > block_tree_entry's height is a programming error, which in this interface we typically handle with assert rather than making it part of the interface (i.e. with nullptr). I'm aware btck_chain_get_by_height does the same thing, but I'm wondering if we should update both? That would change the wrapper signature to (a throwing) BlockTreeEntry BlockTreeEntry::GetAncestor(int32_t height) @sedited as the author of btck_chain_get_by_height, is there a reason we shouldn't use assert here?

@sedited as the author of btck_chain_get_by_height, is there a reason we shouldn't use assert here?

I'd presume that the caller can not ensure that height really does still exist when the call is done (there could be a massive, but unlikely re-org on the main chain?) Whereas in this case here, the chain is fixed to be the one referred to be by the passed block_tree_entry, which has a constant and known height?

What @maflcko said :)

Oh yes of course, btck_Chain is the active chain, I forgot about that. So in that case I agree we should leave btck_chain_get_by_height as-is, but update btck_block_tree_entry_get_ancestor to have the correct height as a precondition.

Addressed in b10b85f

Since we have the same check in GetAncestor, maybe we can avoid duplication and have this assertion instead?

const btck_BlockTreeEntry* btck_block_tree_entry_get_ancestor(const btck_BlockTreeEntry* block_tree_entry, int height)
{
    const auto* ancestor{btck_BlockTreeEntry::get(block_tree_entry).GetAncestor(height)};
    assert(ancestor);
    return btck_BlockTreeEntry::ref(ancestor);
}

Agreed. Done.

This is the first time we are using the @pre tag in the documentation while there are other functions that might hit an assertion on out-of-bounds input. Not sure but maybe it is better to drop it to stay consistent with the rest of the documentation.

Done.

Since for an out-of-bounds height we would hit an assertion in btck_block_tree_entry_get_ancestor, we wouldn't have a nullptr entry to return nulloptfor; so we could simply return a BlockTreeEntry?

    BlockTreeEntry GetAncestor(int height) const
    {
        return BlockTreeEntry{btck_block_tree_entry_get_ancestor(get(), height)};
    }

Done.

I think the new tests belong to the existing test case btck_block_tree_entry_tests:

<details><summary>Diff</summary>

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 49c7f4a6e4..5399fef761 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -973,6 +973,11 @@ BOOST_AUTO_TEST_CASE(btck_block_tree_entry_tests)
     auto prev{entry_1.GetPrevious()};
     BOOST_CHECK(prev.has_value());
     BOOST_CHECK(prev.value() == entry_0);
+
+    // Test GetAncestor
+    BOOST_CHECK(entry_2.GetAncestor(2) == entry_2);
+    BOOST_CHECK(entry_2.GetAncestor(1) == entry_1);
+    BOOST_CHECK(entry_2.GetAncestor(0) == entry_0);
 }
 
 BOOST_AUTO_TEST_CASE(btck_chainman_in_memory_tests)
@@ -1178,33 +1183,3 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
     BOOST_CHECK_THROW(chainman->ReadBlockSpentOutputs(tip), std::runtime_error);
 }
 
-BOOST_AUTO_TEST_CASE(btck_block_tree_entry_ancestor_tests)
-{
-    auto test_directory{TestDirectory{"ancestor_tests"}};
-    auto notifications{std::make_shared<TestKernelNotifications>()};
-    auto context{create_context(notifications, ChainType::REGTEST)};
-    auto chainman{create_chainman(
-        test_directory, /*reindex=*/false, /*wipe_chainstate=*/false,
-        /*block_tree_db_in_memory=*/true, /*chainstate_db_in_memory=*/true,
-        context)};
-    for (const auto& raw_block : REGTEST_BLOCK_DATA) {
-        Block block{hex_string_to_byte_vec(raw_block)};
-        bool new_block{false};
-        BOOST_CHECK(chainman->ProcessBlock(block, &new_block));
-        BOOST_CHECK(new_block);
-    }
-
-    auto tip{chainman->GetBestEntry()};
-    int32_t tip_height{tip.GetHeight()};
-
-    // GetAncestor at the tip's own height returns the tip.
-    auto same{tip.GetAncestor(tip_height)};
-    BOOST_REQUIRE(same.has_value());
-    BOOST_CHECK(same.value() == tip);
-
-    // GetAncestor at height 0 returns the genesis block.
-    auto genesis{tip.GetAncestor(0)};
-    BOOST_REQUIRE(genesis.has_value());
-    BOOST_CHECK_EQUAL(genesis->GetHeight(), 0);
-
-}

</details>

Done, moved into btck_block_tree_entry_tests. Thanks!

 * [@brief](/bitcoin-bitcoin/contributor/brief/) Return the ancestor of a btck_BlockTreeEntry at the given height.

Done.

I noticed we use int32_t in the header but int in bitcoinkernel.cpp (introduced with #34885 (review)). That mismatch seems to be repeated in the existing API (e.g. btck_chain_get_height).

Looking at the history of (https://github.com/bitcoin/bitcoin/pull/30595/#discussion_r2404583482), the mismatch in btck_chain_get_height appears to be an incomplete fix.

Could all height-related functions (btck_chain_get_height, btck_chain_get_by_height, and this new function) not be aligned to int32_t?

Or is that deliberate? @sedited, @stringintech

Oh actually the int in my earlier comment was included unintentionally. I also think the header and the impl should match signatures and both use int32_t. AFAIK we use fixed-width types for data values like the heights (regardless whether the internal core code uses fixed-width types or plain int), and int/unsigned int for boolean-like values and flags.

I see. I think they should match too, but also saw btck_chain_get_height and wasn't sure if it was a purposeful design choice.

I agree. int32_t is unambiguous and self-documenting. I have opened a PR for the other functions. #35096

Let's see what the others say. Maybe we are missing something.

In the meantime, I would recommend aligning the types on this PR too @pzafonte.