--- Log opened Thu Feb 24 00:00:08 2022
05:31 < darosior> sipa: so i expected the CI crash on an integer overflow and i'm confused. What is this supposed to do https://github.com/sipa/miniscript/blob/578e1e3f82334f34dc0b3af36492f95ccebe1395/bitcoin/script/miniscript.h#L256? It seems like it's supposed to give a default value, but it doesn't
05:31 < darosior> s/expected/inspected/
05:33 <@sipa> darosior: Yeah, it should be a default value for any constructor that doesn't explicitly state another initializer for size.
05:34 < darosior> SetAvailable() is setting size to size_t::max, which is the reason for the crash
05:34 < darosior> Ok, thanks
05:35 < darosior> Although i thought you couldn't give default value to struct members. Maybe in C++ you can?
05:36 <@sipa> In C++ a struct and a class are exactly the same.
05:37 <@sipa> Apart from the fact that a struct is by default public members, and a class is by default private.
05:37 < darosior> So the reason for the crash is: for combination of fragments we add the (n)sat together. The nsat of a sub might be unavailable (and therefore its size is at numeric_limits::max()). In this case it overflows and triggers the UB sanitizer
05:38 <@sipa> In operator+() ?
05:38 < darosior> Yes
05:38 <@sipa> If it
05:38 < darosior> So we could either not set the size to the max, use a SaturatingAdd() in operator+(), or something else more invasive
05:38 < darosior> Ok
05:39 <@sipa> If it's unavailable, the size value is essentiaylly ignored, so this doesn't actually matter.
05:39 <@sipa> (unsigned integer overflow is not actually UB)
05:40 <@sipa> though i guess it makes sense to just cause operator+() to set size to max whenever one of the inputs is unavailable
05:40 < darosior> I just conditioned the addition on whether both inputs are available
05:41 < darosior> It one is unavailable it will set it to max() anyways afterward by calling SetAvailable
05:42 <@sipa> makes sense
06:50 <@sipa> darosior: I'll try to open PRs for the remaining improvements to the tests I have today.
06:50 < darosior> ok
07:11 -!- roconnor [~roconnor@coq/roconnor] has joined ##miniscript
08:39 <@sipa> darosior: Needing to mock the equality check is kind of invasive I find (it's consensus code after all), and I wonder if there isn't a way to avoid it.
08:41 <@sipa> I'd think that we can add an IsSolvable() to Miniscript (or in a function around it) instead, and for Miniscript-compatible scripts, IsSolvable() could bypass calling the solver entirely.
08:41 <@sipa> Or even just skip the validity test.
08:43 <@sipa> Because Miniscript's Satisfier knows when it has produced a valid satisfaction (according the provided context), if it's non-malleable (which is all that we're exposing I think), and we do extensive fuzz testing to make sure that this assessment matches consensus/standardness rules.
08:46 < darosior> Yeah i hate it too, if we can avoid it that's good. But iirc i tried a few things and it was the least ugly version i could come up with. Also the diff is isolated and tiny
08:47 < darosior> "and we do extensive fuzz testing to make sure that this assessment matches consensus/standardness rules." -> Yeah and even then it's only for check solvability. For actual transactions we still have the verification
08:48 <@sipa> Right, the fact that in IsSolvable() we not only try to produce a satisfaction (with a dummy context) and then in addition try to verify that that satisfaction is valid is very much overkill.
08:49 < darosior> If we decide to go this route i think i can just PR that separately, then when/if it's merged rebase the signing PR to drop the commit touching consensus code
08:49 <@sipa> I'll have a look.
--- Log closed Fri Feb 25 00:00:09 2022