FWIW, here's an alternative shorter version for the same logic, without branches:

    n = std::min(n, static_cast<int64_t>(std::numeric_limits<int32_t>::max()));
    n = std::max(n, static_cast<int64_t>(std::numeric_limits<int32_t>::min()));

Not sure if it's really more readable though, personally I'd be okay with both versions.

std::clamp in C++17 would be even better and less verbose, but unsure if we can use C++17 features ?

Note that min/max in the standard also do exactly one comparison each.

I can remove the if/else though if people think it fits better here.

Ah, std::clamp would indeed be a very nice option. Unfortunately, upgrading to C++17 has not happened yet (see this discussion: #16684), so it can't be used.
Sometimes substitutes for not-yet-available C++ features are implemented by contributors, like Span for the C++20 std::span by sipa (see #12886), don't know though if that is a desired way to go here, and if yes, if it should be packed in this PR. This should probably include identifiying all clamps throughout the codebase and using our newly introduced clamp function (so it can be replaced with std::clamp once we move to C++17).

Thanks, I'd be down to do that in a different PR. I'm looking for stuffs to do to improve my knowledge of the codebase anyway :)

I'll wait for some other people to confirm "yes this is a good idea, do a separate PR for std::clamp" before starting though

Shouldn't this version be kept as an example of an _invalid script?

OK, thanks. This is a good idea I think, I was hesitant to add more tests. If I understood you correctly, the tests are invalids here, so I guess you mean adding something like:

["Argument 2^32 in decimal (hence clamped to 0xffffffff) with nLockTime=2^32-2"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 CHECKLOCKTIMEVERIFY 1"]],
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000fffffffe", "P2SH,CHECKLOCKTIMEVERIFY"],

If this is correct, I guess I should add similar tests for other modified test cases too ?

Feel free to add as many tests as you like, as long as all existing proper test cases also stay there

OK great, I added 5 more tests in the branch.

What I mean is that this shouldn't be silently clamped. The test here looks right. Why would it be safe to parse a script and silently change it while parsing?

std::runtime_error is thrown on other script parse errors in ParseScript(…). Shouldn't that be thrown also for this type of script parse error in ParseScript?

OK, thanks, this is clearly a possibility.

Note that for now, no test trigger the already existing std::runtime_error. script_json_test doesn't handle exceptions, so it should be modified to catch them (maybe in a different PR then ?). Testcases will have to be modified to handle the exception too I guess.

(the fuzzer harness already handle exceptions well)

I'm not sure which possibility (clamping or throwing an error) fits more the style of the codebase. Maybe getting an ACK from @sipa for the exception first ?

@practicalswift hm sorry but I guess it's better to ask first: if you return an std::runtime_error, how to describe the error in the JSON test file ?

I guess that adding a new value to the ScriptError_t enum is too intrusive.
Adding a special if/else case in script_test.cpp:ParseScriptError seems too hackish.
Reusing an existing ScriptError_t seems the less odd option - something like SCRIPT_ERR_BAD_OPCODE or SCRIPT_ERR_OP_COUNT ?

I don't think there is a need. (tx/script)_(valid/invalid) are testing the transaction and script validity logic - not the script parser.

If a particular test now causes an exception to be thrown, just write that test differently. It's still testing the same thing.

OK, so as stated below, if a test contains 4294967296 CHECKLOCKTIMEVERIFY 1 or 999...999, this is a script parser error which should be covered in bitcoin-util-test.json, not here. I believe this resolve this issue ?

Note that using an exception allowed to raise new errors in tx_valid - before the clamped value also valided the test...

Any reason this needs a static cast?

The second static_cast seems strictly necessary, see this example : https://godbolt.org/z/Ko33NG

The first one is superficial (implicit), I just thought that the symetry looked better. I can remove the first one if you prefer ?

edit : at least fixed whitespace style

edit 2 : also note that the LL suffix could be used but this would assume that long long = 64bits

Explicit is better than implicit :)

I think with C++11 you can also get a safer way to achieve the same: int64_t{bla}. If the value doesn't fit in the integer type, it won't compile. Whereas with static_cast it would compile and give the wrong result.

Thanks, updated the branch accordingly,

Is <limits> still necessary?

Ow, thank you. No it isn't, just removed it.

Why are you changing all the values when translating from dec to hex? It might be fine, but at least for locktime values there are different code paths depending on the value.

Ow, good point. I double checked with the value returned by ParseScript in the tests before/after the PR - so that the behavior of the tests doesn't change at all (i.e. before this PR the script built by ParseScript from 4294967296 contained 0x050000000001)

Ah, oops. My bad