I'm not a fan of adding more try/catch to consensus code, but that's (mostly) a matter of personal taste. Any reason for not simply returning a (differently named) ConsensusException object? Also, as another matter of personal preference, reusing the name CheckTransaction() makes it hard to read/grep. I suppose if we started using the Consensus namespace as a convention I wouldn't mind that though.

grumble..bikeshed...consensus.cpp...

Concept ACK.

@Diapolo 1) Will fix the #ifndef 2) Good catch on #include "primitives/block.h" // for MAX_BLOCK_SIZE maybe we can completely remove that include from a couple of places.
3) Agree on MoneyRange but I didn't wanted to change functionality in this PR. Maybe it's not a big deal, let's see what others say about it. There's other potential optimizations there (for example, if every individual tx.vout[i].nValue is positive, nValueOut cannot be negative).

@theuni Yes, the point of the namespace was to avoid naming these functions ConsensusCheckTransaction(), etc. But I'm fine also changing the name. What about Consensus::CheckTx() ?

@both of you, I agree that ConsensusException is not the most elegant thing in the world as it is now. @Diapolo proposes to unify functionIn and strErrorIn into maybe descriptionIn. That makes sense. I would prefer not putting there anything at all. Remember that this is only for the log. Would it be much worse if instead of using error(e.what()) we just used error("Consensus::CheckTransaction: %s", e.reason) in
https://github.com/bitcoin/bitcoin/pull/5669/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR817
??
Instead of logging "CheckTransaction() : vin empty" we would log "CheckTransaction() : bad-txns-vin-empty" and things like that. If you ask me, it is worth it if it allows us to remove an attribute of the object (be it an exception or a returned object).
About not throwing Exceptions, I was trying to avoid many i/o parameters like int& nDoS and std::string& reason, but it is true that returning an object would do it.
Some errors have nDoS = 10 and most of them have 100. If they all had 100, we could save another attribute for the object. or return bool and have a string& reason param. or return the reason string and treat an empty string as success check...We can have something more beauty if we're willing to minimally change functionality.

@jtimon IMHO it makes absolutely sense to create an additional commit, which picks up my suggestion and perhaps also your optimisation ideas. It makes things easy to review if the last commit would be that.

About my suggestion for the input strings, I just meant changing them to const std::string&, if possible. I did not think about making other changes :).

Thanks for also picking up the other things!

Since it is still very new, I took the liberty of directly editing the commits instead of adding new ones to later rebase and squash.
Changes done:

1. The exception is changed for a Consensus::Result object that is returned as suggested by @theuni
2. The function attribute is removed as I thought @Diapolo was suggesting...
3. Not only the inputs to the constructor are "constified" as suggested by @Diapolo, but also the very attributes of the class.
4. The namespace is created in the first commit to make the second more purely MOVEONLY, and the new Result class is moved inside the namespace.
5. Added a last commit as suggested by @Diapolo with his simplification and my tiny tiny optimization.

Oh, and bikeshed from CheckTransaction() to CheckTx().

Also, I replaced a couple of #include "primitives/block.h" // for MAX_BLOCK_SIZE for #include "consensus/consensus.h" // for MAX_BLOCK_SIZE instead of just adding #include "consensus/consensus.h"

Hmm, looking at this again.. What's the reason for not dropping state.Abort() (it makes no sense for a state object to be aborting the node anyway imo), creating something like "abortNodeBadState(CValidationState& state, std::string reason)" to take its place, and moving CValidationState into consensus.o ? That way we could share much more code with the current implementation.

I was curious to see what it'd look like, so I tried out the above suggested change in one big nasty commit: theuni@fd19dd4

It seems quite advantageous to have CValidationState in consensus.o.

Yesterday I realized that the result object should be more generic to be used by other consensus functions. Also it would be useful for some policy methods.
I concluded that the simplest thing would be to mimic CValidationState::DoS() header:
jtimon@9e67b59 and jtimon@e92e852

But your approach is definitely simpler. The only disadvantage being the lack of an error string to be passed and thus changing a little bit the logging.
For example, instead of logging ERROR: Consensus::CheckTx(): vin empty your approach would log ERROR: Consensus::CheckTx(): bad-txns-vin-empty instead.
Because the reason contained in the state it's different from the string passed to bool error(char*) called to return false for the second parameter of CValidationState::DoS().
In my opinion this is completely acceptable and will also make those calls to CValidationState::DoS() shorter. We will still need to touch those lines to remove the call to error(), but everything will be simpler to do and review.

So I think I'm going to redo this on top of an edited version of your commit.

Rebased with CValidationState in consensus/validation.h.
I chose not to put it in consensus/consensus.h because:

1. Not everything that needs consensus.h needs validation.h (like merkleblock.cpp bitcoin-tx.cpp for now)
2. Not everything that needs validation.h needs consensus.h (like probably policy.o)

Feel free to bikeshed the file names.

Mm, this is now hard to review because it's such a moving target. I was ready to ACK the changes after another quick review based on your comment above, but lots of other things have been touched since then. Could you try to keep this to a somewhat narrow scope for the sake of review?

Also, since a fresh review is needed, please go ahead and rebase to remove the squashme's.

Rebased on top of #5681, added some "SQUASHME" notes since some small commits like those flagged with "MOVEONLY", are only useful for the initial review (for example I separated theuni/bitcoin@fd19dd4 into jtimon@72e5068 and jtimon@8a6590c).
I am very sorry for having screwed review by having rebased on top of #5681.
I should have signaled more clearly that this was a work in progress but I'm trying to keep the policy--refactor-related code in sync with this and I was really having problems editing and moving commits up and down without some solid include basis. I'm thankful to @jgarzik that C's include philosophy is to include everything that you're using, not just the higher level files. I hope that he can give some time to #5681 to make sure I didn't learned anything the wrong way.
I'm also trying to keep in sync https://github.com/luke-jr/bitcoin/compare/nodepolicy2 and #5180.
I hope reviewers don't get mad because I'm using #5681 as a common base for some seemingly unrelated previous proposals, but I believe they will become much easier to maintain.
I needed that to know which #include "main.h" I could really delete when moving things out of main in parallel.
I'll push my longest consensus/policy-related branch soon. If it doesn't make any sense to anyone I can go back to a version without #5681 on the consensus and policy related PRs I have. I may close some of them with some acks to if it makes more sense to concentrate related changes.

Ok. Concept ACK here. I'll re-review and verify the MOVEONLY's after #5681.

Needed rebase. Also I thought more about it and it's good to separate the class from main as soon as possible not only for consensus encapsulation but also for policy encapsulation (maybe wallet too? ), and include organization. So I put the moveonly commit back in.
I left the commit moving the consensus constants out instead, which is included in another mergeable PR (#5595) anyway.

ACK. I think this is merge-ready. My only nit would be validation.h -> validationstate.h, but not a big deal either way.

I'd like to see the DoS functionality moved out, but that can be discussed after this goes in.

I could rename it but I would prefer not to do so to avoid rebasing some branches based on this if it's not a big deal.
I agree the class can be simplified, even to the point of leaving it at the rejection reason, but yeah, we can discuss that later.

ACK, needs rebase though.

Longer term (but not for this PR), handling of system errors (the ERROR) state should be handled completely separately from CValidationState (yes, my fault that I introduced it there). There are several functions that only use a CValidationState to pass on possible system failure (and not actually failed validation).

rebased

Needed rebase

ACK.

reACK. Untested after rebase, but still looks sane.

Rebased

Ping @laanwj it would be cleaner to merge this before #6051

utACK

ut ACK