| Age | Commit message (Collapse) | Author |
|---|
|
|
|
Valgrind
5db506ba5943868cc2c845f717508739b7f05714 tests: Add option --valgrind to run nodes under valgrind in the functional tests (practicalswift)
Pull request description:
What is better than fixing bugs? Fixing entire bug classes of course! :)
Add option `--valgrind` to run the functional tests under Valgrind.
Regular functional testing under Valgrind would have caught many of the uninitialized reads we've seen historically.
Let's kill this bug class once and for all: let's never use an uninitialized value ever again. Or at least not one that would be triggered by running the functional tests! :)
My hope is that this addition will make it super-easy to run the functional tests under Valgrind and thus increase the probability of people making use of it :)
Hopefully `test/functional/test_runner.py --valgrind` will become a natural part of the pre-release QA process.
**Usage:**
```
$ test/functional/test_runner.py --help
…
--valgrind run nodes under the valgrind memory error detector:
expect at least a ~10x slowdown, valgrind 3.14 or
later required
```
**Live demo:**
First, let's re-introduce a memory bug by reverting the recent P2P uninitialized read bug fix from PR #17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have").
```
$ git diff
diff --git a/src/consensus/validation.h b/src/consensus/validation.h
index 3401eb64c..940adea33 100644
--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};
class TxValidationState : public ValidationState {
private:
- TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
+ TxValidationResult m_result;
public:
bool Invalid(TxValidationResult result,
const std::string &reject_reason="",
```
Second, let's test as normal without Valgrind:
```
$ test/functional/p2p_segwit.py -l INFO
2019-11-28T09:30:42.810000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__fc8q3qo
…
2019-11-28T09:31:57.187000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
…
2019-11-28T09:32:08.265000Z TestFramework (INFO): Tests successful
```
Third, let's test with `--valgrind` and see if the test fail (as we expect) when the unitialized value is used:
```
$ test/functional/p2p_segwit.py -l INFO --valgrind
2019-11-28T09:32:33.018000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_gtjecx2l
…
2019-11-28T09:40:36.702000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
2019-11-28T09:40:37.813000Z TestFramework (ERROR): Assertion failed
ConnectionRefusedError: [Errno 111] Connection refused
```
ACKs for top commit:
MarcoFalke:
ACK 5db506ba5943868cc2c845f717508739b7f05714
jonatack:
ACK 5db506ba5943868cc2c845f717508739b7f05714
Tree-SHA512: 2eaecacf4da166febad88b2a8ee6d7ac2bcd38d4c1892ca39516b6343e8f8c8814edf5eaf14c90f11a069a0389d24f0713076112ac284de987e72fc5f6cc3795
|
|
|
|
|
|
|
|
|
|
This ensures TestFramework default parameters are set before setup is called. A
child class will therefore have access to defaults when overriding setup.
|
|
This allows a BitcoinTestFramework child class to set test parameters in an
overridden setup() rather than in an overridden set_test_params().
|
|
TestNode objects need to be removed during shutdown, as setup_nodes does not
remove previous TestNode objects from previous test runs during setup.
|
|
In order for BitcoinTestFramework to correctly restart after shutdown, the
previous logging handlers need to be removed, or else logging will continue in
the previous temp directory. "Flush" ensures buffers are emptied, and "close"
ensures file handler close logging file.
|
|
Setup and shutdown code now moved into dedicated methods. Test "success" is
added as a BitcoinTestFramework member, which can be accessed outside of main.
Argument parsing also moved into separate method and called from main.
|
|
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/connect_nodes_bi\(self.nodes,\s*(.),\s*/connect_nodes(self.nodes[\1], /g' $(git grep -l connect_nodes_bi)
sed -i --regexp-extended -e 's/connect_nodes_bi(,| )/connect_nodes\1/g' $(git grep -l connect_nodes_bi)
-END VERIFY SCRIPT-
|
|
|
|
Co-Authored-By: Jorge Timón <jtimon@jtimon.cc>
|
|
flake8 F841 lints, as of flake8 3.6.0
|
|
|
|
This allows tests which use randomness to be reproducibly run on failure.
|
|
a014373d81 QA: Avoid re-reading config.ini unnecessarily (Luke Dashjr)
Pull request description:
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
ACKs for commit a01437:
practicalswift:
utACK a014373d81534c9df4a2cbac0f74181e708da3cc
hebasto:
utACK a014373d81534c9df4a2cbac0f74181e708da3cc
Tree-SHA512: 36e3dda36cf4fae243feb1655da2891d1b78c86319be9bd9809c20054fa0cb75749370b05aa9d589a4dcab6322d8cdf4e874c5175144ed23ba63b2ed338538ca
|
|
BitcoinTestFramework.main already loads and stores config.ini on the object itself; just access that instead of re-reading the file to check for features
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
|
|
13782b8ba8 docs: add perf section to developer docs (James O'Beirne)
58180b5fd4 tests: add utility to easily profile node performance with perf (James O'Beirne)
Pull request description:
Adds a context manager to easily (and selectively) profile node performance during functional test execution using `perf`.
While writing some tests, I encountered some odd bitcoind slowness. I wrote up a utility (`TestNode.profile_with_perf`) that generates performance diagnostics for a node by running `perf` during the execution of a particular region of test code.
`perf` usage is detailed in the excellent (and sadly unmerged) https://github.com/bitcoin/bitcoin/pull/12649; all due props to @eklitzke.
### Example
```python
with node.profile_with_perf("large-msgs"):
for i in range(200):
node.p2p.send_message(some_large_msg)
node.p2p.sync_with_ping()
```
This generates a perf data file in the test node's datadir (`/tmp/testtxmpod0y/node0/node-0-TestName-large-msgs.perf.data`).
Running `perf report` generates nice output about where the node spent most of its time while running that part of the test:
```bash
$ perf report -i /tmp/testtxmpod0y/node0/node-0-TestName-large-msgs.perf.data --stdio \
| c++filt \
| less
# To display the perf.data header info, please use --header/--header-only options.
#
#
# Total Lost Samples: 0
#
# Samples: 135 of event 'cycles:pp'
# Event count (approx.): 1458205679493582
#
# Children Self Command Shared Object Symbol
# ........ ........ ............... ................... ........................................................................................................................................................................................................................................................................
#
70.14% 0.00% bitcoin-net bitcoind [.] CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
|
---CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
70.14% 0.00% bitcoin-net bitcoind [.] CNetMessage::readData(char const*, unsigned int)
|
---CNetMessage::readData(char const*, unsigned int)
CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
35.52% 0.00% bitcoin-net bitcoind [.] std::vector<char, zero_after_free_allocator<char> >::_M_fill_insert(__gnu_cxx::__normal_iterator<char*, std::vector<char, zero_after_free_allocator<char> > >, unsigned long, char const&)
|
---std::vector<char, zero_after_free_allocator<char> >::_M_fill_insert(__gnu_cxx::__normal_iterator<char*, std::vector<char, zero_after_free_allocator<char> > >, unsigned long, char const&)
CNetMessage::readData(char const*, unsigned int)
CNode::ReceiveMsgBytes(char const*, unsigned int, bool&)
...
```
Tree-SHA512: 9ac4ceaa88818d5eca00994e8e3c8ad42ae019550d6583972a0a4f7b0c4f61032e3d0c476b4ae58756bc5eb8f8015a19a7fc26c095bd588f31d49a37ed0c6b3e
|
|
Original tests by João Barbosa <joao.paulo.barbosa@gmail.com>
Additional contribution by John Newbery <john@johnnewbery.com>
|
|
Introduces `TestNode.profile_with_perf()` context manager which
samples node execution to produce profiling data.
Also introduces a test framework flag, `--perf`, which will run
perf on all nodes for the duration of a given test.
|
|
4999992c34 whitespace: Split ~300 char line into multiple ones (MarcoFalke)
fa71b38168 scripted-diff: Rename rpc_timewait to rpc_timeout (MarcoFalke)
fa3e5786d0 scripted-diff: Remove unused 'split' parameter to setup_network (MarcoFalke)
Pull request description:
This is a bugfix, since wallet_dump currently uses the wrong name:
https://github.com/bitcoin/bitcoin/blob/18857b4c4034af54e7ad3cbd78ff6f87f4f22567/test/functional/wallet_dump.py#L89-L92
Rename all to the same name with a scripted diff (and some unrelated cleanups).
Tree-SHA512: 338ddd20dae12e6cf7aa7adbcfb239cf648017a1572b373f8431fecb184bd2a65492846d81e75a023864d9e41c94afb53044c16b79651a5937d34a5a6b772f81
|
|
through test_runner.py
2474de0265 Fix running individually through test_runner.py, as suggested by @MarcoFalke (#14732) (Kristaps Kaupe)
Pull request description:
As suggested by @MarcoFalke. Resolves #14732.
Tree-SHA512: b4e400ba06075e218dbd97d0390845f1c55be42a2b6fd70513381318cfc2693473ba1d0f9d7f379a96939d1960b53801fad7c02e06bddc50c5a835ad024c37ef
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e 's/self.rpc_timewait/self.rpc_timeout/g' $(git grep -l self.rpc_timewait)
-END VERIFY SCRIPT-
|
|
fa4b8c90d3 test: add_nodes can only be called once after set_test_params (MarcoFalke)
faa831102a Revert "tests: Support calling add_nodes more than once" (MarcoFalke)
Pull request description:
Writing tests should be straightforward and with little side-effects as possible.
I don't see how this is needed and can not be achieved with `self.num_nodes` (and `self.extra_args` et al.)
Tree-SHA512: 83a67f2cba9d97e21d80847ff405a4633fcb0d5674486efa57ee1813e46efe8709ae0fb462b8339a01ebeca5c4f2d29ecb1807d648b8fd9ee8ce336b08d580a8
|
|
|
|
This reverts commit 98a1846b00d9c3076d6dcd96244fae6f923e26a0.
|
|
98a1846b00d9c3076d6dcd96244fae6f923e26a0 tests: Support calling add_nodes more than once (Steven Roose)
Pull request description:
Ran into this while writing [a multi-chain test for Elements](https://github.com/ElementsProject/elements/pull/458) where I call this method more than once.
Tree-SHA512: f2d698fcb560552aa5d81a4c3fbf40b7269b228b34d85a118291649ef83f8c0a30cd82a28d418237b55893bcecd538046b704e64a4d8a41f2c0aef8033dc83e5
|
|
28479f926f21f2a91bec5a06671c60e5b0c55532 qa: Test bitcond shutdown (João Barbosa)
8d3f46ec3938e2ba17654fecacd1d2629f9915fd http: Remove timeout to exit event loop (João Barbosa)
e98a9eede2fb48ff33a020acc888cbcd83e24bbf http: Remove unnecessary event_base_loopexit call (João Barbosa)
6b13580f4e3842c11abd9b8bee7255fb2472b6fe http: Unlisten sockets after all workers quit (João Barbosa)
18e968581697078c36a3c3818f8906cf134ccadd http: Send "Connection: close" header if shutdown is requested (João Barbosa)
02e1e4eff6cda0bfc24b455a7c1583394cbff6eb rpc: Add wait argument to stop (João Barbosa)
Pull request description:
Fixes #11777. Reverts #11006. Replaces #13501.
With this change the HTTP server will exit gracefully, meaning that all requests will finish processing and sending the response, even if this means to wait more than 2 seconds (current time allowed to exit the event loop).
Another small change is that connections are accepted even when the server is stopping, but HTTP requests are rejected. This can be improved later, especially if chunked replies are implemented.
Briefly, before this PR, this is the order or events when a request arrives (RPC `stop`):
1. `bufferevent_disable(..., EV_READ)`
2. `StartShutdown()`
3. `evhttp_del_accept_socket(...)`
4. `ThreadHTTP` terminates (event loop exits) because there are no active or pending events thanks to 1. and 3.
5. client doesn't get the response thanks to 4.
This can be verified by applying
```diff
// Event loop will exit after current HTTP requests have been handled, so
// this reply will get back to the client.
StartShutdown();
+ MilliSleep(2000);
return "Bitcoin server stopping";
}
```
and checking the log output:
```
Received a POST request for / from 127.0.0.1:62443
ThreadRPCServer method=stop user=__cookie__
Interrupting HTTP server
** Exited http event loop
Interrupting HTTP RPC server
Interrupting RPC
tor: Thread interrupt
Shutdown: In progress...
torcontrol thread exit
Stopping HTTP RPC server
addcon thread exit
opencon thread exit
Unregistering HTTP handler for / (exactmatch 1)
Unregistering HTTP handler for /wallet/ (exactmatch 0)
Stopping RPC
RPC stopped.
Stopping HTTP server
Waiting for HTTP worker threads to exit
msghand thread exit
net thread exit
... sleep 2 seconds ...
Waiting for HTTP event thread to exit
Stopped HTTP server
```
For this reason point 3. is moved right after all HTTP workers quit. In that moment HTTP replies are queued in the event loop which keeps spinning util all connections are closed. In order to trigger the server side close with keep alive connections (implicit in HTTP/1.1) the header `Connection: close` is sent if shutdown was requested. This can be tested by
```
bitcoind -regtest
nc localhost 18443
POST / HTTP/1.1
Authorization: Basic ...
Content-Type: application/json
Content-Length: 44
{"jsonrpc": "2.0","method":"stop","id":123}
```
Summing up, this PR:
- removes explicit event loop exit — event loop exits once there are no active or pending events
- changes the moment the listening sockets are removed — explained above
- sends header `Connection: close` on active requests when shutdown was requested which is relevant when it's a persistent connection (default in HTTP 1.1) — libevent is aware of this header and closes the connection gracefully
- removes event loop explicit break after 2 seconds timeout
Tree-SHA512: 4dac1e86abe388697c1e2dedbf31fb36a394cfafe5e64eadbf6ed01d829542785a8c3b91d1ab680d3f03f912d14fc87176428041141441d25dcb6c98a1e069d8
|
|
|
|
|
|
|
|
(#14732)
|
|
This requires a small changes to a few tests, but means that
deterministic addresses will always be imported (unless setup_nodes
behaviour is explicitly overridden).
|
|
|
|
a2a04a5abb Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled (Luke Dashjr)
92af71cea9 configure: Make it possible to build only one of bitcoin-cli or bitcoin-tx (Luke Dashjr)
Pull request description:
Includes #5618 (which the reasons for rejecting no longer hold true)
Tree-SHA512: f30a8e4a2f70166b7cabef77c4674163b3a9da14c6a547d34f00d1056a19bf4d23e22851eea726fad2afc8735d5473ae91122c770b65ac3886663dc20e2c5b70
|
|
skipping .Addreses #14216. Changed get_deterministic_priv_key() to a
named tuple
|
|
|
|
|
|
|
|
|
