This resolves a bug introduced in
66aa1d58a1 where, if when responding
to a series of transaction requests in a getdata we hit the send
buffer limit and set fPauseSend, we will skip one transaction per
call to ProcessGetData.
Bug found by Cory Fields (@theuni).
669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar)
e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar)
0975406 Correct mempool mapTx comment (Suhas Daftuar)
Pull request description:
Following up on #12127 and #12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of `queryHashes()` and a small information leak when prioritizing transactions.
Left undone is nuking queryHashes altogether; that would require changing the behavior of the `getrawmempool` rpc call, which I think I might be in favor of doing, but wanted to save for its own PR.
Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162
2f960b5 [wallet] Indent only change of CWallet::AvailableCoins (João Barbosa)
1beea7a [wallet] Make CWallet::ListCoins atomic (João Barbosa)
Pull request description:
Fix a potencial race in `CWallet::ListCoins`.
Replaces `cs_main` and `cs_wallet` locks by assertions in `CWallet::AvailableCoins`.
Tree-SHA512: 09109f44a08b4b53f7605d950ab506d3f748490ab9aed474aa200e93f7b0b9f96f9bf60abe1c5f658240fd13d9e3267c0dd43fd3c1695d82384198ce1da8109f
fa795cf wallet: Disallow abandon of conflicted txes (MarcoFalke)
Pull request description:
Abandon transactions that are already conflicted is a noop, so don't try and return false/throw instead.
Tree-SHA512: fd2af4149bd2323f7f31fe18685c763790b8589319b4e467b464ab456d5e8971501ab16d124e57a22693666b06ae433ac3e59f0fd6dfbd2be2c6cae8be5bcbd8
db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery)
cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery)
ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery)
d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery)
c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery)
a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery)
a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery)
d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery)
Pull request description:
There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched.
- `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031)
- the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858)
- providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415)
- The return format from `addmultisigaddress` has changed (#11415)
`getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up)
Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124
5bdbbdc Refactor HaveKeys to early return on false result (João Barbosa)
Pull request description:
This consists in a trivial change where the return type of `HaveKeys()` is now `bool` meaning that it returns whether all keys are in the keystore, and early returns when one isn't.
Tree-SHA512: 03e35ea8486404b84884b49f6905c9f4fc161a3eeef080b06482d77985d5242a2bdd57a34b8d16abe19ee8c6cfa3e6fbcb935c73197d53f4cd468a2c7c0b889b
45eea40 Bech32 addresses in dumpwallet (fivepiece)
Pull request description:
Output bech32 addresses in dumpwallet if address type is not as legacy
Tree-SHA512: f6b6f788293779fe6339b94d9b792180e1d1dcb9c8e826caef8693557e1710213ba57891981c17505ace8d67b407eeca6fd9a8825757dd292cca2aa12575d15c
02fc886 Add braces to meet code style on line-after-the-one-changed. (Matt Corallo)
85aa839 Hold mempool.cs for the duration of ATMP. (Matt Corallo)
Pull request description:
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in #12273
Tree-SHA512: 29464b9ca3890010ae13b7dc1c53487cc2bc9c3cf3d32a14cb09c8aa33848f57959d8991ea096beebcfb72f062e4e1962f104aefe4252c7db87633bbfe4ab317
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
1e5d14b qt: Clarify some comments (Wladimir J. van der Laan)
f5a4c3d qt: Make sure splash screen is freed on AppInitMain fail (Wladimir J. van der Laan)
Pull request description:
The `splashFinished` event was never sent if AppInitMain fails, causing the splash screen to stick around, causing problems later.
This bug has existed for a while but is now trigging potential crashed because the splash screen subscribes to wallet events.
Meant to fix#12372.
Tree-SHA512: 192a7e3a528015e771d7860dd95fd7b772292fd8064abf2a3cf3a8ea0d375cd43a6e8ed37ca1a38962fe1410c934599e557adf6a8ef9d87ec7f61b6e5fd8db7e
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke)
Pull request description:
The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs:
e5033a5c9b/src/corelib/thread/qthread.cpp (L412-L415)
Potential fix for https://github.com/bitcoin/bitcoin/issues/12372#issuecomment-363642332
This reverts #11831 for now and hopefully restores the previous behaviour.
Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
dd2de47 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo)
1c9394a Fix fast-shutdown hang on ThreadImport+GenesisWait (Matt Corallo)
Pull request description:
The second commit is a much simpler alternative fix for the issue fixed in #12349. To test I made ShutdownRequested() always StartShutdown() after a certain number of calls, which turned up one other hang, fixed in the first commit.
Tree-SHA512: 86bde6ac4b8b4e2cb99fff87dafeed02c0d9514acee6d94455637fb2da9ffc274b5ad31b0a6b9f5bd7b700ae35395f28ddb14ffc65ddda3619aa28df28a5607d
9ad6746ccd Use static_cast instead of C-style casts for non-fundamental types (practicalswift)
Pull request description:
A C-style cast is equivalent to try casting in the following order:
1. `const_cast(...)`
2. `static_cast(...)`
3. `const_cast(static_cast(...))`
4. `reinterpret_cast(...)`
5. `const_cast(reinterpret_cast(...))`
By using `static_cast<T>(...)` explicitly we avoid the possibility of an unintentional and dangerous `reinterpret_cast`. Furthermore `static_cast<T>(...)` allows for easier grepping of casts.
For a more thorough discussion, see ["ES.49: If you must use a cast, use a named cast"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: bd6349b7ea157da93a47b8cf238932af5dff84731374ccfd69b9f732fabdad1f9b1cdfca67497040f14eaa85346391404f4c0495e22c467f26ca883cd2de4d3c
The `splashFinished` event was never sent if AppInitMain fails,
causing the splash screen to stick around, causing problems
later.
This bug has existed for a while but is now trigging potential crashed
because the splash screen subscribes to wallet events.
Meant to fix#12372.
Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>
The HTTP worker thread counter, as well as the RAII object that was used
to maintain it, is unused now, so can be removed.
Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>
If the ShutdownRequested() check at the top of ActivateBestChain()
returns false during initial genesis block load we will fail an
assertion in UTXO DB flush as the best block hash IsNull(). To work
around this, we move the check until after one round of
ActivateBestChainStep(), ensuring the genesis block gets connected.
If the user somehow manages to get into ShutdownRequested before
ThreadImport gets to ActivateBestChain() we may hang waiting on
condvar_GenesisWait forever. A simple wait_for and
ShutdownRequested resolves this case.
This function, which waits for all threads to exit, is no longer needed
now that threads are joined instead.
Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>
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.
Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in #12273
bdb3231 Implements a virtual destructor on the BaseRequestHandler class. (251)
Pull request description:
Granted that there is no undefined behavior in the current implementation, this PR implements a virtual destructor on the BaseRequestHandler class to protect against undefined behavior in the event that an object of a potential future derived BaseRequestHandler class with a destructor is destroyed through a pointer to this base class.
This PR also fixes "_warning: delete called on 'BaseRequestHandler' that is abstract but has non-virtual destructor [-Wdelete-non-virtual-dtor]_" warnings in environments where the project is built with the `-Wsystem-headers` flag; or environments where the `-Wdelete-non-virtual-dtor` diagnostics flag fires from system headers.
Tree-SHA512: 3c3b0797a8dbce8d8c5b244709e8bca41c4e28d5ba554a974bf7fc9128413e1098c457a00e51b21154ce6c11ce5da3071626e71d593b2550d0020bc589406eed
5460460 Add AbsPathForConfigVal to consolidate datadir prefixing for path args (James O'Beirne)
a1e1305 Clarify help messages for path args to mention datadir prefix (James O'Beirne)
Pull request description:
Change `-conf`'s help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but instead loading the `bitcoin.conf` file in ~/.bitcoin datadir.
### Edit
This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization.
Tree-SHA512: be4fc0595fbeba33d17af08f59898af45e76a44f00719ea0282403b155ac6755584604fab765250a3aa14ed6991882c4d1ccbe601184362c5ba97c886bdda344
d3a185a net: Move misbehaving logging to net logging category (Wladimir J. van der Laan)
Pull request description:
This moves the error messages for misbehavior (when available) into the line that reports the misbehavior, as well as moves the logging to the `net` category.
This is a continuation of #11583 and avoids serious-looking errors due to misbehaving peers. As it is impossible to correlate the `peer=X` numbers to specific incoming connections now without enabling the `net` category, it doesn't really help to see these messages by default.
To do this, Misbehaving() gains an optional `message` argument.
E.g. change:
2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED
2018-01-18 16:02:27 ERROR: non-continuous headers sequence
to
2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED: non-continuous headers sequence
When there is a category for "important" net messages (see #12219 ), we should move it there.
Tree-SHA512: 51c97e9a649bf5409f2fd4625fa1243a036e9c9de6037bb064244207408c2e0eb025e3af80866df673cdc006b8f35dc4078d074033f0d4c6a73bbb03949a269f
c887f87 Extend #11583 to include the most common message generated by non-contributing peers (port scanners?) 37% of the log default log entries for a node that has been up for ~24hrs was "version handshake timeout..." (Clem Taylor)
Pull request description:
37% of the default log entries for a node that has been up for ~24hrs was "version handshake timeout..."
Tree-SHA512: dceeee5d55a9ff7570174aeb63faac9beda239087220522adefef7ed11e0eeffa008ca28726011247c8834c1a222d37817baf895635ab874a95ebc435959070e
Change `-conf`'s and others' help messages to indicate that relative path
values will be prefixed by the datadir path. This behavior is confusing when
attempting to specify a configuration file in the current directory with
`-conf=bitcoin.conf`, but loading the `bitcoin.conf` file in ~/.bitcoin
datadir.
c409b1adac [rpc] Reduce scope of cs_main and cs_wallet locks in listtransactions (João Barbosa)
Pull request description:
Trivial change, no behaviour change.
Benchmark done as follow:
- run with `-regtest`
- wallet with 5000 transactions
- measured the time spent with the lock and the total time
- times are an average of 100 `listtransactions --count=...` calls
| `--count` | lock (ms) | total (ms) | saving |
|--:|--:|--:|--:|
| 10 | 0.2230 | 0.2510 | 11% |
| 100 | 2.5150 | 2.8690 | 12% |
| 1000 | 20.0320 | 23.3490 | 14% |
| 10000 | 105.2070 | 125.5310 | 16% |
Tree-SHA512: ebedfeeb4c8ad75c89128e53cae976a82967dbb5ffd129da0f7204ccf9c3c15070b3d509f3767bebd745512e410200cc546147c836e82409f95fc9b8d14fc3ed
d3e467520f Properly alphabetize output of CLI --help option. (murrayn)
Pull request description:
The --help output of bitcoind, bitcoin-cli, bitcoin-tx, qt/bitcoin-qt, et al. is only about 90% alphabetized by option, which is kind of sloppy and occasionally misleading. This change (mostly) organizes the output alphabetically.
Tree-SHA512: 3029900dbe99f03397c1cbdb5e4ac09a13bc99bafe73c6855097206e4cdd9ad70d0b5cedb5e1e520005c3e9ef1c4cd32bb9d8c98ce6918d8434fec8bf06e56c8
7444149 Document method for reviewers to verify chainTxData (John Newbery)
Pull request description:
This commit adds the final block hash of the window to getchaintxstats
and documents how reviewers can verify changes to chainTxData.
Tree-SHA512: d16abb5f47d058e52660f4d495f1e453205b1b83716d7c810ff62a70338db721386c1808ec1fc8468f514e4d80cc58e3c96eeb3184cbbcb1d07830fa5e53f342
660f5f1 net: don't retry failed oneshot connections forever (Cory Fields)
Pull request description:
As introduced by (my suggestion, sorry, in) #11512, failed dns resolves end up as oneshots. But failed oneshots are re-added as oneshots, so we need to make sure that we're not queuing these up forever after failed resolves.
Rather than trying to differentiate, I think we should just not re-add failed oneshots and be done with it.
Maybe @sipa can shed a light on what the original intention was.
Tree-SHA512: 2dfe35dabfb6354c315cf6f8ae42971765d36575e685662caae7ed8f9dea9472c6fb1fd5e62ec35301550b74b6613a54265e90fca2a6618544f78dacaac4d4fd
6558f8acc [gui] Defer coin control instancing (João Barbosa)
Pull request description:
Defer the GUI coin control instancing so that argument processing
is taken into account for the default coin control values.
Fixes#12312
Tree-SHA512: ecda28b94f4709319e9484b01afe763c7c3569097d2afb89db79da8a195c46d20ea77166df7edce0c8ab77627b295def01c072148714503436d27675d5e75d99
