Consensus build package

jtimon commented at 3:07 pm on November 24, 2015: contributor

Although the encapsulations necessary to expose VerifyScript in libbitcoinconsensus were done, we’re still building it in different packages. Creating an independent consensus package will make much more clear which files are currently part of libbitcoinconsensus. Furthermore, other libconsensus-ready files like arith_uint256.o, consensus/params.h, consensus/validation.h and primitives/block.o can be moved to that package already. This will make it harder to “undo” consensus encapsulation work while having travis happy. Every executable that depends on the util and common packages now depends on the consensus package too. Also, every executable depending on the consensus package seems to depend on the crypto package too, and libbitcoin_consensus only dependencies are crypto and libsecp256k1, so it doesn’t seem to be of harm if we unify the crypto and consensus packages. If we are going to eventually move all the libconsensus code to a subtree, this will make clearer which files need to be moved.

I’m specially interested in @theuni ’s and travis’ opinion before I make some squashes and/or reduce the scope of the PR.

laanwj added the label Refactoring on Nov 24, 2015

jtimon force-pushed on Nov 24, 2015

jtimon commented at 0:16 am on November 25, 2015: contributor

After making travis happy (https://travis-ci.org/bitcoin/bitcoin/builds/93038691 thanks to @theuni ) I force push with some squashes, some suggestions (not all yet) collected from @theuni on IRC and one of the commits nacked and deleted (unifying the crypto and consensus packages). Interestingly enough, travis thinks that new branch is utterly wrong unless you use mac (see https://travis-ci.org/bitcoin/bitcoin/builds/93061856 ).

Building locally on xubuntu-0.14, it is clear to me that I’m not using the following commands efficiently when touching Makefile.am:

0make clean
1git clean -fdx
2./autogen.sh

Advice welcomed.

jtimon force-pushed on Nov 26, 2015

jtimon commented at 5:54 pm on November 26, 2015: contributor

Sorry for abusing travis but I got really paranoid while trying to find the needles (https://github.com/jtimon/bitcoin/compare/consensus-build...jtimon:consensus-build-full). I really don’t understand why the order of the packages in, for example, bitcoind_LDADD matters at linking time (including the local desperate random over-cleaning mentioned before). @sipa said on IRC that it shouldn’t matter too. If anyone else can bring some light to this, it would be very welcomed.

sipa commented at 12:55 pm on November 28, 2015: member

To clarify: it’s my understanding that the problem is that autotools generate both the command-line arguments to the linker, and the emitted rules in the makefile in the same order, based on the order of things in LDADD.

The order of arguments to the linker does matter (needs to be in order from dependers to dependencies), but for fast-failure behaviour, @jtimon wants them to be build in order from dependency to depender, which seems reasonable. @theuni Any idea about this?

jtimon commented at 1:16 pm on November 28, 2015: contributor

Yes, @sipa, that’s what I would like: that packages are built in the opposite way they have to be listed for linking (ie lowest level things first). I was going to ask something like that in http://github.com/jtimon/bitcoin/commit/4f5cf2655f150c65748989aef97c3b7f6a78f9d7#commitcomment-14660501 but your explanation is more complete. In any case, if that is possible, I think it would probably make sense to do that in a separate PR and leave this one as it is (module review/nits).

jtimon commented at 1:21 pm on November 28, 2015: contributor

By the way, the building order discussion is related to #5112

jtimon force-pushed on Nov 29, 2015

jtimon commented at 11:03 pm on November 29, 2015: contributor

Rebased(1).

jtimon force-pushed on Dec 1, 2015

jtimon commented at 9:38 pm on December 1, 2015: contributor

Rebased(2).

jtimon commented at 4:10 pm on December 3, 2015: contributor

@theuni and I plan to peer program this to also avoid building the objects in the consensus package twice like it’s being done now (once for libconsensus and once for the rest). So this will hopefully be replaced with something better.

jtimon closed this on Dec 3, 2015

Build: Consensus: Move consensus files from common to its own module/package a3d5eec546

Build: Libconsensus: Move libconsensus-ready files to the consensus package 4feadec98e

Build: Consensus: Make libbitcoinconsensus_la_SOURCES fully dynamic and dependend on both crypto and consensus packages

Some extra bytes in libconsensus to get all the crypto (except for signing, which is in the common module) below the libconsensus future independent repo (that has libsecp256k1 as a subtree).
hmac_sha256.o seems to be the only thing libbitcoinconsensus doesn't depend on from crypto, some more bytes for the final libconsensus: I'm not personally worried.

cf82d05dd4

jtimon reopened this on Dec 8, 2015

jtimon force-pushed on Dec 8, 2015

jtimon commented at 6:09 am on December 8, 2015: contributor

Reopened and updated (I had missed the new prevector [which by the way could have been created in the consensus folder directly instead of having to move it later]).

devrandom commented at 5:53 pm on December 10, 2015: none

ACK

Looks good, and I tested linking with a trivial main function.

Should also separate out the unit tests, but that could be postponed to a follow up PR. Let me know if you want me to take that on.

jtimon commented at 7:06 pm on December 10, 2015: contributor

At some point it would be ideal to separate the tests for the things in the consensus module/package to the consensus directory (just like wallet and qt). But I haven’t checked whether those tests have undesirable dependencies yet, and I didn’t plan to do any of that on the near future. So, yes, if you want to work on that, I’m happy to review.

devrandom commented at 7:11 pm on December 10, 2015: none

OK, cool. I’ll wait for this to be merged first.

MarcoFalke commented at 7:36 pm on December 10, 2015: member

Concept ACK

theuni commented at 6:39 am on December 16, 2015: member

ut ACK. Objections withdrawn. Lumping everything together is non-ideal, but it’s still an improvement. Makes sense as a step forward.

jtimon commented at 4:07 pm on January 12, 2016: contributor

@theuni I’m also eager to see it building the modules on the consensus package being built only once instead of once, but I’ll leave that in your hands. In the meantime, all the pictures on my promised document for a libconsensus encapsulation plan will make a lot more sense after I rewrite them after this is merged. Otherwise I’ll just have to refer to this PR (which I’ll do in any case), but it’s fine because there’s things that will need to be backported anyway (ie csv and segwit) to the main implementation branch which will be based on top of last-0.12.99 3cd836c1 instead of master for easier consensus backports. Sorry for the unrelated divagation…ping.

jtimon commented at 3:52 pm on January 28, 2016: contributor

anything holding this?

laanwj commented at 4:27 pm on February 2, 2016: member

Not really, going to test…

laanwj commented at 5:58 pm on February 2, 2016: member

ACK cf82d05

laanwj merged this on Feb 2, 2016

laanwj closed this on Feb 2, 2016

laanwj referenced this in commit fd13fe7ca0 on Feb 2, 2016

codablock referenced this in commit 5e8a8a9b19 on Sep 16, 2017

codablock referenced this in commit cc7d6bcb9c on Sep 19, 2017

codablock referenced this in commit 364efeb4cb on Dec 9, 2017

codablock referenced this in commit cc09eec674 on Dec 9, 2017

codablock referenced this in commit bcbd2cda24 on Dec 11, 2017

MarcoFalke locked this on Sep 8, 2021

Consensus build package #7091