* Disable move ctor/operator for CKeyHolder
Also fixes these warnings:
```
In file included from dsnotificationinterface.cpp:12:
In file included from ./privatesend/privatesend-client.h:8:
./privatesend/privatesend-util.h:18:5: warning: explicitly defaulted move constructor is implicitly deleted [-Wdefaulted-function-deleted]
CKeyHolder(CKeyHolder&&) = default;
^
./privatesend/privatesend-util.h:13:17: note: move constructor of 'CKeyHolder' is implicitly deleted because field 'reserveKey' has a deleted move constructor
CReserveKey reserveKey;
^
./wallet/wallet.h:1282:5: note: 'CReserveKey' has been explicitly marked deleted here
CReserveKey(const CReserveKey&) = delete;
^
In file included from dsnotificationinterface.cpp:12:
In file included from ./privatesend/privatesend-client.h:8:
./privatesend/privatesend-util.h:19:17: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
CKeyHolder& operator=(CKeyHolder&&) = default;
^
./privatesend/privatesend-util.h:13:17: note: move assignment operator of 'CKeyHolder' is implicitly deleted because field 'reserveKey' has a deleted move assignment operator
CReserveKey reserveKey;
^
./wallet/wallet.h:1283:18: note: 'operator=' has been explicitly marked deleted here
CReserveKey& operator=(const CReserveKey&) = delete;
^
2 warnings generated.
```
* Slightly refactor `CKeyHolderStorage::AddKey()` to clarify that it's ptr and not the object itself that we are moving
a0daea459cf4812fbdda9a7ead131a73f5856c09 [build] depends macOS: point --sysroot to SDK (Sjors Provoost)
Pull request description:
Fixes errors like `fatal error: 'unistd.h' file not found` when building depends on macOS.
Replaces #14352 (which doesn't work on Catalina).
ACKs for top commit:
jonasschnelli:
utACK a0daea459cf4812fbdda9a7ead131a73f5856c09
Tree-SHA512: 995b1e1e84e635b32d1d4038bc63730c94a7c318b7240f6d62825977e5c97fe52c5aa5a0f39070beb0df8271dd294b36d6b5cf7f09ad07494fb15d5bd4d77f68
* [Qt] make sure transaction table entry gets updated after bump
* Remove unnecessary tracking of IS lock count
* Track lockedByChainLocks in TransactionRecord
* Only update record when the TX was not ChainLocked before
* Emit dataChanged for CT_UPDATED transactions
* Use plain seconds since epoch comparison in TransactionFilterProxy::filterAcceptsRow
The QDateTime::operator< calls inside TransactionFilterProxy::filterAcceptsRow
turned out to be the slowest part in the UI when many TXs are inside
the wallet. DateRoleInt allows us to request the plain seconds since epoch
which we then use to compare against dateFrom/dateTo, which are also both
stored as seconds since epoch now.
* Don't invoke updateConfirmations directly and let pollBalanceChanged handle it
* Implement AddressTableModel::labelForDestination
This one avoids converting from string to CBitcoinAddress and calling
.Get() on the result.
* Also store CBitcoinAddress object and CTxDestination in TransactionRecord
This avoids frequent and slow conversion
* Use labelForDestination when possible
This avoids unnecessary conversions
* Don't set fForceCheckBalanceChanged to true when IS lock is received
We already do this through updateTransaction(), which is also called when
an IS lock is received for one of our own TXs.
* Only update lockedByChainLocks and lockedByInstantSend when a change is possible
lockedByChainLocks can never get back to false, so no need to re-check it.
Same with lockedByInstantSend, except when a ChainLock overrides it.
* Hold and update label in TransactionRecord
Instead of looking it up in data()
* Review suggestions
* Use proper columns in dataChanged call in updateAddressBook
a0daea459cf4812fbdda9a7ead131a73f5856c09 [build] depends macOS: point --sysroot to SDK (Sjors Provoost)
Pull request description:
Fixes errors like `fatal error: 'unistd.h' file not found` when building depends on macOS.
Replaces #14352 (which doesn't work on Catalina).
ACKs for top commit:
jonasschnelli:
utACK a0daea459cf4812fbdda9a7ead131a73f5856c09
Tree-SHA512: 995b1e1e84e635b32d1d4038bc63730c94a7c318b7240f6d62825977e5c97fe52c5aa5a0f39070beb0df8271dd294b36d6b5cf7f09ad07494fb15d5bd4d77f68
62c304ea48 tests: Allow closed http server in assert_start_raises_init_error (Chun Kuan Lee)
Pull request description:
The rpc handler may be unregistered when http server haven't been closed yet. So it may be allowable to get -342 `non-JSON HTTP response with \'%i %s\' from server` (503 Service Unavailable)
See https://ci.appveyor.com/project/DrahtBot/bitcoin/build/master.2001. It shows "Rejecting request while shutting down" between "RPC stopped" and "Stopped HTTP server"
Tree-SHA512: e1f50ab9096cf23494ccc2850c01516c4a75f112c99108759157b22fce2011682a4b88216f206702f6a56e960182c00098053ad75f13aa0eafe27046adae63da
* Make parent cache download quiet
This spammed logs quite a bit
* Add "-p" to "mkdir $CI_PROJECT_DIR/cache-artifact"
Otherwise the after_script is silently aborted when the dir already exists.
* Extract parent cache artifact in own directory and later delete it
Otherwise we end up having "testlogs" (and maybe other stuff) in the
project directory, which causes future failures.
Turned out that this causes SelectCoinsMinConf to (unnecessary) lean towards selecting mempool txes which can cause "too long mempool chain" error even when there are more funds to spend.
* Fix cache usage in SelectCoinsGroupedByAddresses
* Reset cache flags in SelectCoinsGroupedByAddresses when a block is (dis)connected
* MakeCollateralAmounts should call SelectCoinsGroupedByAddresses with a limited number of inputs
This avoids sorting before looping through it to figure out what to
request. The assumption that sorting would be cheap when vecAskFor is
already mostly sorted (only unsorted at the end) turned out to be false.
In reality, ~50% of CPU time was consumed by the sort when a lot of traffic
(thousands of TXs) happen.
* More/better logging for InstantSend
* Implement CRecoveredSigsDb::TruncateRecoveredSig
* Truncate recovered sigs for ISLOCKs instead of completely removing them
This makes AlreadyHave() return true even when the recovered sig is deleted
locally. This avoids re-requesting and re-processing of old recovered sigs.
* Also truncate recovered sigs for freshly received ISLOCKs
* Fix comment
* Add .gitlab-ci.yml
* Use | instead of > for multiline commands
This honor new-lines and makes ; unnecessary
* Use ubuntu:bionic as base image
* Move cache initialization before apt-get installs
* Cache apt packages
* Move installation of wget and unzip up as we need it for the cache
* Prevent apt from deleting caches
* Collect test logs into artifact
* Make combine_logs.py always look for the template in the correct dir
* Move final cache stuff into after_script
* Reintroduce PYTHON_DEBUG=1, but only for .travis.yml
* Install jinja2 in Travis builder image
* Enable ChainLocks after quorums have been created
Creating 4 quorums causes a lot of blocks to be created and signed by
ChainLocks, which then causes timeouts later.
* Increase timeout in wallet-dump.py test
The first dumpwallet is quite slow sometimes, which then makes the
later called dumpwallet throw a wallet locked exception.
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
11e0151 http: Remove numThreads and ThreadCounter (Wladimir J. van der Laan)
f946654 http: Remove WaitExit from WorkQueue (Wladimir J. van der Laan)
b1c2370 http: Join worker threads before deleting work queue (Wladimir J. van der Laan)
Pull request description:
This prevents a potential race condition if control flow ends up in
`ShutdownHTTPServer` before the thread gets to `queue->Run()`,
deleting the work queue while workers are still going to use it.
Meant to fix#12362.
Tree-SHA512: 8108514aeee5b2067a3736ed028014b580d1cbf8530ac7682b8a23070133dfa1ca21db4358c9158ea57e8811e0551395b6cb769887876b9cfce067ee968d0642
* Remove crownium files
* Remove trad theme files
* Remove drkblue theme files
* Remove light-retro theme files
* Remove old themes from optimize-pngs script
* Remove refs to old themes in Makefile.qt
* Remove more old theme file references
* Remove old themes from options dialog
* No need to care about themes for images and icons anymore
* Bring `trad` back
* Drop remaining `drkblue` references
Rename files that are actually used and drop no longer needed ones
* Use `wait_until` in sporks.py
* Drop unused spork functions in `dip3-deterministicmns.py`
* Use `wait_until` in multikeysporks.py
* scripted-diff: Rename `*_test_spork_state` to `*_test_spork_value` in multikeysporks.py to better match actual functionality
-BEGIN VERIFY SCRIPT-
sed -i 's/_test_spork_state/_test_spork_value/g' test/functional/multikeysporks.py
-END VERIFY SCRIPT-
* Introduce getprivatesendinfo and deprecate getpoolinfo
* s/ToJson/GetJsonInfo/
* s/queues/queue_size/
* Add TODO to explain `denomination` string convertion
* s/entries/entries_count/
* Use `CURRENCY_UNIT`
* Who needs safety belts after all
* Always check for expired queues on masternodes
* Check if a queue is too old or too far into the future
Instead of only checking that it's to old
* Check that no masternode can spam us with dsqs regardless of dsq readiness
* Drop `get_mnsync_status`, `wait_to_sync` and `sync_masternodes` and introduce `force_finish_mnsync` for MNs only
* Use `force_finish_mnsync` from util.py in dip3-deterministicmns.py and drop local unused functions
Also move the call, `force_finish_mnsync` should be called before `connect_nodes_bi`
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.
4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón)
3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón)
832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón)
3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón)
Pull request description:
...is created by individual transactions to 2 places (but call only once in each):
- ConnectBlock ( before calculated fees per txs twice )
- AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated
fees per tx one extra time )
Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2)
For more motivation:
~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~https://github.com/jtimon/bitcoin/compare/0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments
EDIT: partially replaces #6445
Near-Bugfix as pointed out in https://github.com/bitcoin/bitcoin/pull/8498#discussion_r124346132
Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346
e97b113b04 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery)
2b7064eda7 [tests] Fix flake8 warnings in invalidblockrequest (John Newbery)
54b8c580b7 [test] Fix nits leftover from 11771 (Conor Scott)
Pull request description:
Builds on #11771. Please review that PR first
Next step in #10603.
- first commit tidies up invalidblockrequest.py
- second commit removes usage of ComparisonTestFramework
Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456
95e2e9a [tests] Change invalidtxrequest to use BitcoinTestFramework (John Newbery)
359d067 [tests] Fix flake8 warnings in invalidtxrequest (John Newbery)
c32cf9f [tests] Add P2PDataStore class (John Newbery)
cc046f6 [tests] Reduce NodeConn connection logging from info to debug (John Newbery)
Pull request description:
Next step in #10603
- first commit changes log level for an internal log from INFO to DEBUG. (Not really related, but I started finding the INFO level logging annoying when debuging test failures)
- second commit introduces a `P2PStub` class - a subclass of `NodeConnCB` which has its own block and tx store and responds appropriately to getdata requests. Not all the functionality is used in `invalidtxrequest.py`, but will be used in `invalidblockrequest.py` and `p2p-fullblocktest` when those are changed to use `BitcoinTestFramework`
- third commit tidies up `invalidtxrequest.py`
- fourth commit removes usage of `ComparisonTestFramework`
Tree-SHA512: f3085c73c15d6ce894e401490bce8a7fa7cf52b0c9d135ff7e351f1f6f517c99accab8588fcdc443f39ea8315329aaabd66b2baa32499df5a774737882030373
5c8ff26 [tests] Add NetworkThread assertions (John Newbery)
34e08b3 [tests] Fix network threading in functional tests (John Newbery)
74e64f2 [tests] Use network_thread_start() in tests. (John Newbery)
5fc6e71 [tests] Add network_thread_ utility functions. (John Newbery)
Pull request description:
Add assert that only one NetworkThread exists at any time in functional tests, and fix cases where that wasn't true.
fixes#11776
Tree-SHA512: fe5d1c59005f94bf66e11bb23ccf274b1cd9913741b56ea11dbcd21db4cc0b53b4413c0c4c16dbcd6ac611adad5e5cc2baaa39720598ce7b6393889945d06298
faaa7db qa: Only allow disconnecting all NodeConns (MarcoFalke)
Pull request description:
Disconnecting the connection with `index=0` makes no sense when there are more than one connections, as the list "rotates around" and populates index 0 after `del`.
Just disconnect all NodeConns in any case.
Tree-SHA512: e5cf540823fccb31634b5a11501f54222be89862e80ccafc28bc06726480f8d2153b8c1b6f859fa6a6d087876251d48a6c6035bccdaaf16831e300bc17ff613d
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
86b47fa741408b061ab0bda784b8678bfd7dfa88 speed up Unserialize_impl for prevector (Akio Nakamura)
Pull request description:
The unserializer for prevector uses `resize()` for reserve the area, but it's prefer to use `reserve()` because `resize()` have overhead to call its constructor many times.
However, `reserve()` does not change the value of `_size` (a private member of prevector).
This PR make the logic of read from stream to callback function, and prevector handles initilizing new values with that call-back and ajust the value of `_size`.
The changes are as follows:
1. prevector.h
Add a public member function named 'append'.
This function has 2 params, number of elemenst to append and call-back function that initilizing new appended values.
2. serialize.h
In the following two function:
- `Unserialize_impl(Stream& is, prevector<N, T>& v, const unsigned char&)`
- `Unserialize_impl(Stream& is, prevector<N, T>& v, const V&)`
Make a callback function from each original logic of reading values from stream, and call prevector's `append()`.
3. test/prevector_tests.cpp
Add a test for `append()`.
## A benchmark result is following:
[Machine]
MacBook Pro (macOS 10.13.3/i7 2.2GHz/mem 16GB/SSD)
[result]
DeserializeAndCheckBlockTest => 22% faster
DeserializeBlockTest => 29% faster
[before PR]
# Benchmark, evals, iterations, total, min, max, median
DeserializeAndCheckBlockTest, 60, 160, 94.4901, 0.0094644, 0.0104715, 0.0098339
DeserializeBlockTest, 60, 130, 65.0964, 0.00800362, 0.00895134, 0.00824187
[After PR]
# Benchmark, evals, iterations, total, min, max, median
DeserializeAndCheckBlockTest, 60, 160, 77.1597, 0.00767013, 0.00858959, 0.00805757
DeserializeBlockTest, 60, 130, 49.9443, 0.00613926, 0.00691187, 0.00635527
ACKs for top commit:
laanwj:
utACK 86b47fa741408b061ab0bda784b8678bfd7dfa88
Tree-SHA512: 62ea121ccd45a306fefc67485a1b03a853435af762607dae2426a87b15a3033d802c8556e1923727ddd1023a1837d0e5f6720c2c77b38196907e750e15fbb902
5aad635 Use memset() to optimize prevector::resize() (Evan Klitzke)
e46be25 Reduce redundant code of prevector and speed it up (Akio Nakamura)
f0e7aa7 Add new prevector benchmarks. (Evan Klitzke)
Pull request description:
This branch optimizes various `prevector` operations, especially resizing vectors. While profiling the `loadblk` thread I noticed that a lot of time was being spent in `prevector::resize()` which led to this work. I have some data here indicating that it takes up **37%** of the time in `ReadBlockFromDisk()`: https://monad.io/readblockfromdisk.svg
This branch improves things significantly. For trivial types, the new results for the prevector benchmark are:
* `PrevectorClearTrivial` which tests `prevector::clear()` becomes 24.6x faster
* `PrevectorDestructorTrivial` which tests `prevector::~prevector()` becomes 20.5x faster
* `PrevectorResizeTrivial` which tests `prevector::resize()` becomes 20.3x faster
Note that in practice it looks like the prevector is only used to contain `unsigned char` types, which is a trivial type. The benchmarks are testing a bit of an extreme case, but the changes here are motivated by the profiling data for `ReadBlockFromDisk()` I linked to above.
The pull request here consists of a series of three commits:
* The first adds new benchmarks but does not change the prevector code.
* The second is from @AkioNak , and merges some prevector optimizations he submitted in #11988
* The third optimizes `prevector::resize()` to use `memset()` when the prevector contains trivially constructible types
Tree-SHA512: 28f7cbb91a19f9f43b6a5942781d7eb2e3197389186b666f086b69df12bee37773140f765426d715bfb8ebff79cb27a5f1206d0325b54b4aa65598b50fb18368
* Add timeout params to wait_for*_chainlock methods
* Give chainlocks more time in specific case
* Add logs to llmq-chainlock.py
* Replace wait_for_chainlocked_tip_all_nodes with wait_for_chainlocked_block_all_nodes
wait_for_chainlocked_tip_all_nodes did wait for the tip of each individual
node, which would not necessarily be the same. We should only allow to
explicitly specify which block to wait for.
* Get rid of wait_for_chainlocked_tip
Same as with wait_for_chainlocked_tip_all_nodes
This speeds up assumevalid.py from 22s to 7s on my machine. On travis, this
should be an improvement of a few minutes. Without this, Travis actually
fails due to block download timeouts.
