miniscript_string: Core accepts `+` sign for numbers

[brunoerg]

We got a crash on miniscript_string target due to validation of numbers. For older(), after(), thresh() and maybe other fragments, Bitcoin Core accept the usage of the + sign, e.g. (l:older(+1), u:after(+1)), because of ParseInt64 function. However, rust-miniscript checks the character itself, so it only accepts "1", "2", "3"..."9".

if !('1'..='9').contains(&ch) {
    return Err(Error::Unexpected("Number must start with a digit 1-9".to_string()));
}

There is a test to ensure it doesn't accept those ones:

fn test_parse_num() {
    assert!(parse_num("0").is_ok());
    assert!(parse_num("00").is_err());
    assert!(parse_num("0000").is_err());
    assert!(parse_num("06").is_err());
    assert!(parse_num("+6").is_err());
    assert!(parse_num("-6").is_err());
}

[maflcko]

ParseInt64

There is ToIntegral in src/util/strencodings.h, which should be used for (almost all) new code:

/**
 * Convert string to integral type T. Leading whitespace, a leading +, or any
 * trailing character fail the parsing. The required format expressed as regex
 * is `-?[0-9]+`. The minus sign is only permitted for signed integer types.
 *
 * @returns std::nullopt if the entire string could not be parsed, or if the
 *   parsed value is not in the range representable by the type T.
 */
template <typename T>
std::optional<T> ToIntegral(std::string_view str)

[brunoerg]

@maflcko thanks. ToIntegral sounds better, I think we still would have some discrepancy because ToIntegral accepts, for example, 000001, while rust-miniscript would not, but it's ok

[brunoerg]

https://github.com/bitcoin/bitcoin/pull/30577

[apoelstra]

I wouldn't mind updating rust-miniscript. But we should probably update the Miniscript BIP with the exact number format.

[brunoerg]

Fixed https://github.com/bitcoin/bitcoin/pull/30577