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

1. 
   hodlinator commented at 7:56 am on April 12, 2025: contributor

   First instances discovered by darosior in #31727 (comment).
2. miniscript: Correct off-by-one assert guards

   Co-authored-by: Antoine Poinsot <darosior@protonmail.com>

   3693e4d6ee
3. 
   DrahtBot commented at 7:56 am on April 12, 2025: contributor

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

   Code Coverage & Benchmarks

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, sipa, l0rinc

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

   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.

4. DrahtBot added the label Descriptors on Apr 12, 2025
5. 
   maflcko commented at 8:07 am on April 12, 2025: member

   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).

6. 
   hodlinator commented at 8:12 am on April 12, 2025: contributor

   Agree that since asserts are always enabled in this project, using .at(n) is fairly equivalent. Might switch if I re-touch.
7. 
   sipa commented at 1:48 pm on April 12, 2025: member

   ACK 3693e4d6ee003623bebb6c4c2c1672ed4c6e7a4a
8. 
   l0rinc commented at 4:16 pm on April 13, 2025: contributor

   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.

    0diff --git a/src/script/miniscript.h b/src/script/miniscript.h
    1index 3256ef967d..b20a1d3a95 100644
    2--- a/src/script/miniscript.h
    3+++ b/src/script/miniscript.h
    4@@ -1009,8 +1009,7 @@ private:
    5                     next_sats.push_back(sats[sats.size() - 1] + sub->ops.sat);
    6                     sats = std::move(next_sats);
    7                 }
    8-                assert(k < sats.size());
    9-                return {count, sats[k], sats[0]};
   10+                return {count, sats[k], sats.front()};
   11             }
   12         }
   13         assert(false);
   14@@ -1177,8 +1176,7 @@ private:
   15                     next_sats.push_back(sats[sats.size() - 1] + sub->ws.sat);
   16                     sats = std::move(next_sats);
   17                 }
   18-                assert(k < sats.size());
   19-                return {sats[k], sats[0]};
   20+                return {sats[k], sats.front()};
   21             }
   22         }
   23         assert(false);
   24@@ -1227,8 +1225,7 @@ private:
   25                     // satisfying 0 keys.
   26                     auto& nsat{sats[0]};
   27                     CHECK_NONFATAL(node.k != 0);
   28-                    assert(node.k < sats.size());
   29-                    return {std::move(nsat), std::move(sats[node.k])};
   30+                    return {std::move(nsat), std::move(sats.at(node.k))};
   31                 }
   32                 case Fragment::MULTI: {
   33                     // sats[j] represents the best stack containing j valid signatures (out of the first i keys).
   34@@ -1253,8 +1250,7 @@ private:
   35                     // The dissatisfaction consists of k+1 stack elements all equal to 0.
   36                     InputStack nsat = ZERO;
   37                     for (size_t i = 0; i < node.k; ++i) nsat = std::move(nsat) + ZERO;
   38-                    assert(node.k < sats.size());
   39-                    return {std::move(nsat), std::move(sats[node.k])};
   40+                    return {std::move(nsat), std::move(sats.at(node.k))};
   41                 }
   42                 case Fragment::THRESH: {
   43                     // sats[k] represents the best stack that satisfies k out of the *last* i subexpressions.
   44@@ -1288,8 +1284,7 @@ private:
   45                         // Include all dissatisfactions (even these non-canonical ones) in nsat.
   46                         if (i != node.k) nsat = std::move(nsat) | std::move(sats[i]);
   47                     }
   48-                    assert(node.k < sats.size());
   49-                    return {std::move(nsat), std::move(sats[node.k])};
   50+                    return {std::move(nsat), std::move(sats.at(node.k))};
   51                 }
   52                 case Fragment::OLDER: {
   53                     return {INVALID, ctx.CheckOlder(node.k) ? EMPTY : INVALID};
   

9. fanquake merged this on Apr 14, 2025
10. fanquake closed this on Apr 14, 2025

    ---

11. 
    maflcko commented at 11:12 am on April 14, 2025: member

    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

Contributors
hodlinator DrahtBot maflcko sipa l0rinc

Labels
Descriptors