Eliminate use of multiple-return from secp256k1.c. #218

Goto, multiple returns, continue, and/or multiple breaks in a loop are often used to build complex or non-local control flow in software.

(They're all basically the same thing, and anyone axiomatically opposing goto and not the rest is probably cargo-culting from the title of Dijkstra's essay without thinking hard about it.)

Personally, I think the current use of these constructs in the code base is fine: no where are we using them to create control- flow that couldn't easily be described in plain English, which is hard to read or reason about, or which looks like a trap for future developers.

Some, however, prefer a more rules based approach to software quality. In particular, MISRA forbids all of these constructs, and for good experience based reasons. Rule based criteria (as opposed to good judgement) also has the benefit of being machine checkable and surviving individual developers.

(To be fair-- MISRA also has a process for accommodating code that breaks the rules for good reason).

I think that in general we should also try to satisfy the rules- based measures of software quality, except where there is an objective reason not do: a measurable performance difference, logic that turns to spaghetti, etc.

Changing out all the multiple returns in secp256k1.c appears to be basically neutral: Some parts become slightly less clear, some parts slightly more.

For discussion: to complete this I should also go and get the short-cutting returns in the group logic, normalizes, think hard about the continue in wnaf, etc.

I don't propose we hold the tests to the same standards.

@gmaxwell two first commits belong elsewhere.

@paveljanik They are elsewhere. This is a "For discussion" commit. I'm not going to waste my time rewriting patch not on-top of the other changes just to make github happy.

utACK for the multiple-return commit. I'd have done a couple of sections a bit differently. But, it really is six of one and half-a-dozen of the other. The patched code is readable and, as best I can tell, doesn't change the logic in a negative manner.

Also, +1 for initializing the return variables. All variables ought to be initialized whenever possible.

Also, +1 for initializing the return variables. All variables ought to be initialized whenever possible.

I usually disagree there: initializing something unnecessarily undermines static analysis (compiler warnings) that the code is broken and its possible to use the variable in a way that wasn't intended. With the initialization you just silently get the default value. I considered it safe to do in this case because returning an error is 'safe'. But maybe this was a mistake, since for Bitcoin's purposes returning an error on a signature that was actually valid is a security vulnerability.

Needs rebase.

rebased

ACK