test: Verify findCommonAncestor always initializes outputs #18660

Also add code comment to clarify surprising code noted by practicalswift #18657 (comment)

ACK 9986608ba93de040490ee0d5584ea33e605f1df0

Concept ACK

Thanks for addressing this.

ACK on the addition of the test, but for the chain.cpp change I suggest using the following equivalent formulation which removes the need for the clarifying comment (and as an added bonus avoids static analyser warnings for static analysers using the AutoSAR ruleset :)):

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index c8311b298..ed2142884 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -275,7 +275,10 @@ public:
         const CBlockIndex* block1 = LookupBlockIndex(block_hash1);
         const CBlockIndex* block2 = LookupBlockIndex(block_hash2);
         const CBlockIndex* ancestor = block1 && block2 ? LastCommonAncestor(block1, block2) : nullptr;
-        return FillBlock(ancestor, ancestor_out, lock) & FillBlock(block1, block1_out, lock) & FillBlock(block2, block2_out, lock);
+        const bool ancestor_filled = FillBlock(ancestor, ancestor_out, lock);
+        const bool block1_filled = FillBlock(block1, block1_out, lock);
+        const bool block2_filled = FillBlock(block2, block2_out, lock);
+        return ancestor_filled && block1_filled && block2_filled;
     }
     void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(m_node, coins); }
     double guessVerificationProgress(const uint256& block_hash) override

ACK 9986608ba93de04 modulo @practicalswift's #18660 (comment)

I suggest using the following equivalent formulation which removes the need for the clarifying comment

Readability is subjective, but to me that suggestion is less readable, more verbose, and less clear that the evaluations are intentional not accidental. Comments are useful when they convey intent.

This is adding documentation and tests. Should be good to merge with two ACKs. If the AutoSAR static analyser can't read the comment that the & are desired, it should probably be fixed upstream.

ACK 9986608ba93de040490ee0d5584ea33e605f1df0.

In both could have checked ancestor_out hash is null.