Separate CValidationState from main

jtimon commented at 7:35 pm on January 15, 2015: contributor

First decouple CValidationState from main::AbortNode(), Then move it to consensus/validation.h This is part of the effort to create libconsensus. It’s also useful for policy encapsulation.

jtimon force-pushed on Jan 15, 2015

theuni commented at 4:44 am on January 16, 2015: member

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.

in src/consensus/consensus.cpp: in 89ce4684ba outdated

25+        if (tx.vout[i].nValue < 0)
26+            throw ConsensusException(100, __func__, "txout.nValue negative", "bad-txns-vout-negative");
27+        if (tx.vout[i].nValue > MAX_MONEY)
28+            throw ConsensusException(100, __func__, "txout.nValue too high", "bad-txns-vout-toolarge");
29+        nValueOut += tx.vout[i].nValue;
30+        if (!MoneyRange(nValueOut))

Diapolo commented at 7:31 am on January 16, 2015:

Just a question, MoneyRange() does return (nValue >= 0 && nValue <= MAX_MONEY), why not reuse MoneyRange() also for the check before accumulating the amounts and just give another error message? This would de-duplicate the above code.

in src/consensus/consensus.h: in 89ce4684ba outdated

0@@ -0,0 +1,43 @@
1+// Copyright (c) 2009-2010 Satoshi Nakamoto
2+// Copyright (c) 2009-2014 The Bitcoin Core developers
3+// Distributed under the MIT software license, see the accompanying
4+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+#ifndef BITCOIN_CONSENSUS_H

Diapolo commented at 7:32 am on January 16, 2015:

This has to be BITCOIN_CONSENSUS_CONSENSUS_H, because you are in the directory consensus.

in src/consensus/consensus.h: in 89ce4684ba outdated

19+public:
20+    int nDoS;
21+    std::string reason;
22+    std::string description;
23+
24+    ConsensusException(int nDoSIn, std::string functionIn, std::string strErrorIn, std::string reasonIn)

Diapolo commented at 7:33 am on January 16, 2015:

Perhaps some of the input strings can be constified?

in src/merkleblock.cpp: in 89ce4684ba outdated

 4@@ -5,6 +5,7 @@
 5 
 6 #include "merkleblock.h"
 7 
 8+#include "consensus/consensus.h"
 9 #include "hash.h"
10 #include "primitives/block.h" // for MAX_BLOCK_SIZE

Diapolo commented at 7:35 am on January 16, 2015:

If MAX_BLOCK_SIZE really should go into consensus.h then you need to remove this comment (and perhaps we have that also in other places).

jtimon commented at 10:55 am on January 16, 2015: contributor

@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.

Diapolo commented at 11:08 am on January 16, 2015: none

@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!

jtimon force-pushed on Jan 16, 2015

jtimon commented at 12:48 pm on January 16, 2015: contributor

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:

The exception is changed for a Consensus::Result object that is returned as suggested by @theuni
The function attribute is removed as I thought @Diapolo was suggesting…
Not only the inputs to the constructor are “constified” as suggested by @Diapolo, but also the very attributes of the class.
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.
Added a last commit as suggested by @Diapolo with his simplification and my tiny tiny optimization.

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

jtimon commented at 12:51 pm on January 16, 2015: contributor

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"

theuni commented at 10:53 pm on January 16, 2015: member

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.

theuni commented at 11:53 pm on January 16, 2015: member

I was curious to see what it’d look like, so I tried out the above suggested change in one big nasty commit: https://github.com/theuni/bitcoin/commit/fd19dd4b92e27d2e967daf47c36147eaed3d1e22

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

jtimon commented at 11:20 am on January 17, 2015: contributor

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: https://github.com/jtimon/bitcoin/commit/9e67b59d52834fb7b52ad9e3b428117c35d28822 and https://github.com/jtimon/bitcoin/commit/e92e852413c4c8418edd872230710570c4a6fc6f

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.

jtimon force-pushed on Jan 17, 2015

jtimon commented at 1:37 pm on January 17, 2015: contributor

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

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

Feel free to bikeshed the file names.

laanwj added the label Validation on Jan 19, 2015

jtimon force-pushed on Jan 19, 2015

theuni commented at 9:03 pm on January 19, 2015: member

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.

jtimon force-pushed on Jan 19, 2015

jtimon commented at 0:08 am on January 20, 2015: contributor

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@fd19dd4 into https://github.com/jtimon/bitcoin/commit/72e5068ebe1aad33f05a02f7eabe65abb098dff0 and https://github.com/jtimon/bitcoin/commit/8a6590ccb3f51978333d50878af3d79005ca4dfb). 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.

theuni commented at 1:06 am on January 20, 2015: member

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

jtimon force-pushed on Jan 20, 2015

jtimon force-pushed on Jan 21, 2015

jtimon force-pushed on Jan 23, 2015

jtimon force-pushed on Jan 24, 2015

jtimon force-pushed on Jan 26, 2015

jtimon commented at 12:34 pm on January 26, 2015: contributor

Rebased, now it is dependent on #5696 instead of #5681.

jtimon force-pushed on Feb 3, 2015

jtimon commented at 8:59 pm on February 3, 2015: contributor

Needed rebase

jtimon force-pushed on Feb 15, 2015

jtimon commented at 9:04 pm on February 15, 2015: contributor

Rebased on top of rebased #5696

jtimon commented at 9:48 pm on February 21, 2015: contributor

Rebased on top of the modified #5696

jtimon force-pushed on Feb 21, 2015

jtimon force-pushed on Mar 3, 2015

jtimon force-pushed on Mar 11, 2015

jtimon commented at 1:29 pm on March 11, 2015: contributor

Needed rebase

jtimon force-pushed on Mar 13, 2015

jtimon commented at 11:51 am on March 13, 2015: contributor

Rebased to make travis happy, like in #5696

jtimon force-pushed on Mar 13, 2015

jtimon commented at 12:41 pm on March 13, 2015: contributor

Rebased without @laanwj ’s fix after #5883 has been merged

jtimon force-pushed on Mar 17, 2015

jtimon commented at 10:59 am on March 17, 2015: contributor

Once again, rebased on top of a modified #5696.

jtimon force-pushed on Mar 24, 2015

jtimon force-pushed on Mar 25, 2015

jtimon force-pushed on Mar 26, 2015

jtimon commented at 11:59 am on March 26, 2015: contributor

Needed rebase

jtimon force-pushed on Mar 28, 2015

jtimon commented at 12:57 pm on March 28, 2015: contributor

I left the MOVEONLY commit moving Consensus::CheckTx() for later to move more functions at once.

jtimon renamed this:
~~Consensus: Move CheckTransaction() from main to consensus/consensus~~
Consensus preparations
on Apr 1, 2015

jtimon force-pushed on Apr 1, 2015

jtimon commented at 1:08 pm on April 1, 2015: contributor

Reducing the scope of the PR again. It doesn’t move any policy global or constant anymore. It doesn’t refactor main::CheckTransaction() either. So what remains is basically creating the consensus/consensus.h file with the consensus constants and preparing CValidationState for consensus movements.

jtimon force-pushed on Apr 19, 2015

jtimon commented at 10:44 pm on April 19, 2015: contributor

Needed rebase. Also I reduced the scope of the PR once again. The moveonly commit can wait for more things to be moved at the same time, but the AbortNode() vs state.Abort() is making me work more with rebases. That’s why I included the decoupling commit in #5970

jtimon renamed this:
~~Consensus preparations~~
Separate CValidationState from main
on Apr 20, 2015

jtimon force-pushed on Apr 20, 2015

jtimon commented at 7:10 pm on April 20, 2015: contributor

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.

theuni commented at 8:49 pm on April 23, 2015: member

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.

jtimon commented at 9:37 pm on April 23, 2015: contributor

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.

sipa commented at 10:08 am on April 24, 2015: member

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).

jtimon force-pushed on Apr 25, 2015

jtimon commented at 10:00 am on April 25, 2015: contributor

rebased

jtimon force-pushed on Apr 26, 2015

jtimon commented at 1:21 pm on April 26, 2015: contributor

Needed rebase

jonasschnelli commented at 5:38 pm on May 9, 2015: contributor

ACK.

theuni commented at 7:03 pm on May 9, 2015: member

reACK. Untested after rebase, but still looks sane.

Consensus: Refactor: Decouple CValidationState from main::AbortNode() 27afcd89db

Consensus: MOVEONLY: Move CValidationState from main consensus/validation da29ecbcc6

jtimon force-pushed on May 15, 2015

jtimon commented at 2:06 pm on May 15, 2015: contributor

Rebased

jtimon commented at 9:30 pm on May 19, 2015: contributor

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

laanwj commented at 10:35 am on May 21, 2015: member

utACK

theuni commented at 0:39 am on May 22, 2015: member

ut ACK

laanwj merged this on May 27, 2015

laanwj closed this on May 27, 2015

laanwj referenced this in commit c7c9af381c on May 27, 2015

MarcoFalke locked this on Sep 8, 2021

Separate CValidationState from main #5669