miniscript: Correct off-by-one assert guards (#31727 follow-up) #32255

First instances discovered by darosior in #31727 (comment).

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #31719 (miniscript: fixes #29098 by only use first k valid signatures by tnndbtc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

lgtm ACK 3693e4d6ee003623bebb6c4c2c1672ed4c6e7a4a

This reminds me of #32122 and seems like a common error. An alternative may be to remove the verbose and erroneous asserts and just use std::(linear_container)::at(size_t) instead (downside would be on error the exception string is less descriptive), or enable the fast standard library hardening by default (probably a longer task, starting by opening a brainstorming issue for this first. Also has the same downside of slightly less descriptive error message compared to assert/Assert/CHECK_NONFATAL).

Agree that since asserts are always enabled in this project, using .at(n) is fairly equivalent. Might switch if I re-touch.

ACK 3693e4d6ee003623bebb6c4c2c1672ed4c6e7a4a

Tested ACK 3693e4d6ee003623bebb6c4c2c1672ed4c6e7a4a Checked that each modified line is covered by tests.

I also would prefer having .at() and .front() instead of the extra asserts to document it better - but I'm also fine with this.

<details> <summary>Suggestion</summary>

diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 3256ef967d..b20a1d3a95 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -1009,8 +1009,7 @@ private:
                     next_sats.push_back(sats[sats.size() - 1] + sub->ops.sat);
                     sats = std::move(next_sats);
                 }
-                assert(k < sats.size());
-                return {count, sats[k], sats[0]};
+                return {count, sats[k], sats.front()};
             }
         }
         assert(false);
@@ -1177,8 +1176,7 @@ private:
                     next_sats.push_back(sats[sats.size() - 1] + sub->ws.sat);
                     sats = std::move(next_sats);
                 }
-                assert(k < sats.size());
-                return {sats[k], sats[0]};
+                return {sats[k], sats.front()};
             }
         }
         assert(false);
@@ -1227,8 +1225,7 @@ private:
                     // satisfying 0 keys.
                     auto& nsat{sats[0]};
                     CHECK_NONFATAL(node.k != 0);
-                    assert(node.k < sats.size());
-                    return {std::move(nsat), std::move(sats[node.k])};
+                    return {std::move(nsat), std::move(sats.at(node.k))};
                 }
                 case Fragment::MULTI: {
                     // sats[j] represents the best stack containing j valid signatures (out of the first i keys).
@@ -1253,8 +1250,7 @@ private:
                     // The dissatisfaction consists of k+1 stack elements all equal to 0.
                     InputStack nsat = ZERO;
                     for (size_t i = 0; i < node.k; ++i) nsat = std::move(nsat) + ZERO;
-                    assert(node.k < sats.size());
-                    return {std::move(nsat), std::move(sats[node.k])};
+                    return {std::move(nsat), std::move(sats.at(node.k))};
                 }
                 case Fragment::THRESH: {
                     // sats[k] represents the best stack that satisfies k out of the *last* i subexpressions.
@@ -1288,8 +1284,7 @@ private:
                         // Include all dissatisfactions (even these non-canonical ones) in nsat.
                         if (i != node.k) nsat = std::move(nsat) | std::move(sats[i]);
                     }
-                    assert(node.k < sats.size());
-                    return {std::move(nsat), std::move(sats[node.k])};
+                    return {std::move(nsat), std::move(sats.at(node.k))};
                 }
                 case Fragment::OLDER: {
                     return {INVALID, ctx.CheckOlder(node.k) ? EMPTY : INVALID};

</details>

I also would prefer having .at() and .front() instead of the extra asserts to document it better - but I'm also fine with this.

I don't think front() is safe in C++20. See https://en.cppreference.com/w/cpp/container/span/front. This requires https://github.com/bitcoin/bitcoin/issues/32265