script: Remove dead code from OP_CHECKMULTISIG#33977
script: Remove dead code from OP_CHECKMULTISIG#33977vostrnad wants to merge 1 commit intobitcoin:masterfrom
Conversation
Removes a redundant stack size check from OP_CHECKMULTISIG that was added in bitcoin#3843 (commit 6380180).
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33977. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
|
Code review ACK 53ca0dc The lines are safe to remove since they're side-effect free and the mentioned extra element is included in the initial stack size validation while the cleanup loop stops before popping it. |
|
|
|
The script interpreter code is so thoroughly tested, so infrequently refactored, and any change to it reviewed with such extreme caution that I don't think we need to bother with an assert, but I can add it if others think it's a good idea. |
|
tACK 53ca0dc Verified using existing tests and a new test that omits the dummy element: 8dd3db3 rebased on 53ca0dc. The previous stack size check already fails script evaluation when stack size < |
|
ACK 53ca0dc Verified the logic - after popping |
|
Concept -0 While this is probably true, the implementation of |
Adding this as a code comment just before the final stack-size check would make it easier to eyeball the code and see that the stack operations are safe: i += nSigsCount;
+++ // i is now 3 + nKeysCount + nSigsCount
if ((int)stack.size() < i) |
yashbhutwala
left a comment
There was a problem hiding this comment.
ACK 53ca0dc
Verified the logic by tracing through the code and ran script_tests which pass.
Verification of the invariant:
At line 1135, i = 3 + nKeysCount + nSigsCount, and the stack is validated to have >= i elements.
The cleanup loop while (i-- > 1) uses post-decrement, running for i values from (3 + nKeysCount + nSigsCount) down to 2 (inclusive), which pops exactly (i - 1) = (2 + nKeysCount + nSigsCount) elements.
Remaining: (3 + nKeysCount + nSigsCount) - (2 + nKeysCount + nSigsCount) = 1 element minimum.
The removed check can never be true.
Regarding @achow101's Concept -0: I understand the concern about reviewer time vs. impact. That said, dead code in consensus-critical paths is worth removing - it can mislead future readers into thinking there's a code path that can trigger it, and the missing coverage in the coverage reports is a minor recurring cost. The verification is a one-time cost that's now documented in this review thread.
Regarding @billymcbip's suggestion to add // i is now 3 + nKeysCount + nSigsCount before the final stack-size check: this could be a useful addition to make the invariant more immediately obvious to future readers, though I'd leave that to the author's discretion since the arithmetic is fairly straightforward when you trace through.
|
I'm Concept -0 on this too. Not sure if it is worthwhile to be cleaning up the script interpreter of these handful of cases not receiving coverage and thus revealing dead code. I also don't think this really improves readability, since it does document an invariant. |
|
Going to close this for now. |
The stack size check before the dummy element check in OP_CHECKMULTISIG is redundant. The stack is previously validated to have least i elements (where i is number of pubkeys + number of signatures + 3), and then i - 1 elements are popped off the stack, leaving at least one element still in.
The redundant check was added in #3843 (commit 6380180). There it's easy to see why it's redundant – it's inserted into a code path where another
popstackwould previously follow.Note the missing code coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L1200