Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FreeBSD and OpenBSD build fixes #1815

Closed
wants to merge 1 commit into from

Conversation

jrmithdobbs
Copy link
Contributor

OpenBSD is failing some tests related to wallet coin selection AND SHOULD NOT UNDER ANY CIRCUMSTANCES BE USED AS THE OS ON ANY INSTANCES HANDLING A WALLET YET.

The *.d cleanup in the clean target is needed for instances when you try and link against a bad db version (<4.8) but they shouldn't exist under "normal" circumstances. Should still clean them up.

The swap of the bdb and boost include search paths is intentional. The openbsd port/package for db4 is stuck at 4.6 for right now and exists in /usr/local. Boost also exists in /usr/local so if it's specified first you can't get the linker to find the correct bdb.

Getting the unit tests working on OpenBSD requires patching the boost 1.42 port before building as the provided one will not work with any unit tests whatsoever.

I'll write up build instructions for both platforms for a different pull request.

I've tested that this builds on both OpenBSD 5.1 and FreeBSD 9.0-RELEASE-p4. (AKA, current stable versions as of this writing.)

I've also tested that it does not break builds on debian wheezy.

Edit:

FreeBSD build example:

BOOST_INCLUDE_PATH=/usr/local/include
BOOST_LIB_PATH=/usr/local/lib
BDB_INCLUDE_PATH=/usr/local/include/db48
BDB_LIB_PATH=/usr/local/lib/db48
gmake -f makefile.unix -j8 USE_UPNP= bitcoind test_bitcoin

OpenBSD build example:

BOOST_INCLUDE_PATH=/usr/local/include
BOOST_LIB_PATH=/usr/local/lib
BDB_INCLUDE_PATH=/opt/OpenBSD/5.1/amd64/db4-4.8.30/include
BDB_LIB_PATH=/opt/OpenBSD/5.1/amd64/db4-4.8.30/lib
BOOST_LIB_SUFFIX=-mt
gmake -f makefile.unix -j8 USE_UPNP= bitcoind test_bitcoin

See this pastebin for details on the failing unit tests on OpenBSD: http://pastebin.com/kFjiXui2

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f518b2c39868eeacae871dc79a7f270736b984e8 for binaries and test log.

aiHint.ai_flags = fAllowLookup ? AI_ADDRCONFIG : AI_NUMERICHOST;
# else
aiHint.ai_flags = fAllowLookup ? 0 : AI_NUMERICHOST;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just
#define AI_ADDRCONFIG 0
?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add (#ifndef AI_ADDRCONFIG #define AI_ADDRCONFIG 0) to compat.h, it has other such special cases.

@laanwj
Copy link
Member

laanwj commented Sep 11, 2012

I think it's best to a OPENBSD=1 option to the makefile for the OpenBSD-specific changes (unless there's a way to detect that automagically...).

@jrmithdobbs
Copy link
Contributor Author

They're not openbsd specific, they apply to both (and probably netbsd) platforms. The db thing is weird and can occur on other platforms from what I can tell.

I moved the AI_ADDRCONFIG def into compat.h where it should be. Nice catch.

@laanwj
Copy link
Member

laanwj commented Sep 11, 2012

BSD=1 then...

@jrmithdobbs
Copy link
Contributor Author

