validation: rename block script verification error from "mandatory" to "block"

darosior commented at 2:14 pm on August 13, 2025: member

This is a followup to #33050 now that it’s merged. Using “block”/“mempool” as the error reason is clearer to a user than “mandatory”/“non-mandatory”. The “non-mandatory” errors got renamed to “mempool” in #33050 (see #33050 (review)). This takes care of the second part of the renaming.

DrahtBot added the label Validation on Aug 13, 2025

DrahtBot commented at 2:14 pm on August 13, 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/33183.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, davidgumberg, ajtowns, janb84, Crypt-iQ, instagibbs

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

fanquake commented at 2:35 pm on August 13, 2025: member

Error: script block marker but no scripted-diff in title of commit 70cf8987fc5737f237be746c6426e155d11fd826

Changing the title of 70cf8987fc5737f237be746c6426e155d11fd826 to scripted-diff: validation: rename mandatory errors into block errors will fix running the scripted diff.

fanquake added this to the milestone 30.0 on Aug 13, 2025

scripted-diff: validation: rename mandatory errors into block errors

Using "block" or "mempool" as the prefix in place of "mandatory" or "non-mandatory" is clearer
to a user. "non-mandatory" was renamed into "mempool" as part of #33050. This takes care of the
other half of this renaming as a scripted diff.

-BEGIN VERIFY SCRIPT-
sed -i 's/mandatory-script-verify/block-script-verify/g' $(git grep -l mandatory-script-verify)
-END VERIFY SCRIPT-

9a04635432

contrib: adapt max reject string size in tracing demo

The Script errors were last touched in 2020. This value was calculated after that
in 2022 (commit 4b7aec2951fe4595946cdc804b0dec1921d79d05). The previous commit
made the size of the largest reject reason string 4 characters smaller ("mandatory"
became "block"), so adapt the constant.

b3f781a0ef

darosior force-pushed on Aug 13, 2025

darosior commented at 3:06 pm on August 13, 2025: member

Done, thanks.

DrahtBot added the label CI failed on Aug 13, 2025

DrahtBot commented at 3:06 pm on August 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48005962432 LLM reason (✨ experimental): The CI failure was caused by a missing scripted-diff marker in the commit message.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Aug 13, 2025

fanquake requested review from ajtowns on Aug 14, 2025

fanquake requested review from mzumsande on Aug 14, 2025

willcl-ark commented at 1:41 pm on August 14, 2025: member

Do you think this warrants a release note?

Previously changes similar to this have caused downstream breakages/compatibility issues ( #30212 (comment) ) when projects have been string matching on errors, but I don’t think that would be the case with this error?

darosior commented at 1:53 pm on August 14, 2025: member

Better safe than sorry. I can add a release note.

darosior referenced this in commit 2c6afb0ccb on Aug 14, 2025

darosior commented at 6:09 pm on August 14, 2025: member

For what it’s worth this breakage was necessary. The code does not differentiate between failures on consensus vs standardness script flags anymore, only between mempool / block verification context.

Add release note for #33050 and #33183 error string changes c0d91fc69c

darosior force-pushed on Aug 14, 2025

darosior commented at 6:11 pm on August 14, 2025: member

Added a release note.

DrahtBot added the label CI failed on Aug 14, 2025

DrahtBot commented at 6:11 pm on August 14, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/48114804714 LLM reason (✨ experimental): The CI failure is caused by a lint error related to release note snippet placement.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Aug 14, 2025

fjahr commented at 10:33 pm on August 14, 2025: contributor

utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b

The renaming makes sense, I have gotten used to the old name over time but I still remember that I found it a bit odd when I started working the script verification stuff.

davidgumberg commented at 10:46 pm on August 14, 2025: contributor

lgtm ACK https://github.com/bitcoin/bitcoin/commit/c0d91fc69c67e6f7123326d4f3caeac069d2637b

ajtowns commented at 0:02 am on August 15, 2025: contributor

I don’t think this should be problematic for lnd; the old non-mandatory-script-verify-flag error should map to ErrNonMandatoryScriptVerifyFlag which seems otherwise unused in btcsuite and lnd, and the mandatory-script-verify-flag-failed error should likewise map to ErrScriptVerifyFlag which also seems otherwise in used in btcsuite and lnd.

utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b

fanquake commented at 8:16 am on August 15, 2025: member

Looks like some renaming is already happening downstream: https://github.com/btcsuite/btcd/pull/2405.

janb84 commented at 8:36 am on August 15, 2025: contributor

utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b

PR brings error strings inline with the changes of #33050, changes mandatory to block. The change has some downstream implications but there are already downstream implications by #33050 and there are mitigating actions in the form of a release note. I’m more in favour of having downstream implications at once than over several releases and I think the mitigation actions are sufficient.

code review ✅

in doc/release-notes-33183.md:4 in c0d91fc69c

0@@ -0,0 +1,7 @@
1+Updated RPCs
2+------------
3+
4+Transaction Script validation errors used to return the reason for the error prefixed by either

instagibbs commented at 10:21 am on August 15, 2025:

0When submitting transactions via RPC commands such as `sendrawtransaction` and `submitpackage`, script validation errors were reported with the reason for the error prefixed by either

in contrib/tracing/mempool_monitor.py:22 in b3f781a0ef outdated

16@@ -17,9 +17,9 @@
17 PROGRAM = """
18 # include <uapi/linux/ptrace.h>
19 
20-// The longest rejection reason is 118 chars and is generated in case of SCRIPT_ERR_EVAL_FALSE by
21+// The longest rejection reason is 114 chars and is generated in case of SCRIPT_ERR_EVAL_FALSE by
22 // strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))
23-#define MAX_REJECT_REASON_LENGTH        118
24+#define MAX_REJECT_REASON_LENGTH        114

instagibbs commented at 10:52 am on August 15, 2025:

eyeballed this new value and that the comment is correct, seems right

Crypt-iQ commented at 10:58 am on August 15, 2025: contributor

utACK c0d91fc69c67e6f7123326d4f3caeac069d2637b

I don’t think this should be problematic for lnd

Yup, my mistake. I thought undefined errors would get mapped to a different one and used, but seems like that’s not the case.

instagibbs approved

instagibbs commented at 10:59 am on August 15, 2025: member

ACK c0d91fc69c67e6f7123326d4f3caeac069d2637b

nits on release notes only

fanquake merged this on Aug 15, 2025

fanquake closed this on Aug 15, 2025

validation: rename block script verification error from “mandatory” to “block” #33183

Code Coverage & Benchmarks

Reviews