Merge #16570: tests: Make descriptor tests deterministic

b9ee63c71b Make descriptor test deterministic (David Reikher)

Pull request description:

  This is an improvement to a test, inspired by #14343 - removing non determinism from a test.

  The test `descriptor_test` is non-deterministic, as it relies on the `MaybeUseHInsteadOfApostrophy` function which randomly either swaps all apostrophes with 'h' or doesn't at all in a descriptor. This fix makes both cases always run, if an apostrophe is found in a test descriptor.
  This does not reduce test coverage but removes the non-determinism.

  Additionally, the `MaybeUseHInsteadOfApostrophy` function removed the checksum if found at the end of a descriptor when the apostrophes are swapped by 'h's, since after being swapped the checksum is no longer correct. I instead added re-calculation of the checksum using the `DescriptorChecksum` function, which adds coverage for the case of a descriptors having 'h's instead of apostrophes and a checksum. This was previously lacking.
  To achieve this I had to move `DescriptorChecksum` and `PolyMod` out of the anonymous namespace in descriptor.cpp to make `DescriptorChecksum` accessible in descriptor_tests.cpp.

  All tests complete successfully (functional as well as unit tests).

ACKs for top commit:
  achow101:
    Code Review ACK b9ee63c71b

Tree-SHA512: 992c73a6644a07bfe7c72301ee2666f3c4845a012aaedd7a099a05cea8bdac84fa8280b28e44a7856260c00c0be1a6f1b6768f5694c2a22edf4c489e53fec424

src/test/descriptor_tests.cpp

@@ -42,33 +42,47 @@ bool EqualDescriptor(std::string a, std::string b)
     return a == b;
 }
 
-std::string MaybeUseHInsteadOfApostrophy(std::string ret)
+std::string UseHInsteadOfApostrophe(const std::string& desc)
 {
-    if (InsecureRandBool()) {
-        while (true) {
-            auto it = ret.find("'");
-            if (it != std::string::npos) {
-                ret[it] = 'h';
-                if (ret.size() > 9 && ret[ret.size() - 9] == '#') ret = ret.substr(0, ret.size() - 9); // Changing apostrophe to h breaks the checksum
-            } else {
-                break;
-            }
-        }
+    std::string ret = desc;
+    while (true) {
+        auto it = ret.find('\'');
+        if (it == std::string::npos) break;
+        ret[it] = 'h';
+    }
+
+    // GetDescriptorChecksum returns "" if the checksum exists but is bad.
+    // Switching apostrophes with 'h' breaks the checksum if it exists - recalculate it and replace the broken one.
+    if (GetDescriptorChecksum(ret) == "") {
+        ret = ret.substr(0, desc.size() - 9);
+        ret += std::string("#") + GetDescriptorChecksum(ret);
     }
     return ret;
 }
 
 const std::set<std::vector<uint32_t>> ONLY_EMPTY{{}};
 
-void Check(const std::string& prv, const std::string& pub, int flags, const std::vector<std::vector<std::string>>& scripts, const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY)
+void DoCheck(const std::string& prv, const std::string& pub, int flags, const std::vector<std::vector<std::string>>& scripts, const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY,
+    bool replace_apostrophe_with_h_in_prv=false, bool replace_apostrophe_with_h_in_pub=false)
 {
     FlatSigningProvider keys_priv, keys_pub;
     std::set<std::vector<uint32_t>> left_paths = paths;
     std::string error;
 
+    std::unique_ptr<Descriptor> parse_priv;
+    std::unique_ptr<Descriptor> parse_pub;
     // Check that parsing succeeds.
-    auto parse_priv = Parse(MaybeUseHInsteadOfApostrophy(prv), keys_priv, error);
-    auto parse_pub = Parse(MaybeUseHInsteadOfApostrophy(pub), keys_pub, error);
+    if (replace_apostrophe_with_h_in_prv) {
+        parse_priv = Parse(UseHInsteadOfApostrophe(prv), keys_priv, error);
+    } else {
+        parse_priv = Parse(prv, keys_priv, error);
+    }
+    if (replace_apostrophe_with_h_in_pub) {
+        parse_pub = Parse(UseHInsteadOfApostrophe(pub), keys_pub, error);
+    } else {
+        parse_pub = Parse(pub, keys_pub, error);
+    }
+
     BOOST_CHECK(parse_priv);
     BOOST_CHECK(parse_pub);
 
@@ -167,6 +181,32 @@ void Check(const std::string& prv, const std::string& pub, int flags, const std:
     BOOST_CHECK_MESSAGE(left_paths.empty(), "Not all expected key paths found: " + prv);
 }
 
+void Check(const std::string& prv, const std::string& pub, int flags, const std::vector<std::vector<std::string>>& scripts, const std::set<std::vector<uint32_t>>& paths = ONLY_EMPTY)
+{
+    bool found_apostrophes_in_prv = false;
+    bool found_apostrophes_in_pub = false;
+
+    // Do not replace apostrophes with 'h' in prv and pub
+    DoCheck(prv, pub, flags, scripts, paths);
+
+    // Replace apostrophes with 'h' in prv but not in pub, if apostrophes are found in prv
+    if (prv.find('\'') != std::string::npos) {
+        found_apostrophes_in_prv = true;
+        DoCheck(prv, pub, flags, scripts, paths, /* replace_apostrophe_with_h_in_prv = */true, /*replace_apostrophe_with_h_in_pub = */false);
+    }
+
+    // Replace apostrophes with 'h' in pub but not in prv, if apostrophes are found in pub
+    if (pub.find('\'') != std::string::npos) {
+        found_apostrophes_in_pub = true;
+        DoCheck(prv, pub, flags, scripts, paths, /* replace_apostrophe_with_h_in_prv = */false, /*replace_apostrophe_with_h_in_pub = */true);
+    }
+
+    // Replace apostrophes with 'h' both in prv and in pub, if apostrophes are found in both
+    if (found_apostrophes_in_prv && found_apostrophes_in_pub) {
+        DoCheck(prv, pub, flags, scripts, paths, /* replace_apostrophe_with_h_in_prv = */true, /*replace_apostrophe_with_h_in_pub = */true);
+    }
+}
+
 }
 
 BOOST_FIXTURE_TEST_SUITE(descriptor_tests, BasicTestingSetup)