Consistently bounds-check vin/vout access

[Empact]

An out of range transaction output is invalid / an error condition, so let's
blow up rather than skip the output or default.

I audited indexing into vin/vout looking for unchecked or quietly handled bounds errors and converted them to throw on out of bounds, because noisy failure is better than silent failure.

Disclaimer: I'm not that familiar with the role that out of bounds outputs and inputs might play in the network, apart from the SIGHASH_SINGLE case commented below which was established in the test. Please do educate me if I'm misguided on this. If there are more "valid but invalid" cases we should get them under test.

[sdaftuar]

I'd suggest dropping the third commit, which at first glance would seem to introduce a number of vulnerabilities. I agree though that it would be useful to add tests that would fail if that third commit were introduced.

[Empact]

@sdaftuar thanks, by vulnerabilities, I suppose you mean there is a risk of maliciously-crafted transactions bringing down the recipient node by triggering the throws, is that right? I hadn't been considering the changes through that lens, but I can take a second look with that in mind.

[Empact]

@sdaftuar I removed the third commit. Seems the remaining cases should be safe given they're already asserted against or otherwise assumed to be safe, such that the invalid values are not contended with at all.

[theuni]

Concept ACK on clarifying bounds, but I really don't like the use of exceptions replacing explicit prior checks. I understand that they accomplish the same thing, but IMO it is far less safe because it equates checks that are allowed to fail with those that logically never should. It also turns potential compile-time checks into guaranteed runtime ones.

Lastly, it really harms code clarity. This one for example, is a significant regression IMO.

[ryanofsky]

it equates checks that are allowed to fail with those that logically never should

According to http://en.cppreference.com/w/cpp/error/logic_error, this exception type "reports errors that are a consequence of faulty logic within the program such as violating logical preconditions or class invariants", so it doesn't seem switching from operator[] to at() in c++ should be taken to indicate any change in preconditions or intent.

But there may still be practical reasons to favor throwing exceptions explicitly or using assert() instead of relying on at(). For example, Matt thought it was important to assert instead of raising an exception here: #11640 (comment) to prevent errors from being caught in generic catch blocks.

[ryanofsky]

utACK 174de7e8fd4b4e52714029b6f6dd27c1ed5d080e. Change looks correct, but I think it would be safer to only add exceptions in places that currently trigger out of bounds memory accesses, not in places that currently trigger asserts, since assert behavior is probably preferable in most of these cases.

[sipa]

it doesn't seem switching from operator[] to at() in c++ should be taken to indicate any change in preconditions or intent.

But there may still be practical reasons to favor throwing exceptions explicitly or using assert() instead of relying on at().

This seems to only be a philosophical distinction to me. Yes, maybe std::logic_error isn't designed to caught by normal program code, and can thus be treated similarly to an assertion failure - but that's not the case in our code. We have 54 catch (const std::exception& e) cases in our codebase, some of which will happily continue after catching an out of bounds error.

Perhaps we should work on making those catch statements be more specific and guarantee they can't catch std::logic_errors? Without that, I'm very scared about replacing asserts with exceptions.

[Empact]

Ok my earlier message (now removed) related to a whitespace error - we only have 6 catch( blocks without a space separating the catch and parens, but we have 54 const std::exception catches and 71 if you include .... So the behavior change concern is fair. I'll drop the first commit as well.

relevant regex: catch ?\([^)]*(std::exception&|\.\.\.)

[Empact]

Checks are now strictly additional.

[Empact]

Removed a check relative to CreateTransaction nOutput where the value was locally guaranteed to be valid.

[DrahtBot]

The last travis run for this pull request was 65 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

[DrahtBot]

Needs rebase