bitcoin-tx: Stricter check for valid integers #13603

Just calling atoi to convert strings to integers does not check for valid integers very thoroughly; in particular, it just ignores everything starting from the first non-numeral character. Even a string like "foo" is fine and silently returns 0.

This meant that bitcoin-tx would not fail if such a string was passed in various places where an integer is expected (like the locktime or an input/output index); this means that it would, for instance, silently accept a typo and interpret it in an unexpected way.

In this change, we use ParseInt64 for parsing strings to integers, which actually verifies that the full string is valid as number. New tests in the bitcoin-util-test cover the new error paths.

This fixes #13599.

Just use ParseInt64 ?

Makes sense, wasn't aware that ParseInt64 exists. I still keep that ParseInteger function, though, because it handles the error check. Otherwise all places where it is used would be more verbose and the code would be less readable (IMHO) - but if you prefer to not have this function at all, I can also do that.

In most cases here, the error check tends to duplicate another error check at the callsite.

It is also a bit disappointing that the error message returned to the user regresses from the specific ("invalid TX version") to less specific ("invalid integer").

Agree using ParseInt64 directly to retain the current error message would be nicer, particularly if all errors included the invalid string, rather than just half of them.

It is a good point that the callers have already existing (although different) error checks - with this in mind, I agree that using ParseInt64 directly is good. I've updated the patch accordingly. Now all error messages are specific as to which value it is and what is the exact invalid string - this improves also some existing error messages that previously did not show the invalid value.

LGTM - original-author ACK, though more active devs may have other comments

I'll note that the temporary variables are now only conditionally initialized, not unconditionally initialized. Should be OK as we unconditionally throw in the uninitialized-var case, but it's something to double-check.

utACK 57889e688dd0987a1e087cd48d216a413127601e

utACK 57889e688dd0987a1e087cd48d216a413127601e The conditional initialization looks OK to me, it will never pass the point of initialization in the control flow in case of leaving the value uninitialized.