BSD is already a reserved define since BSD4.3. (It's a version number, i forget details of parsing it.)

@jrmithdobbs
Copy link
Contributor Author

The pthread_np.h header stands for "non portable," I know that function exists on those two platforms in the expected form, I don't know if there's something better to key off of for it.

@jrmithdobbs
Copy link
Contributor Author

Cleaned up and clarified some comments.

@laanwj
Copy link
Member

laanwj commented Sep 11, 2012

I don't really care what specific flag you use, but you should put the OBSD specific changes (especially the -ldl) behind some flag so that build on Linux is unaffected.

@jrmithdobbs
Copy link
Contributor Author

The stuff in the makefile doesn't have access to the things that define things like FreeBSD, etc since they're in included headers is the bigger problem. I'm trying to hack it in as cleanly as possible without resorting to autotools because that's a big change. (and last time that got left to rot when jaromil did it)

Trying to make as few changes as possible to attempt to get this fixed, at least for freebsd (don't know that i'll have time to track down the openbsd wallet issues), before 0.7 final.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e56b181ffc35c6eaf488fd09fc0948c2ea722158 for binaries and test log.

@jgarzik
Copy link
Contributor

jgarzik commented Nov 16, 2012

ACK, seems sane, needs rebase.

Maybe HAVE_CXX_STDHEADERS should be conditionalized for the platforms that need it.

@luke-jr
Copy link
Member

luke-jr commented Apr 8, 2013

Needs rebase.

@jgarzik
Copy link
Contributor

jgarzik commented May 30, 2013

Closing due to inattention. If you want to rebase, feel free to re-open.

@jgarzik jgarzik closed this May 30, 2013
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
…further clean up.

063401c PoS miner: add height to the first input scriptSig of the coinstake tx + remove unneded IncrementExtraNonce call in PoS blocks. (furszy)
7fc4680 Do not calculate the block value twice for the same block. (furszy)
7893818 Removing not needed CBlock::payee member. (furszy)
3e07280 Remove unused zPIVStake argument in FillBlockPayee. (furszy)
f702628 miner: decouple coinbase transaction creation into its own function. (furszy)
377f87e FillBlockPayee: reworked to not require chainActive.Tip redundant access. (furszy)
920652b miner: remove FillBlockPayee unneded fee argument + refactor coinbase tx creation to be in one single place and not dispersed across the CreateNewBlock method. (furszy)

Pull request description:

  Another step forward improving the miner `CreateNewBlock` function.
  This time, have unified the coinbase tx creation flow that was previously dispersed over the function and having some redundant initializations/sets.
  Plus, cleaned up some unused and/or unneeded fields and function arguments.

  This is a preparation for bitcoin#1815 work.  It will not be possible to modify any of the elements of a transaction once them are appended to a block, thus why the transaction needs (more than ever) to be crafted in one single place and not all over the sources.

ACKs for top commit:
  Fuzzbawls:
    ACK 063401c
  random-zebra:
    utACK 063401c and merging...

Tree-SHA512: c32cd87f82d9b31dedf9d47063a67978ef683bf35ff625823d6ece1a2d2904599dcbf15791682ffc1deb412082b1ecc24d11b364e300d4ceb259c65e65b0e9f6
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
911e31c Make CBlock::vtx a vector of shared_ptr<CTransaction> (furszy)
9d27b75 Add deserializing constructors to CTransaction and CMutableTransaction (Pieter Wuille)
5f90940 Add serialization for unique_ptr and shared_ptr (Pieter Wuille)

Pull request description:

  Inspired on bitcoin#9125 (with many more adaptations), preparation for bitcoin#8580. Ant work 🐜

  Establishing the bases for the work, we will need to continue migrating `CTransaction` to `std::shared_ptr<const CTransaction>` where is possible. Example bitcoin#8126 (can find more in bitcoin#1726 list).

  TODO:
  * back port final `CTransactionRef` implementation commit.

  EDIT:
  This is now depending on bitcoin#1816 .

ACKs for top commit:
  random-zebra:
    ACK 911e31c
  Fuzzbawls:
    ACK 911e31c

Tree-SHA512: 61d937aba7dce4ac0867496e7f81ae8351dcdd60b4e72c4f0ed58152a7e556bf455836c766bc97bbca331227e5deed92fa5ce609fa63bb9cb71600b430dc4405
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
…actoring.

9d8b8a2 BugFix: CMutableTransaction following the same sapling nVersion rules as CTransaction. (furszy)
1ca1b89 PeerLogicValidation missing destructor declaration warning fix. (furszy)
093a766 Introduce convenience type CTransactionRef (furszy)
8345a87 Refactor: make the read function simpler (gnuser)

Pull request description:

  PR solving the current master's syncing issue (syncing process not passing through block 5840), solved in 9d8b8a2.

  Essentially, when we merged bitcoin#1815, we moved from using the default transaction constructor and the ser/unser template methods (`SerializationOp`) to be using, inside the templated serialization puzzle, the deserializing constructor (`CTransaction(deserialize_type, Stream& s)`) which internally creates a `CMutableTransaction` which wasn't having the same sapling tx version guard as `CTransaction` ser/unser method. So, in other words, it was trying to parse the shielded transaction data from an old version two transaction (yes, we already have version two transaction in our network.. first one is in block 5840).

  Plus, i took the mischief of not only including the bugfix, have added:

  * stream::read function readability improvement coming from bitcoin#11221.
  * a pretty straightforward adaptation of upstream's b4e4ba4  (missing last commit not included in bitcoin#1815).
  * a `PeerLogicValidation` class compiler warning fix 1ca1b89

ACKs for top commit:
  random-zebra:
    ACK 9d8b8a2
  Fuzzbawls:
    ACK 9d8b8a2

Tree-SHA512: ffb60001160d177d20da3d9198f73910921ec521e6cae1ccbf9f6359dc96e3e4fdbbcaabb85331ce2a8b839c07ab34b3981c69d83aa6990aa79e134588539f48
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants