This is a follow-up to #17954 which was merged yesterday.
See post-merge comments in #17954 for context:
Oh, I didn't see this during review. I read the method as nothing is filled when the ancestor is not found. I.e. I read the
&as&&. Is there any caller that depends on this edge case?
I don't think this is a good change. I would prefer to just add a test making sure the API works as designed instead of making the API leave behind uninitialized output variables and be awkward to use and then document the awkwardness. Here is the test I would add:
--- a/src/test/interfaces_tests.cpp
+++ b/src/test/interfaces_tests.cpp
@@ -116,6 +116,12 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
BOOST_CHECK_EQUAL(orig_height, orig_tip->nHeight);
BOOST_CHECK_EQUAL(fork_height, orig_tip->nHeight - 10);
BOOST_CHECK_EQUAL(fork_hash, active[fork_height]->GetBlockHash());
+
+ uint256 active_hash, orig_hash;
+ BOOST_CHECK(!chain->findCommonAncestor(active.Tip()->GetBlockHash(), {}, {}, FoundBlock().hash(active_hash), {}));
+ BOOST_CHECK(!chain->findCommonAncestor({}, orig_tip->GetBlockHash(), {}, {}, FoundBlock().hash(orig_hash)));
+ BOOST_CHECK_EQUAL(active_hash, active.Tip()->GetBlockHash());
+ BOOST_CHECK_EQUAL(orig_hash, orig_tip->GetBlockHash());
}
BOOST_AUTO_TEST_CASE(hasBlocks)
The PR description here is also not justifying the change, only linking to a long discussion thread. The only apparent reason for this change is is that it makes the code "AutoSAR" and "MISRA" complaint. I haven't heard of these things and didn't know my PRs were supposed to comply with them. If they are, I'd like to see automated compliance tests or notes in the developer guide with testing instructions.
Code review ACK f8bfda16ebefda40efd66eeb0225c353a88d8e53, in case other reviewers think this PR is an improvement and want to merge it. Despite the fact that this PR may leave FoundBlock output variables uninitialized, I don't think it causes any bugs
@ryanofsky The justification is to minimise surprising code: apparently both @MarcoFalke and I were somewhat surprised by the current formulation. Perhaps the intentional use of & instead of && was clear to other reviewers.
No one is claiming that we are following AutoSAR, or that you should have thought about AutoSAR when submitting your PR :)
@ryanofsky The justification is to minimise surprising code: apparently both @MarcoFalke and I were somewhat surprised by the current formulation
Thanks for finding this and pointing it out. It's a good catch that uncovered a gap in test coverage and point of potential confusion. I didn't like this particular solution to the problem, but I posted another one in https://github.com/bitcoin/bitcoin/pull/18660