748157913 [tests] Make comp test framework more debuggable (John Newbery)
Pull request description:
We should remove the comparison test framework entirely (see #10603).
Until we do that, let's make it a bit more debuggable. Currently, if there's an assert in the framework, it's very difficult to track down where we are in the test generator. Make the logging a bit better to help with debugging.
Before this PR:
```
→ ./p2p-fullblocktest.py
2017-10-09 14:05:11.302000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testzdnax_yr
2017-10-09 14:05:11.557000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:11975
2017-10-09 14:05:11.712000 TestFramework.comptool (INFO): Test 1: PASS
2017-10-09 14:05:11.947000 TestFramework.comptool (INFO): Test 2: PASS
2017-10-09 14:05:12.057000 TestFramework.comptool (INFO): Test 3: PASS
2017-10-09 14:05:12.058000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "./p2p-fullblocktest.py", line 72, in run_test
self.test.run()
File "/home/ubuntu/bitcoin/test/functional/test_framework/comptool.py", line 306, in run
assert test_number != 4
AssertionError
2017-10-09 14:05:12.059000 TestFramework (INFO): Stopping nodes
2017-10-09 14:05:14.203000 TestFramework (WARNING): Not cleaning up dir /tmp/user/1000/testzdnax_yr
2017-10-09 14:05:14.204000 TestFramework (ERROR): Test failed. Test logging available at /tmp/user/1000/testzdnax_yr/test_framework.log
```
With this PR:
```
→ ./p2p-fullblocktest.py
2017-10-09 14:03:54.069000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testuey7t3tf
2017-10-09 14:03:54.329000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:11783
2017-10-09 14:03:54.383000 TestFramework.comptool (INFO): Running test 1: ./p2p-fullblocktest.py line 184
2017-10-09 14:03:54.496000 TestFramework.comptool (INFO): Running test 2: ./p2p-fullblocktest.py line 193
2017-10-09 14:03:54.758000 TestFramework.comptool (INFO): Running test 3: ./p2p-fullblocktest.py line 205
2017-10-09 14:03:54.867000 TestFramework.comptool (INFO): Running test 4: ./p2p-fullblocktest.py line 208
2017-10-09 14:03:54.867000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 117, in main
self.run_test()
File "./p2p-fullblocktest.py", line 72, in run_test
self.test.run()
File "/home/ubuntu/bitcoin/test/functional/test_framework/comptool.py", line 309, in run
assert test_number != 4
AssertionError
2017-10-09 14:03:54.868000 TestFramework (INFO): Stopping nodes
2017-10-09 14:03:56.950000 TestFramework (WARNING): Not cleaning up dir /tmp/user/1000/testuey7t3tf
2017-10-09 14:03:56.950000 TestFramework (ERROR): Test failed. Test logging available at /tmp/user/1000/testuey7t3tf/test_framework.log
```
Tree-SHA512: 5525958b0098d661c281bd955c92c72bf81359464376e96d44e6c88f18aea57ba08270ecd564edda4b47d674e3b27d20e5b1060544bf8dd5c6a68103d2bb35b8
Static analyzer (and humans!) will see ...
```
else if (state.m_chain_sync.m_timeout == 0 || (state.m_chain_sync.m_work_header != nullptr && ...
```
... and infer that state.m_chain_sync.m_work_header might be set to nullptr,
and thus flag `state.m_chain_sync.m_work_header->GetBlockHash().ToString()`
as a potential null pointer dereference.
This commit makes the tacit assumption (m_work_header != nullptr) explicit.
Code introduced in 5a6d00 ("Permit disconnection of outbound peers on
bad/slow chains") which was merged into master four days ago.
620bae3 Require a steady clock for bench with at least micro precision (Matt Corallo)
Pull request description:
Using a non-steady high_precision_clock by default is definitely not what we want, and in practice steady_clock has more than enough precision. Should double-check that travis passes on this one to make sure we actually have at least microsecond precision on all platforms.
Tree-SHA512: 54a4af3b6addca9897e8ab04694f9461343691b475ca3ed2368595c37520612e284969be94a8ee3d7c66d16532f7bb16b6ad80284cbc153653e8ef2d56696e9d
We should generally avoid writing to debug.log unconditionally for
inbound peers which misbehave (the peer being about to be banned
being an exception, since they cannot do this twice).
To avoid removing logs for outbound peers, a new log is added to
notify users when a new outbound peer is connected which mimics
the version print.
487aff421 Check subtree consistency in Travis (Pieter Wuille)
e1d0cc23a Improve git-subtree-check.sh (Pieter Wuille)
Pull request description:
Apparently many of our subtrees get modified by PRs in this repository, without getting noticed.
To improve upon this:
* Make git-subtree-check.sh capable of doing a weaker consistency check (that doesn't need access to external repositories), but which should be sufficient to detect unintended changes. It can be fooled by a fake subtree merge commit, but that would hopefully be obvious to reviewers.
* Make Travis invoke this subtree check for each of our subtrees.
Note that Travis is currently expected to fail on this PR, as 2 out of 4 subtrees (`src/secp156k1` and `src/univalue` have been modified directly in master).
Tree-SHA512: 465b680392d3daf38a8c1dda77d6f74b1d1c23324c378774777fb95aa673e119a8f7e3ccc124e41d97b5ac8975f3d79f3015797d2d309666582394364917ec4e
fa0025dc3 Revert "Remove unused variable in shell script" (MarcoFalke)
Pull request description:
This partially reverts commit ab8e8b97a3 (#10771), as the variable is still used. See for example #11394.
Tree-SHA512: 1788d5471e1399d4a15d287cd8c41979833524e31b8fe61af8a7d20c9777828460d61ab87885a228ba7ca919f1d08703f4cb182d5840eb863e2154b3cf8ff4e6
a357293 Use MakeUnique<Db>(...) (practicalswift)
3e09b39 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (practicalswift)
8617989 Add MakeUnique (substitute for C++14 std::make_unique) (practicalswift)
d223bc9 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (practicalswift)
b45c597 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift)
29ab96d Use unique_ptr for dbenv (DbEnv) (practicalswift)
f72cbf9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift)
8ccf1bb Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift)
73db063 Use unique_ptr for upnp_thread (boost::thread) (practicalswift)
0024531 Use unique_ptr for dbw (CDBWrapper) (practicalswift)
fa6d122 Use unique_ptr:s for {fee,short,long}Stats (TxConfirmStats) (practicalswift)
5a6f768 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift)
860e912 Use unique_ptr for pwalletMain (CWallet) (practicalswift)
Pull request description:
Use `std::unique_ptr` (C++11) where possible.
Rationale:
1. Avoid resource leaks (specifically: forgetting to `delete` an object created using `new`)
2. Avoid undefined behaviour (specifically: double `delete`:s)
**Note to reviewers:** Please let me know if I've missed any obvious `std::unique_ptr` candidates. Hopefully this PR should cover all the trivial cases.
Tree-SHA512: 9fbeb47b800ab8ff4e0be9f2a22ab63c23d5c613a0c6716d9183db8d22ddbbce592fb8384a8b7874bf7375c8161efb13ca2197ad6f24b75967148037f0f7b20c
ab8e8b9 Remove unused variables in shell scripts. (practicalswift)
Pull request description:
Remove unused variables in shell scripts. Use `_` where we don't care about the result.
Tree-SHA512: 35049e79ee432c805f061456c32902a92811b5214d50ce6770b22d1442cc5999ed53cfe05bb2347f6995ca33c707a0f3fe92d5829c0385c4a3e254953924cbc4
dd9bb25 Fix code style in keystore.cpp/crypter.cpp (Jonas Schnelli)
208fda6 CCrypter: move relevant implementation out of the header (Jonas Schnelli)
3155fd2 CKeystore: move relevant implementation out of the header (Jonas Schnelli)
Pull request description:
Tree-SHA512: 4ce73cca5609199b74b8ff2614ee2b6af949545a1332a3a0135c6453c98665d2b0da171c1e390c9a2aec6b12b7fad931ec90084bb7c2defe243786bfc70daf60
725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky)
3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo)
Pull request description:
Nowhere else in the protocol do we send headers which are for
blocks we have not fully validated except in response to getheaders
messages with a null locator. On my public node I have not seen any
such request (whether for an invalid block or not) in at least two
years of debug.log output, indicating that this should have minimal
impact.
Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a
fb00c45c3 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aabbd [tests] Remove support for p2p alert messages (John Newbery)
c0b127470 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e301c [tests] Remove dead code from mininode.py (John Newbery)
Pull request description:
This is the first part of #11518. It removes a ~150 lines of unused code from the mininode module:
- remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
- remove support for pre-BIP31 ping messages
- remove support for alert message
- explicitly don't support p2p versions lower than 60001
Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)
Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0
6c4042a Assert that CWallet::SyncMetaData finds oldest transaction. (Eelis)
Pull request description:
Without this assert, the Clang static analyzer warns about subsequent dereferencing of copyFrom, because it can't be sure that it's not nullptr. See #9573.
Tree-SHA512: 83cbcb32c52c94fcfefbc90ec7de2011dacd6bdb0da35adc401b8d8dda6a86de2fa0403e2158592268c2cf15eef4f3d887d98c90f1031d4735d5f4bf9dbc1d23
5a5e4e9 [wallet] Remove CTransaction&() helper conversion operator from wallet implementation. (Karl-Johan Alm)
Pull request description:
The `CTransaction&()` operator in `CMerkleTx` makes conversion into `CTransaction`s transparent, but was marked as to-be-removed in favor of explicitly getting the `tx` ivar, presumably as the operator can lead to ambiguous behavior and makes the code harder to follow.
This PR removes the operator and adapts callers. This includes some cases of `static_cast<CTransaction>(wtx)` → `*wtx.tx`, which is definitely an improvement.
Tree-SHA512: 95856fec7194d6a79615ea1c322abfcd6bcedf6ffd0cfa89bbdd332ce13035fa52dd4b828d20df673072dde1be64b79c513529a6f422dd5f0961ce722a32d56a
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)
Pull request description:
Rationale:
* `delete ptr` is a no-op if `ptr` is `nullptr`
Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
7963335 Fix -disablewallet default value (João Barbosa)
b411c2a Improve -disablewallet parameter interaction (João Barbosa)
Pull request description:
The first commit logs a message for each configured wallet if `-disablewallet` is set:
```
bitcoind -printtoconsole -regtest -disablewallet -wallet=foo -wallet=bar
...
WalletParameterInteraction: parameter interaction: -disablewallet -> ignoring -wallet=foo
WalletParameterInteraction: parameter interaction: -disablewallet -> ignoring -wallet=bar
```
It also moves up the `-disablewallet` check which avoids the unnecessary `-wallet` soft set.
The second commit fixes the default value of `-disablewallet`, currently the value is correct, but it should use `DEFAULT_DISABLE_WALLET`.
The third commit can be dropped or squashed, just took the opportunity to fix the coding style there.
Tree-SHA512: bec13d2b2be5adf4680c77212020ed27dd05f15c4c73542d2005d91108bf704e2df1707ed2bec696e584ecd40eff7a63e25201fd70400222aa5a8da6aed6afeb
16be7dd Improve bitcoind systemd service file (Florian Schmaus)
Pull request description:
Add comment how further options can be added or existing ones
modified. Use /run/${RuntimeDirectory} for PID file.
Remove TimeoutStopSec, TimeoutStartSec, StartLimitInterval,
StartLimitBurst directives as those should be set indivdually.
Remove Group to user the bitcoin user's default group.
Changed Restart from 'always' to 'on-failure' (can also be overwritten
individually).
Tree-SHA512: f76674c11fd6e3faaf786aa05686926523d9c875aad6b776337f800108fdb716470286805c532b494f8cf713cb5eea6b735e1c7c238ffb407a5cc909dda41aa4
9db9d62 Refactor: make the read function simpler (gnuser)
Pull request description:
Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
bc9c0a7 Improve wallet-accounts test (Russell Yanofsky)
Pull request description:
Add comments and
- Verify sending to a account causes getaccountaddress to generate new addresses.
- Verify sending to a account causes getreceivedbyaccount to return amount received.
- Verify ways setaccount updates the accounts of existing addresses.
Tree-SHA512: 4facc8d10fb23847081e8081839d4050faa855cca40f34012b82297bf1a8cfdb975b26314bc518dc6b3f5997452a7acf309f9627e0dff359347c628b37014bfa
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.
The mininode module includes code to support p2p versions below
60001. However, the test_framework does not support versions
of Bitcoin Core before V0.13.0. Remove code supporting
p2p versions before 60001 (which has never been run).
BIP31 support was added to Bitcoin Core in version 0.6.1. Our test
framework is incompatible with Bitcoin Core versions that old, so remove
all special logic for handling pre-BIP31 pings.
32ae82f5c [tests] use TestNode p2p connection in tests (John Newbery)
5e5725cc2 [tests] Add p2p connection to TestNode (John Newbery)
b86c1cd20 [tests] fix TestNode.__getattr__() method (John Newbery)
Pull request description:
Final two steps of #10082 : Adding the "mininode" P2P interface to `TestNode`
This PR adds the mininode P2P interface to `TestNode`. It simplifies the process for opening a P2P connection to the node-under-test from this:
```python
node0 = NodeConnCB()
connections = []
connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], node0))
node0.add_connection(connections[0])
```
to this:
```python
self.nodes[0].add_p2p_connection(p2p_conn_type=NodeConnCB)
```
The first commit adds the infrastructure to `test_node.py`. The second updates the individual test cases to use it. Can be separated if this is too much review for one PR.
Tree-SHA512: 44f1a6320f44eefc70489ae8350c6a16ad1a6035e4b9b7bafbdf19f5905ed0e2db85beaaf4758eec3059dd89a375a47a45352a029f39f57a86ab38a9ae66650e
p2p connections can now be added to TestNode instances.
This commit also updates the example test to use the new
p2p interface in TestNode to demonstrate usage.
A future commit will update the existing tests to use p2p through the
TestNode.
MarcoFalke