Avoid termination due to unhandled exception when parsing invalid arguments to after(...), older(...), thresh(...) and thresh_m(...)

[practicalswift]

Hi sipa!

Thanks for your excellent work on miniscript and this C++ implementation!

This is really important work so I decided to put some time into breaking it :-)

That has proven quite hard: the implementation seems to be very robust and well-written. I really like the modern C++ style.

Anyways, this is a small issue I found:

Before:

$ ./miniscript <<< "after(1)"
      0 scriptlen=2 maxops=1 type=B safe=no nonmal=yes dissat=no input=0 output=nonzero miniscript=after(1)
$ ./miniscript <<< "after(100000000000000000000)"
terminate called after throwing an instance of 'std::out_of_range'
  what():  stoul
Aborted
$ ./miniscript <<< "after()"
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoul
Aborted
$ for FUNC in after older thresh thresh_m; do
    for N in "" "100000000000000000000"; do
      MINISCRIPT="${FUNC}(${N})"
      echo "Testing miniscript: ${MINISCRIPT}"
      ./miniscript <<< "${MINISCRIPT}"
      echo
    done
  done
Testing miniscript: after()
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoul
Aborted

Testing miniscript: after(100000000000000000000)
terminate called after throwing an instance of 'std::out_of_range'
  what():  stoul
Aborted

Testing miniscript: older()
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoul
Aborted

Testing miniscript: older(100000000000000000000)
terminate called after throwing an instance of 'std::out_of_range'
  what():  stoul
Aborted

Testing miniscript: thresh()
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoul
Aborted

Testing miniscript: thresh(100000000000000000000)
terminate called after throwing an instance of 'std::out_of_range'
  what():  stoul
Aborted

Testing miniscript: thresh_m()
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoul
Aborted

Testing miniscript: thresh_m(100000000000000000000)
terminate called after throwing an instance of 'std::out_of_range'
  what():  stoul
Aborted

After:

$ ./miniscript <<< "after(1)"
      0 scriptlen=2 maxops=1 type=B safe=no nonmal=yes dissat=no input=0 output=nonzero miniscript=after(1)
$ ./miniscript <<< "after(100000000000000000000)"
Failed to parse as policy or miniscript 'after(100000000000000000000)'
$ ./miniscript <<< "after()"
Failed to parse as policy or miniscript 'after()'
$ for FUNC in after older thresh thresh_m; do
    for N in "" "100000000000000000000"; do
      MINISCRIPT="${FUNC}(${N})"
      echo "Testing miniscript: ${MINISCRIPT}"
      ./miniscript <<< "${MINISCRIPT}"
      echo
    done
  done
Testing miniscript: after()
Failed to parse as policy or miniscript 'after()'

Testing miniscript: after(100000000000000000000)
Failed to parse as policy or miniscript 'after(100000000000000000000)'

Testing miniscript: older()
Failed to parse as policy or miniscript 'older()'

Testing miniscript: older(100000000000000000000)
Failed to parse as policy or miniscript 'older(100000000000000000000)'

Testing miniscript: thresh()
Failed to parse as policy or miniscript 'thresh()'

Testing miniscript: thresh(100000000000000000000)
Failed to parse as policy or miniscript 'thresh(100000000000000000000)'

Testing miniscript: thresh_m()
Failed to parse as policy or miniscript 'thresh_m()'

Testing miniscript: thresh_m(100000000000000000000)
Failed to parse as policy or miniscript 'thresh_m(100000000000000000000)'

[sipa]

Nice find. It'd be better to abstract out the whole parse-a-number-and-catch-if-necessary logic rather than repeating it 4 times, though.

[practicalswift]

@sipa Good point! I thought about that but wasn't sure about your preference. I'll add a function :-)

[practicalswift]

Now using ParseUInt64 and ParseUInt32 instead.

When reviewing please note the change from unsigned long num to uint64_t num.

Looks OK?

[sipa]

@practicalswift Sorry, I forgot about this. I fixed this independently after seeing the Bitcoin Core linter complaining about the use of locale-dependent functions stoul etc. It's fixed in 2e9deb2.