util_tests.cpp needs to include the signal.h header on FreeBSD.
Reported by denis2342 on IRC.
Github-Pull: #12447
Rebased-From: dd7e42cbb4390788705031ffa0bc893d26f0597e
Tree-SHA512: 10ead029bb59f5d69e37b5679c710f22d64051de26e1ec8342eec4e4dec4d76249e16dff78d192972bcb8d139d99c7555a7cb2fe43b2b911103eab6d6f943b79
Add a unit test for LockDirectory, introduced in #11281.
Github-Pull: #12422
Rebased-From: 1d4cbd26e4220982f7f2f60e447199d6f62ae254
Tree-SHA512: 8186f4b22c65153e30ec1e0b68be20fd59d34e1fe565d99f3b5a302780125953b867fb14c37144de3fd7baf5751df94bf3901eebbb7b820491ca452706d4e205
An array with 7 elements was setup for checking argument parsing, but
was passed to ParseParamaeters with argc=5, meaning the interpretation
of the last two arguments was never actually checked.
This requires the removal of some very liberal (incorrect) cs_mains
sprinkled in some tests. It adds some chainActive.Tip() races, but
the tests are all single-threaded anyway.
blockchain.cpp has low unit test coverage. This commit is intended
to start improving its code coverage to reasonable levels. One or more
follow up commits will complete the task that this commit is starting
(though the usefulness of this commit is not dependent upon later
commits).
Note that these tests were not written based upon a specification of how
GetDifficulty *should* work, but rather how it actually *does* work. As
a result, if there are any bugs in the current GetDifficulty
implementation, these unit tests serve to lock them in rather than
expose them.
-- Why has blockchain.cpp been modified if this is a unit testing change?
Since the existing GetDifficulty function relies on a global variable,
chainActive, it was not suitable for unit testing purposes. Both the
existing GetDifficulty function and the unit tests now call through to
a new, more modular version of GetDifficulty that can work on any chain,
not just chainActive.
-- Why does blockchain_tests.cpp directly include blockchain.cpp instead
of blockchain.h?
While the new GetDifficulty function's signature is arguably better than
the old one's, it still isn't great, and doesn't seem to warrant inclusion
as part of the blockchain.h API, especially since only test code is
directly using it. If a better way of exposing the new GetDifficulty
function to unit tests exists, please mention it and the commit will be
updated accordingly.
-- Why is the test fixture named blockchain_difficulty_tests rather than
blockchain_tests?
The Bitcoin Core policy for naming unit test files is to match the the
file under test ("blockchain" becomes "blockchain_tests"). While this
commit complies with that, blockchain.cpp is a massive file, such that
having all of the unit tests in one file will tend towards disorder.
Since there will be a lot more tests added to this file, the intention
is to divide up different types of tests into different test fixtures
within the same file.
BOOST_CHECK_THROW merely checks that some std::runtime_error is
thrown, but not which one.
One example of how this could lead to a test passing when a developer
introduces a consensus bug: the test for the sigops limit assumes
that CreateNewBlock fails with bad-blk-sigops. However it can
also fail with bad-txns-vout-negative, e.g. if a naive developer lowers
BLOCKSUBSIDY to 1*COIN in the test.
BOOST_CHECK_EXCEPTION allows an additional predicate function. This
commit uses this for all exceptions that are checked for in
miner_tets.cpp:
* bad-blk-sigops
* bad-cb-multiple
* bad-txns-inputs-missingorspent
* block-validation-failed
An instance of the CheckRejectInvalid class (for a given validation string)
is passed to BOOST_CHECK_EXCEPTION.
If our tip hasn't updated in a while, that may be because our peers are
not relaying blocks to us that we would consider valid. Allow connection
to an additional outbound peer in that circumstance.
Also, periodically check to see if we are exceeding our target number of
outbound peers, and disconnect the one which has least recently
announced a new block to us (choosing the newest such peer in the case
of tie).
The LevelDB docs seem to indicate that an iterator will not take
snapshots (even providing instructions on how to do so yourself).
In several of the places we use them, we assume snapshots to have
been taken.
In order to make sure LevelDB doesn't change out from under us
(and to prevent the next person who reads the docs from having the
same fright I did), verify that snapshots are taken in our tests.
Combine fLimitFree and fOverrideMempoolLimit into a single boolean:
bypass_limits. This is used to indicate that mempool limiting based on feerate
should be bypassed. It is used when readding transactions from a reorg and then
the mempool is trimmed to size after all transactions are added and they can be
evaluated in the context of their descendants. No changes to behavior.
This resolves an issue where estimatesmartfee would return 999
sat/byte instead of 1000, due to floating point loss of precision
Thanks to sipa for suggesting is_integral.
There are a few too many edge-cases here to make this a scripted diff.
The following commits will move a few functions into PeerLogicValidation, where
the local connman instance can be used. This change prepares for that usage.
Wladimir J. van der Laan