Consensus: Trivial transform BOOST

NicolasDorier commented at 5:22 am on July 15, 2016: contributor

For consensus block verification method into libconsensus, we need to get rid of BOOST dependencies. This commit trivially transform the BOOST_FOREACH into for loop so we can move those functions into libconsensus later. Ping @jtimon

NicolasDorier force-pushed on Jul 15, 2016

in src/main.cpp: in 3721aecd87 outdated

1060@@ -1061,24 +1061,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state)
1061 
1062     // Check for negative or overflow output values
1063     CAmount nValueOut = 0;
1064-    BOOST_FOREACH(const CTxOut& txout, tx.vout)
1065+    for(auto txout = tx.vout.begin(); txout != tx.vout.end(); txout++)

dcousens commented at 5:47 am on July 15, 2016:

Why not?

0for (auto txout : tx.vout) {

It’s C++11

sipa commented at 5:53 am on July 15, 2016:

Please use

0for (const auto& txout : tx.vout)

to avoid copying the entire txout.

NicolasDorier force-pushed on Jul 15, 2016

NicolasDorier commented at 7:00 am on July 15, 2016: contributor

Fixed nit, now using C++11 iteration loop style.

MarcoFalke commented at 7:19 am on July 15, 2016: member

move those functions into libconsensus later

Concept ACK on doing required trivial refactors before moving chunks of code.

MarcoFalke added the label Refactoring on Jul 15, 2016

NicolasDorier commented at 7:22 am on July 15, 2016: contributor

I’m doing a big PR on #8339 that is still work in progress. My goal is to make it smaller by doing trivial stuff I come accross while doing it, then I will rebase on top of it.

dcousens commented at 7:33 am on July 15, 2016: contributor

utACK 4358252

NicolasDorier force-pushed on Jul 15, 2016

NicolasDorier commented at 10:32 am on July 15, 2016: contributor

@dcousens updated the commit, I forgot GetLegacySigopsCount

jtimon commented at 11:32 am on July 15, 2016: contributor

Concept ACK on doing required trivial refactors before moving chunks of code.

Why is that? Why can’t #8329 , for example, happen before this?

Regarding the PR, I would prefer to move to plain C fors to move towards C rather than towards C++11 (I mean for libconsensus code only). But I don’t care enough to nack.

NicolasDorier commented at 1:08 pm on July 15, 2016: contributor

I would prefer C++ 11, only because I’m a spoiled kid using high level language like C# and even with C++11 I feel at the stone age. :(

I don’t think it make sense to move something in consensus/* which can’t yet be compiled into libconsensus. But I’m not feeling strongly about it neither.

MarcoFalke commented at 1:47 pm on July 15, 2016: member

Why is that? Why can’t #8329 , for example, happen before this?

Haven’t seen the other pull is already open. I have no strong opinion on which should go first. My comment was meant as a general guideline in case the more disruptive pull may not be uncontroversial enough to merge immediately and the trivial improvement can go in right now. Also note that the plan was to get rid of some boost stuff in 0.14, anyway. We could even do the replacement for the whole file … hides .

jtimon commented at 1:47 pm on July 15, 2016: contributor

Oh, personally I like the new loop more, and I’m very used to classes. Most of my experience is with C++, java and python, not C. But apparently some analysis tools are much more mature for C than for C++ and we could potentially benefit from them for consensus code if it was C. Maybe @gmaxwell can say more about this. In any case, was not a nack.

jtimon commented at 1:56 pm on July 15, 2016: contributor

@MarcoFalke well, I was asking on the general guideline. It makes sense to open both options because we don’t know what’s going to be merged first. That’s what I’ve been doing so far: any changes after a moveonly have been mostly ignored and the moveonlies themselves, separated or not, have been delayed forever. With the exception of this PR and #8341 all “dumb fixes” have been already done. We need to make new consensus functions to be populated with code from main, mostly from connectBlock. But those functions will need to call existing consensus functions. Therefore either we move before doing that or we introduce a circular dependency between the new consensus file and main that won’t be resolved until the existing consensus functions are moved out of main. Plus people would have a place to write new consensus functions like, say SequenceLocks() better than main. Sorry, maybe we should discuss this in #8329

dcousens commented at 2:39 pm on July 15, 2016: contributor

@jtimon there is so much that would have to change to make libconsensus C only. This for-loop change does not change that in any way. If a dedicated effort begins, when libconsensus is actually separated, to make it solely C, I’m sure these loops will be the simplest to change.

jtimon commented at 2:53 pm on July 15, 2016: contributor

@dcousens agreed, while we keep adding classes to the consensus code, these lines are not a concern. I still would have preferred to do this with the old style loops ages ago.

in src/main.cpp: in 55b70b0042 outdated

 998@@ -999,11 +999,11 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool
 999 unsigned int GetLegacySigOpCount(const CTransaction& tx)
1000 {
1001     unsigned int nSigOps = 0;
1002-    BOOST_FOREACH(const CTxIn& txin, tx.vin)
1003+    for (const CTxIn& txin : tx.vin)

paveljanik commented at 8:26 pm on July 15, 2016:

Why are you using CTxIn& here and CTxOut& below instead of auto& as in other changes? Is there any reason for this I do not see?

luke-jr commented at 8:36 pm on July 15, 2016: member

utACK (and don’t mind using auto where @paveljanik suggests)

This for loop cannot be C period - it’s using classes. C++11 loops make sense.

Consensus: Trivial transform BOOST_FOREACH into for loop a3e1984651

NicolasDorier force-pushed on Jul 16, 2016

NicolasDorier commented at 2:09 am on July 16, 2016: contributor

@paveljanik good catch, I just updated commit to use auto everywhere.

paveljanik commented at 8:02 am on July 16, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/8342/commits/a3e19846519952a3d22e387031258a88ec24d280

btcdrak commented at 9:53 am on July 16, 2016: contributor

ACK a3e1984

jtimon commented at 10:38 pm on July 16, 2016: contributor

Upgrade from non-nack to utACK a3e1984

MarcoFalke commented at 8:24 am on July 17, 2016: member

utACK a3e1984

laanwj commented at 5:31 am on July 18, 2016: member

@jtimon We’ve switched the project to c++11 just so that the code can use c++11, please don’t come up with any lousy excuses not to. If you want to port the consensus code to some other language later that should not affect how development is done in this project.

utACK a3e1984 (after .13 split-off)

laanwj merged this on Jul 21, 2016

laanwj closed this on Jul 21, 2016

laanwj referenced this in commit 6f4092da80 on Jul 21, 2016

codablock referenced this in commit 376837387a on Sep 19, 2017

codablock referenced this in commit 69eb76ad92 on Dec 29, 2017

codablock referenced this in commit 0bea37d7b8 on Jan 8, 2018

vecopay referenced this in commit 49e92c8fe9 on Dec 16, 2018

andvgal referenced this in commit ac3584cfe7 on Jan 6, 2019

MarcoFalke locked this on Sep 8, 2021

Consensus: Trivial transform BOOST_FOREACH into for loop #8342