22b4aae02 [arith_uint256] Avoid unnecessary this-copy using prefix operator (Karl-Johan Alm)
Pull request description:
I noticed while profiling a related project that `operator-()` actually calls the `base_uint` constructor, which is because the postfix operator version of `operator++` (used in `operator-()`) creates a copy of `this` and returns it.
Tree-SHA512: d9a2665caa3d93f064cdeaf1c6fada101b9943bb53d93ccac6d9a0edac20279d2e921349e30239039c71e0a9629e45c29ec9f10d8d7499e936cdba6cb7c3c3eb
b7cd08b71 Add documentation to PeerLogicValidation interface and related functions (James O'Beirne)
Pull request description:
Adds docs for PeerLogicValidation's public interface and two related functions.
Tree-SHA512: b4c2f47e9baa9396d2b6faf3792e46b371c50cd91b9ac890f263f4d14eb24a71e7b40ceb4cbb41e254f5008eff357f417b842618e7ebece9039802ab2a5dd728
e68172ed9 Add test-before-evict discipline to addrman (Ethan Heilman)
Pull request description:
This change implement countermeasures 3 (test-before-evict) suggested in our paper: ["Eclipse Attacks on Bitcoin’s Peer-to-Peer Network"](http://cs-people.bu.edu/heilman/eclipse/).
# Design:
A collision occurs when an address, addr1, is being moved to the tried table from the new table, but maps to a position in the tried table which already contains an address (addr2). The current behavior is that addr1 would evict addr2 from the tried table.
This change ensures that during a collision, addr1 is not inserted into tried but instead inserted into a buffer (setTriedCollisions). The to-be-evicted address, addr2, is then tested by [a feeler connection](https://github.com/bitcoin/bitcoin/pull/8282). If addr2 is found to be online, we remove addr1 from the buffer and addr2 is not evicted, on the other hand if addr2 is found be offline it is replaced by addr1.
An additional small advantage of this change is that, as no more than ten addresses can be in the test buffer at once, and addresses are only cleared one at a time from the test buffer (at 2 minute intervals), thus an attacker is forced to wait at least two minutes to insert a new address into tried after filling up the test buffer. This rate limits an attacker attempting to launch an eclipse attack.
# Risk mitigation:
- To prevent this functionality from being used as a DoS vector, we limit the number of addresses which are to be tested to ten. If we have more than ten addresses to test, we drop new addresses being added to tried if they would evict an address. Since the feeler thread only creates one new connection every 2 minutes the additional network overhead is limited.
- An address in tried gains immunity from tests for 4 hours after it has been tested or successfully connected to.
# Tests:
This change includes additional addrman unittests which test this behavior.
I ran an instance of this change with a much smaller tried table (2 buckets of 64 addresses) so that collisions were much more likely and observed evictions.
```
2016-10-27 07:20:26 Swapping 208.12.64.252:8333 for 68.62.95.247:8333 in tried table
2016-10-27 07:20:26 Moving 208.12.64.252:8333 to tried
```
I documented tests we ran against similar earlier versions of this change in #6355.
# Security Benefit
This is was originally posted in PR #8282 see [this comment for full details](https://github.com/bitcoin/bitcoin/pull/8282#issuecomment-237255215).
To determine the security benefit of these larger numbers of IPs in the tried table I modeled the attack presented in [Eclipse Attacks on Bitcoin’s Peer-to-Peer Network](https://eprint.iacr.org/2015/263).
**Default node:** 595 attacker IPs for ~50% attack success.
**Default node + test-before-evict:** 620 attacker IPs for ~50% attack success.
**Feeler node:** 5540 attacker IPs for ~50% attack success.
**Feeler node + test-before-evict:** 8600 attacker IPs for ~50% attack success.
The node running feeler connections has 10 times as many online IP addresses in its tried table making an attack 10 times harder (i.e. requiring the an attacker require 10 times as many IP addresses in different /16s). Adding test-before-evict increases resistance of the node by an additional 3000 attacker IP addresses.
Below I graph the attack over even greater attacker resources (i.e. more attacker controled IP addresses). Note that test-before-evict maintains some security far longer even against an attacker with 50,000 IPs. If this node had a larger tried table test-before-evict could greatly boost a nodes resistance to eclipse attacks.
Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
6fbc0986f gui: Show messages as text not html (Wladimir J. van der Laan)
Pull request description:
Currently, error messages (such as InitError) are displayed as-is, which means Qt does auto detection on the format.
This means that it's possible to inject HTML from the command line though e.g. specifying a wallet name with HTML in it. This isn't a direct security risk because fetching content from internet is
disabled (and as far as I know we never report strings received from the network this way). However, it can be confusing.
So explicitly force the format as text.
Tree-SHA512: 96c9196f20552544b862071bca61817ef03653019cc3548023d435f3a9c48b6cd501fab3246783cb0be68c8c7bb1b865913d92070a7c4e84e82c6577709f0934
cfaac2a60 Add build support for 'gprof' profiling. (murrayn)
Pull request description:
Support for profiling build: `./configure --enable-profiling`
Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
937bf4335 Use std:🧵:hardware_concurrency, instead of Boost, to determine available cores (fanquake)
Pull request description:
Following discussion on IRC about replacing Boost usage for detecting available system cores, I've opened this to collect some benchmarks + further discussion.
The current method for detecting available cores was introduced in #6361.
Recap of the IRC chat:
```
21:14:08 fanquake: Since we seem to be giving Boost removal a good shot for 0.15, does anyone have suggestions for replacing GetNumCores?
21:14:26 fanquake: There is std:🧵:hardware_concurrency(), but that seems to count virtual cores, which I don't think we want.
21:14:51 BlueMatt: fanquake: I doubt we'll do boost removal for 0.15
21:14:58 BlueMatt: shit like BOOST_FOREACH, sure
21:15:07 BlueMatt: but all of boost? doubtful, there are still things we need
21:16:36 fanquake: Yea sorry, not the whole lot, but we can remove a decent chunk. Just looking into what else needs to be done to replace some of the less involved Boost usage.
21:16:43 BlueMatt: fair
21:17:14 wumpus: yes, it makes sense to plan ahead a bit, without immediately doing it
21:18:12 wumpus: right, don't count virtual cores, that used to be the case but it makes no sense for our usage
21:19:15 wumpus: it'd create a swarm of threads overwhelming any machine with hyperthreading (+accompanying thread stack overhead), for script validation, and there was no gain at all for that
21:20:03 sipa: BlueMatt: don't worry, there is no hurry
21:59:10 morcos: wumpus: i don't think that is correct
21:59:24 morcos: suppose you have 4 cores (8 virtual cores)
21:59:24 wumpus: fanquake: indeed seems that std has no equivalent to physical_concurrency, on any standard. That's annoying as it is non-trivial to implement
21:59:35 morcos: i think running par=8 (if it let you) would be notably faster
21:59:59 morcos: jeremyrubin and i discussed this at length a while back... i think i commented about it on irc at the time
22:00:21 wumpus: morcos: I think the conclusion at the time was that it made no difference, but sure would make sense to benchmark
22:00:39 morcos: perhaps historical testing on the virtual vs actual cores was polluted by concurrency issues that have now improved
22:00:47 wumpus: I think there are not more ALUs, so there is not really a point in having more threads
22:01:40 wumpus: hyperthreads are basically just a stored register state right?
22:02:23 sipa: wumpus: yes but it helps the scheduler
22:02:27 wumpus: in which case the only speedup using "number of cores" threads would give you is, possibly, excluding other software from running on the cores on the same time
22:02:37 morcos: well this is where i get out of my depth
22:02:50 sipa: if one of the threads is waiting on a read from ram, the other can use the arithmetic unit for example
22:02:54 morcos: wumpus: i'm pretty sure though that the speed up is considerably more than what you might expect from that
22:02:59 wumpus: sipa: ok, I back down, I didn't want to argue this at all
22:03:35 morcos: the reason i haven't tested it myself, is the machine i usually use has 16 cores... so not easy due to remaining concurrency issues to get much more speedup
22:03:36 wumpus: I'm fine with restoring it to number of virtual threads if that's faster
22:03:54 morcos: we should have somene with 4 cores (and  actually test it though, i agree
22:03:58 sipa: i would expect (but we should benchmark...) that if 8 scriot validation threads instead of 4 on a quadcore hyperthreading is not faster, it's due to lock contention
22:04:20 morcos: sipa: yeah thats my point, i think lock contention isn't that bad with 8 now
22:04:22 wumpus: on 64-bit systems the additional thread overhead wouldn't be important at least
22:04:23 gmaxwell: I previously benchmarked, a long time ago, it was faster.
22:04:33 gmaxwell: (to use the HT core count)
22:04:44 wumpus: why was this changed at all then?
22:04:47 wumpus: I'm confused
22:05:04 sipa: good question!
22:05:06 gmaxwell: I had no idea we changed it.
22:05:25 wumpus: sigh 
22:05:54 gmaxwell: What PR changed it?
22:06:51 gmaxwell: In any case, on 32-bit it's probably a good tradeoff... the extra ram overhead is worth avoiding.
22:07:22 wumpus: https://github.com/bitcoin/bitcoin/pull/6361
22:07:28 gmaxwell: PR 6461 btw.
22:07:37 gmaxwell: er lol at least you got it right.
22:07:45 wumpus: the complaint was that systems became unsuably slow when using that many thread
22:07:51 wumpus: so at least I got one thing right, woohoo
22:07:55 sipa: seems i even acked it!
22:07:57 BlueMatt: wumpus: there are more alus
22:08:38 BlueMatt: but we need to improve lock contention first
22:08:40 morcos: anywya, i think in the past the lock contention made 8 threads regardless of cores a bit dicey.. now that is much better (although more still to be done)
22:09:01 BlueMatt: or we can just merge #10192, thats fee
22:09:04 gribble: https://github.com/bitcoin/bitcoin/issues/10192 | Cache full script execution results in addition to signatures by TheBlueMatt · Pull Request #10192 · bitcoin/bitcoin · GitHub
22:09:11 BlueMatt: s/fee/free/
22:09:21 morcos: no, we do not need to improve lock contention first. but we should probably do that before we increase the max beyond 16
22:09:26 BlueMatt: then we can toss concurrency issues out the window and get more speedup anyway
22:09:35 gmaxwell: wumpus: yea, well in QT I thought we also diminished the count by 1 or something? but yes, if the motivation was to reduce how heavily the machine was used, thats fair.
22:09:56 sipa: the benefit of using HT cores is certainly not a factor 2
22:09:58 wumpus: gmaxwell: for the default I think this makes a lot of sense, yes
22:10:10 gmaxwell: morcos: right now on my 24/28 physical core hosts going beyond 16 still reduces performance.
22:10:11 wumpus: gmaxwell: do we also restrict the maximum par using this? that'd make less sense
22:10:51 wumpus: if someone *wants* to use the virtual cores they should be able to by setting -par=
22:10:51 sipa: *flies to US*
22:10:52 BlueMatt: sipa: sure, but the shared cache helps us get more out of it than some others, as morcos points out
22:11:30 BlueMatt: (because it means our thread contention issues are less)
22:12:05 morcos: gmaxwell: yeah i've been bogged down in fee estimation as well (and the rest of life) for a while now.. otherwise i would have put more effort into jeremy's checkqueue
22:12:36 BlueMatt: morcos: heh, well now you can do other stuff while the rest of us get bogged down in understanding fee estimation enough to review it 
22:12:37 wumpus: [to answer my own question: no, the limit for par is MAX_SCRIPTCHECK_THREADS, or 16]
22:12:54 morcos: but to me optimizing for more than 16 cores is pretty valuable as miners could use beefy machines and be less concerned by block validation time
22:14:38 BlueMatt: morcos: i think you may be surprised by the number of mining pools that are on VPSes that do not have 16 cores 
22:15:34 gmaxwell: I assume right now most of the time block validation is bogged in the parts that are not as concurrent. simple because caching makes the concurrent parts so fast. (and soon to hopefully increase with bluematt's patch)
22:17:55 gmaxwell: improving sha2 speed, or transaction malloc overhead are probably bigger wins now for connection at the tip than parallelism beyond 16 (though I'd like that too).
22:18:21 BlueMatt: sha2 speed is big
22:18:27 morcos: yeah lots of things to do actually...
22:18:57 gmaxwell: BlueMatt: might be a tiny bit less big if we didn't hash the block header 8 times for every block. 
22:21:27 BlueMatt: ehh, probably, but I'm less rushed there
22:21:43 BlueMatt: my new cache thing is about to add a bunch of hashing
22:21:50 BlueMatt: 1 sha round per tx
22:22:25 BlueMatt: and sigcache is obviously a ton
```
Tree-SHA512: a594430e2a77d8cc741ea8c664a2867b1e1693e5050a4bbc8511e8d66a2bffe241a9965f6dff1e7fbb99f21dd1fdeb95b826365da8bd8f9fab2d0ffd80d5059c
Changes addrman to use the test-before-evict discipline in which an
address is to be evicted from the tried table is first tested and if
it is still online it is not evicted.
Adds tests to provide test coverage for this change.
This change was suggested as Countermeasure 3 in
Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman,
Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report
2015/263. March 2015.
Currently, error messages (such as InitError) are displayed as-is, which
means Qt does auto detection on the format.
This means that it's possible to inject HTML from the command line
though e.g. specifying a wallet name with HTML in it. This isn't
a direct security risk because fetching content from internet is
disabled (and as far as I know we never report strings received
from the network this way). However, it can be confusing.
So explicitly force the format as text.
741f0177c Add DynamicMemoryUsage() to LevelDB (Evan Klitzke)
Pull request description:
This adds a new method `CDBWrapper::DynamicMemoryUsage()` similar to Bitcoin's existing methods of the same name. It's implemented by asking LevelDB for the information, and then parsing the string response. I've also added logging to `CDBWrapper::WriteBatch()` to track this information:
```
$ tail -f ~/.bitcoin/testnet3/debug.log | grep WriteBatch
2018-03-05 19:34:55 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:17 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:17 WriteBatch memory usage: db=chainstate, before=0.0MiB, after=8.0MiB
2018-03-05 19:35:22 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:22 WriteBatch memory usage: db=chainstate, before=8.0MiB, after=17.0MiB
2018-03-05 19:35:26 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:27 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=18.0MiB
2018-03-05 19:35:40 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:41 WriteBatch memory usage: db=chainstate, before=9.0MiB, after=7.0MiB
2018-03-05 19:35:52 WriteBatch memory usage: db=index, before=0.0MiB, after=0.0MiB
2018-03-05 19:35:52 WriteBatch memory usage: db=chainstate, before=7.0MiB, after=9.0MiB
^C
```
As LevelDB doesn't seem to provide a way to get the database name, I've also added a new `m_name` field to the `CDBWrapper`. This is necessary because we have multiple LevelDB databases (two now, and possibly more later, e.g. #11857).
I am using this information in other branches where I'm experimenting with changing LevelDB buffer sizes.
Tree-SHA512: 7ea8ff5484bb07ef806af17d000c74ccca27d2e0f6c3229e12d93818f00874553335d87428482bd8acbcae81ea35aef2a243326f9fccbfac25989323d24391b4
874e81808 Allow dustrelayfee to be set to zero (Luke Dashjr)
Pull request description:
I don't see and can't think of any rationale for forbidding this configuration.
Tree-SHA512: df09441f4aec63e79bea94838b7f8e336cebaeb0a22b5e58d27937bbeb1377f229921aeae43674e0b63fc40a39ae51a264d48aa1cdb4cbd0e3339d32856698bf
9c5a4a6ed Stop special-casing phashBlock handling in validation for TBV (Matt Corallo)
Pull request description:
There is no reason to do this, really, we already have "ignore PoW" flags. Motivated by https://github.com/bitcoin/bitcoin/pull/11739#discussion_r155841721
Tree-SHA512: 37cb1ae5b11c9e8ed7a679bb07ad3b119a2a014744b26d197d67ba21beb19fe6815271df935e40f7c7bd5f2e4d7ae4dad7bd4d00fa230a8d789f37e9de31a769
0bc095efd [qt] Improved "custom fee" explanation in tooltip (Randolf Richardson)
Pull request description:
Thanks to @dooglus for asking about this tooltip in Issue 12500.
Reference: https://www.github.com/bitcoin/bitcoin/issues/12500
I would also appreciate it if someone can confirm that 1 kilobyte in this field indeed represents 1,000 bytes rather than 1,024 bytes (if it's supposed to be 1,024, then I'll gladly make the necessary changes to reflect this).
Tree-SHA512: da2fe0128411b5ef6f0a26382a80601efcf823c3f3591bdd83a7fe7e25777728e7eb89e2e8b175b991566e63838aca12d204792f981031b86e7b2ba28ca50021
9360f5032 Drop extra script variable in ProduceSignature (Russell Yanofsky)
Pull request description:
Was slightly confusing.
Tree-SHA512: 1d18f92c133772ffc8eb71826c8d778988839a14bcefc50f9c591111b0a5f81ebc12bca0f1ab25d5fdd02d3d50c2325c04cbfcbdcd18a7b80ca112d049c2327d
2736c9e05 Avoid unintentional unsigned integer wraparounds in tests (practicalswift)
Pull request description:
Avoid unintentional unsigned integer wraparounds in tests.
This is a subset of #11535 as suggested by @MarcoFalke :-)
Tree-SHA512: 4f4ee8a08870101a3f7451aefa77ae06aaf44e3c3b2f7555faa2b8a8503f97f34e34dffcf65154278f15767dc9823955f52d1aa7b39930b390e57cdf2b65e0f3
18307849b Consensus: Fix bug when compiler do not support __builtin_clz* (532479301)
Pull request description:
#ifdef is not correct since defination is defined to 0 or 1. Should change to #if
Tree-SHA512: ba13a591d28f4d7d6ebaab081be4304c43766a611226f8d2994c8db415dfcf318e82217d26a8c4af290760c68eded9503b39535b0e6e079ded912e6a8fca5b36
fa9461473 [doc] dev-notes: Members should be initialized (MarcoFalke)
Pull request description:
Also, remove mention of threads that were removed long ago.
Motivation:
Make it easier to spot bugs such as #11654 and #12426
Tree-SHA512: 8ca1cb54e830e9368803bd98a8b08c39bf2d46f079094ed7e070b32ae15a6e611ce98d7a614f897803309f4728575e6bc9357fab1157c53d2536417eb8271653
The call to CheckBlock() in ConnectBlock() is redundant with calls to it
prior to storing a block on disk. If CheckBlock() fails with an error
indicating the block is potentially corrupted, then shut down
immediately, as this is an indication that the node is experiencing
hardware issues. (If we didn't shut down, we could go into an infinite
loop trying to reconnect this same bad block, as we're not setting the
block's status to FAILED in the case where there is potential
corruption.)
If CheckBlock() fails for some other reason, we'll end up flagging this
block as bad (perhaps some prior software version "let a bad block in",
as the comment indicates), and not trying to connect it again, so this
case should be properly handled.
ee041196fc Show a transaction's virtual size in its details dialog. (Chris Moore)
Pull request description:
#12501 looks like it is going to mention transaction's "virtual size" in the custom fee tooltip, so let's display the virtual size when the user double-clicks a transaction.
Tree-SHA512: c60ae23c9f86edfba086b840519941d8e8ee1be9da5987ffe6dee3255943ea5d215708ce57464f109a1d1c612c4c0eeb11f8f3e203d8a8cfc1f8ec753a8aac27
This change should make it easier for users to make complete backups of wallets
because they can now just back up the specified `-wallet=<path>` path directly,
instead of having to back up the specified path as well as the transaction log
directory (for incompletely flushed wallets).
Another advantage of this change is that if two wallets are located in the same
directory, they will now use their own BerkeleyDB environments instead using a
shared environment. Using a shared environment makes it difficult to manage and
back up wallets separately because transaction log files will contain a mix of
data from all wallets in the environment.
Remove requirement that two wallet files can only be opened at the same time if
they are contained in the same directory.
This change mostly consists of updates to function signatures (updating
functions to take fs::path arguments, instead of combinations of strings,
fs::path, and CDBEnv / CWalletDBWrapper arguments).
New global variables were introduced in #11882 and not setting them causes:
wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed
wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2]
wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
3f592b8 [QA] add wallet-rbf test (Jonas Schnelli)
8222e05 Disable wallet fallbackfee by default on mainnet (Jonas Schnelli)
Pull request description:
Removes the default fallback fee on mainnet (but keeps it on testnet/regtest).
Transactions using the fallbackfee in case the fallback fee has not been set are getting rejected.
Tree-SHA512: e54d2594b7f954e640cc513a18b0bfbe189f15e15bdeed4fe02b7677f939bca1731fef781b073127ffd4ce08a595fb118259b8826cdaa077ff7d5ae9495810db
eb91835 Add setter for g_initial_block_download_completed (Jonas Schnelli)
3f56df5 [QA] add NODE_NETWORK_LIMITED address relay and sync test (Jonas Schnelli)
158e1a6 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
fa999af [QA] Allow addrman loopback tests (add debug option -addrmantest) (Jonas Schnelli)
6fe57bd Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD (Jonas Schnelli)
31c45a9 Accept addresses with NODE_NETWORK_LIMITED flag (Jonas Schnelli)
Pull request description:
Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD.
Accept and relay NODE_NETWORK_LIMITED peers in addrman.
Tree-SHA512: 8a238fc97f767f81cae1866d6cc061390f23a72af4a711d2f7158c77f876017986abb371d213d1c84019eef7be4ca951e8e6f83fda36769c4e1a1d763f787037
e7d9fc5 [qt] navigate to transaction history page after send (Sjors Provoost)
Pull request description:
Before this change QT just remained on the Send tab, which I found confusing. Now it switches to the Transactions tab. This makes it more clear to the user that the send actually succeeded, and here they can monitor progress.
Ideally I would like to highlight the transaction, e.g. by refactoring `TransactionView::focusTransaction(const QModelIndex &idx)` to accept a transaction hash, but I'm not sure how to do that.
Tree-SHA512: 8aa93e03874de8434e18951f8aec47377814c0bcaf7eda4766fc41d5a4e32806346e12e4139e4d45468dfdf0b786f5a7faa393a31b8cd6c65ccac21fb3782c33
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
From the header include guidelines (developer-notes.md):
"One exception is that a `.cpp` file does not need to re-include the
includes already included in its corresponding `.h` file."
* rpc/util.h includes pubkey.h + utilstrencodings.h. rpc/util.cpp includes rpc/util.h.
* util.h includes fs.h. util.cpp includes util.h.
Further optimize prevector::resize() (which is called by a number of
other prevector methods) to use memset to initialize memory when the
prevector contains trivial types.
In prevector.h, the code which like item_ptr(size()) apears in the loop.
Both item_ptr() and size() judge whether values are held directly or
indirectly, but in most cases it is sufficient to make that judgement
once outside the loop.
This PR adds 2 private function fill() which has the loop to initialize
by specified value (or iterator of the other prevector's element),
but don't call item_ptr() in their loop.
Other functions(assign(), constructor, operator=(), insert())
that has similar loop, call fill() instead of original loop.
Also, resize() was changed like fill(), but it calls the default
constructor for that element each time.
90ba2df11 Fix missing cs_main lock for GuessVerificationProgress() (Jonas Schnelli)
Pull request description:
`GuessVerificationProgress()` needs `cs_main` due to accessing the `pindex->nChainTx`.
This adds a `AssertLockHeld` in `GuessVerificationProgress()` and adds the missing locks in...
* `LoadChainTip()`
* `ScanForWalletTransactions()` (got missed in #11281)
* GUI, `ClientModel::getVerificationProgress()` <--- **this may have GUI performance impacts**, but could be relaxed later with a cache or something more efficient.
Tree-SHA512: 13302946571422375f32af8e396b9d2c1180f2693ea363aeba9e98c8266ddec64fe7862bfdcbb5a93a4b12165a61eec1e51e4e7d7a8515fa50879095dc163412
835a21b Squashed 'src/leveldb/' changes from c521b3ac65..64052c76c5 (MarcoFalke)
Pull request description:
Pull in changes from https://github.com/bitcoin/bitcoin/pull/11674#issuecomment-348174674.
Merges cleanly into master and 0.16 branch.
Tree-SHA512: 819c042c0dfac8dc3078fc182c1e22d4a85b343967475d3389be5b5b056361114d8c9892437cd1dc4b45808c27880c0e166e047afc2c2bd2bbc33e55336a8c33
d2ee6e3 init: Remove translation for `-blockmaxsize` option help (Wladimir J. van der Laan)
Pull request description:
Move `-blockmaxsize`, a deprecated option which is replaced by `-blockmaxweight`, to debug options and remove the translation.
This message is absolutely terrible for translators (esp the `* 4` part).
(for 0.17 we should probably remove this option completely?)
(reported by French Language Coordinator)
Tree-SHA512: 379150c9217672d2f2f93b4c02a3ac638e77ca56fb518e30c56c46d59f89eac422b4f540e70a9abd3c6ad653ac4b786d4734621b18f93804885d81e223f1a908
a6e6e39a8b Bugfix: respect user defined configuration file (-conf) when open conf. file from QT settings (Jonas Schnelli)
Pull request description:
Fixes#12488.
In master, opening the configuration file from the GUI settings will always open the file "bitcoin.conf" regardless of the `-conf=` settings.
This PR makes the GUI settings open configuration file function respect the `-conf` option.
Tree-SHA512: fb54cc699b4d2a3947f749fdf5f1a51251ffd67d0f6c6a937a5b80f0ba5a5c1085d0eef190453bbc04696d4d76c2c266de0fe9712e65e4bb36116158b54263d4
Move `-blockmaxsize`, a deprecated option which is replaced by
`-blockmaxweight`, to debug options and remove the translation.
This message is absolutely terrible for translators (esp the `* 4`
part).
Splits signrwatransaction into a wallet version (signrawtransactionwithwallet) and
non-wallet version (signrawtransactionwithkey). signrawtransaction is marked as DEPRECATED
and will call the right signrawtransaction* command as per the parameters in order to
maintain compatibility.
Updated signrawtransactions test to use new RPCs
b22cce014 scripted-diff: validateaddress to getaddressinfo in tests (Andrew Chow)
b98bfc5ed Create getaddressinfo RPC and deprecate parts of validateaddress (Andrew Chow)
1598f3230 [rpc] Move DescribeAddressVisitor to rpc/util (John Newbery)
39633ecd5 [rpc] split wallet and non-wallet parts of DescribeAddressVisitor (John Newbery)
Pull request description:
This PR makes a new RPC command called `getaddressinfo` which relies on the wallet. It contains all of `validateaddress`'s address info stuff. Those parts in `validateaddress` have been marked as deprecated. The tests have been updated to use `getaddressinfo` except the `disablewallet` test which is the only test that actually uses `validateaddress` to validate an address.
Tree-SHA512: ce00ed0f2416200b8de1e0a75e8517c024be0b6153457d302c3879b3491cce28191e7c29aed08ec7d2eeeadc62918f5c43a7cb79cd2e4b6d9291bd83ec31c852
Previously this was an inline test where the specificity was probably judged
overly specific. As a class method it makes sense to maintain consistency.
And replace some magic values with their constant equivalents.
Moves the parts of validateaddress which require the wallet into getaddressinfo
which is part of the wallet RPCs. Mark those parts of validateaddress which
require the wallet as deprecated.
Validateaddress will call getaddressinfo
for the data that both share for right now.
Moves IsMine functions to libbitcoin_common and then links libbitcoin_wallet
before libbitcoin_common in order to prevent linker errors since IsMine is no
longer used in libbitcoin_server.
Previously, if `invalidateblock` was called on a block in a branch,
NotifyBlockTip would be called on that block's predecessor, creating an
incorrect `rpc/blockchain.cpp:latestblock` value.
Only call NotifyBlockTip if the chain being modified is activeChain.
be45a67 Add some script tests related to BOOL ops and odd values like negative 0. (Richard Kiss)
Pull request description:
Add some script tests related to BOOL ops and odd values like negative 0.
Tree-SHA512: 8e633f7ea5eea39e31016994baf60f295fa1dc8cae27aa5fcfc741ea97136bfb3ddc57bb62b9c6bf9fe256fc09cdd184906ba8e611e297cf8d2d363da2bbf1d4
5f605e1 Make signrawtransaction accept P2SH-P2WSH redeemscripts (Pieter Wuille)
Pull request description:
This is a quick fix for #12418, which is a regression in 0.16.
It permits specifying just the inner redeemscript to let `signrawtransaction` succeed. This inner redeemscript is already reported by `addmultisigaddress` & co.
#11708 uses a different approach, where `listunspent` reports both inner & outer redeemscript, but requires both to be provided to `signrawtransaction`. Part of #11708 is still needed even in combination with this PR however, as currently the inner redeemscript isn't reported by `listunspent`.
Tree-SHA512: a6fa2b2661ce04db25cf029dd31da39c0b4811d43692f816dfe0f77b4159b5e2952051664356a579f690ccd58a626e0975708afcd7ad5919366c490944e3a9a5
1d4cbd2 test: Add unit test for LockDirectory (Wladimir J. van der Laan)
fc888bf util: Fix multiple use of LockDirectory (Wladimir J. van der Laan)
Pull request description:
Wrap the `boost::interprocess::file_lock` in a `std::unique_ptr` inside the map that keeps track of per-directory locks.
This fixes a build issue with the clang 4.0.0+boost-1.58.0p8 version combo on OpenBSD 6.2, and should have no effect otherwise.
Also add a unit test, make the function thread-safe, and fix Linux versus Windows behavior inconsistency.
Meant to fix#12413.
Tree-SHA512: 1a94c714c932524a51212c46e8951c129337d57b00fd3da5a347c6bcf6a947706cd440f39df935591b2079995136917f71ca7435fb356f6e8a128c509a62ec32
bb00c95 Consistently use FormatStateMessage in RPC error output (Ben Woosley)
8b8a1c4 Add test for 'mempool min fee not met' rpc error (Ben Woosley)
c04e0f6 Fix 'mempool min fee not met' debug output (Ben Woosley)
Pull request description:
Output the value that is tested, rather than the unmodified fee value.
Prompted by looking into: #11955
Tree-SHA512: fc0bad47d4af375d208f657a6ccbad6ef7f4e2989ae2ce1171226c22fa92847494a2c55cca687bd5a1548663ed3313569bcc31c00d53c0c193a1b865dd8a7657
This commit fixes problems with calling LockDirectory multiple times on
the same directory, or from multiple threads. It also fixes the build on
OpenBSD.
- Wrap the boost::interprocess::file_lock in a std::unique_ptr inside
the map that keeps track of per-directory locks. This fixes a build
issue with the clang 4.0.0+boost-1.58.0p8 version combo on OpenBSD
6.2, and should have no observable effect otherwise.
- Protect the locks map using a mutex.
- Make sure that only locks that are successfully acquired are inserted
in the map.
- Open the lock file for appending only if we know we don't have the
lock yet - The `FILE* file = fsbridge::fopen(pathLockFile, "a");`
wipes the 'we own this lock' administration, likely because it opens
a new fd for the locked file then closes it.
fa27623 qt: Initialize members in WalletModel (MarcoFalke)
Pull request description:
This prevents segfaults (or errors when running qt in valgrind)
```
Conditional jump or move depends on uninitialised value(s)
WalletModel::checkBalanceChanged() (walletmodel.cpp:156)
Tree-SHA512: 38c8c03c7fa947edb3f1c13eab2ac7a62ef8f8141603c2329a7dc5821a887a349af8014dc739b762e046f410f44a9c6653b6930f08b53496cf66381cadc06246
2e9406c Interrupt loading thread after shutdown request (João Barbosa)
Pull request description:
This change (currently) avoids loading the mempool if shutdown is requested.
Tree-SHA512: 3dca3a6ea5b09bd71db0974584d93dfe81819bc0bdbb4d9b6fa0474755306d1403f6c058ecb8211384493a8f7ca3a9134173db744b7344043cfc7d79286c8fd4
d6f3a73 Remove redundant locks (practicalswift)
Pull request description:
Remove redundant locks:
* ~~`FindNode(...)` is locking `cs_vNodes` internally~~
* `SetAddressBook(...)` is locking `cs_wallet` internally
* `DelAddressBook(...)` is locking `cs_wallet` internally
**Note to reviewers:** From what I can tell these locks are redundantly held from a data integrity perspective (guarding specific variables), and they do not appear to be needed from a data consistency perspective (ensuring a consistent state at the right points). Review thoroughly and please let me know if I'm mistaken :-)
Tree-SHA512: 7e3ca2d52fecb16385dc65051b5b20d81b502c0025d70b0c489eb3881866bdd57947a9c96931f7b213f5a8a76b6d2c7b084dff0ef2028a1e9ca9ccfd83e5b91e
6ef86c9 Do not un-mark fInMempool on wallet txn if ATMP fails. (Matt Corallo)
Pull request description:
Irrespective of the failure reason, un-marking fInMempool
out-of-order is incorrect - it should be unmarked when
TransactionRemovedFromMempool fires.
Clean up of #11839, which I think was the wrong fix.
Tree-SHA512: 580731297eeac4c4c99ec695e15b09febf62249237bc367fcd1830fc811d3166f9336e7aba7f2f6f8601960984ae22cebed781200db0f04e7cd2008db1a83f64
b7f6002ed5 Fix rescan test failure due to unset g_address_type, g_change_type (Russell Yanofsky)
Pull request description:
New global variables were introduced in #11403 and not setting them causes:
```
test_bitcoin: wallet/wallet.cpp:4259: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed.
unknown location(0): fatal error in "importwallet_rescan": signal: SIGABRT (application abort requested)
```
It's possible to reproduce the failure reliably by running:
```
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/importwallet_rescan
```
Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug.
This is similar to bug #12150. Example travis failure is https://travis-ci.org/bitcoin/bitcoin/jobs/340642010
Tree-SHA512: ab40662b3356892b726f1f552e22d58d86b5e982538741e52b37ee447a0c97c76c24ae543687edf2e25d9dd925722909d37abfae95d93bf09e23fa245a4c3351
New global variables were introduced in #11403 and not setting them causes:
test_bitcoin: wallet/wallet.cpp:4259: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed.
unknown location(0): fatal error in "importwallet_rescan": signal: SIGABRT (application abort requested)
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/importwallet_rescan
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
a71c56a clientversion: Use full commit hash for commit-based version descriptions (Luke Dashjr)
Pull request description:
git keeps changing the number of digits in abbreviated hashes, resulting in the GitHub archive hash changing because we include it here.
To workaround this and avoid hashes that become increasingly ambiguous later on, just include the full commit hash when building from git.
This has no effect on tagged releases.
(Cleanly mergable back to 0.10 without backport)
Tree-SHA512: b0be5391fadd16fbc9bbeffe1574a61c95931cbf6dea885d7e3cfcd3474b89e71767b1b55b4eeeeb66e4e119e78ff579cd9d206366d36928a209a31e1c1eed75
91986ed206 scripted-diff: Use UniValue.pushKV instead of push_back(Pair()) (Karel Bilek)
a570098021 Squashed 'src/univalue/' changes from 07947ff2da..51d3ab34ba (MarcoFalke)
Pull request description:
Rebased version of #11386 by karel-3d.
Closes: #11386
Tree-SHA512: f3a81447e573c17e75813f4d41ceb34b9980eac81efdd98ddb149d7c51f792be7e2b32239b6ea7e6da68af23897afa6b4ce3f4e8070f9c4adf5105bf6075f2a0
faca18dcf feebumper: Use PreconditionChecks to determine bump eligibility (MarcoFalke)
718f05cab move more bumpfee prechecks to feebumper::PreconditionChecks (Gregory Sanders)
Pull request description:
This only affects the gui.
Fee-bumping of transactions that are already confirmed or are already conflicted by other transactions should not be offered by the gui.
Tree-SHA512: 4acf8087c69fbe5bd67be0485cdb4055e985bbf84acc420aa786ad31e2dc6c2572baaac1d359af10a6907790f626edca690285d9a46ae5440900ea12624c634f
a25cb0f Use ptrdiff_t type to more precisely indicate usage and avoid compiler warnings. (murrayn)
Pull request description:
ptrdiff_t is a more strictly correct type, and gets rid of compiler warnings.
Tree-SHA512: 39718a5cdc10e698f14185f4622a9b439728bce619bd8b3a86f2b99ed5b056cf5a8545a3e5c4bc8a6a01b845fb73510036cee5e6d2629c58df26be692a957fba
004f999 boost: drop boost threads for [alert|block|wallet]notify (Cory Fields)
0827267 boost: drop boost threads from torcontrol (Cory Fields)
ba91724 boost: remove useless threadGroup parameter from Discover (Cory Fields)
f26866b boost: drop boost threads for upnp (Cory Fields)
Pull request description:
This doesn't completely get rid of boost::thread, but this batch should be easy to review, and leaves us with only threadGroup (scheduler + scriptcheck) remaining.
Note to reviewers: The upnp diff changes a bunch of whitespace, it's much more clear with 'git diff -w'
Tree-SHA512: 5a356798d0785f93ed143d1f0afafe890bc82f0d470bc969473da2d2aa78bcb9b096f7ba11b92564d546fb447d4bd0d347e7842994ea0170aafd53fda7e0a66e
c4af738 Fix ignoring tx data requests when fPauseSend is set on a peer (Matt Corallo)
Pull request description:
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).
Probably worth slipping into 0.16 :/.
Tree-SHA512: a9313cef8ac6da31eb099c9925c8401a638220cf7bc9b7b7b83151ecae4b02630f2db45ef6668302b9bb0f38571afbd764993427f1ec9e4d74d9a3be6647d299
a8b5d20 Reset pblocktree before deleting LevelDB file (Sjors Provoost)
Pull request description:
#11043 repaced:
```
delete pblocktree;
pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset);
```
With:
```
pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset));
```
This is problematic because `new CBlockTreeDB` tries to delete the existing file, which will fail with `LOCK: already held by process` if it's still open. That's the case for QT.
When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened `blocks/index`. It then runs this [while loop](https://github.com/bitcoin/bitcoin/blob/v0.16.0rc3/src/init.cpp#L1415) again with `fReset = 1`, resulting in the above error.
This change makes that error go away, presumably because `reset()` without an argument closes the file.
Tree-SHA512: fde8b546912f6773ac64da8476673cc270b125aa2d909212391d1a2001b35c8260a8772126b99dfd76b39faaa286feb7c43239185fe584bd4dc2bc04a64044ce
1687cb4 Refactor: One CBaseChainParams should be enough (Jorge Timón)
Pull request description:
There's no need for class hierarchy with CBaseChainParams, it is just a struct with 2 fields.
This starts as a +10-43 diff
Tree-SHA512: 0a7dd64ab785416550b541787c6083540e4962d76b6cffa806bb3593aec2daf1752dfe65ac5cd51b34ad5c31dd8292c422b483fdd2d37d0b7e68725498ed4c2d
c8edc2c [docs] initial QT documentation, move Qt Creator instructions (Sjors Provoost)
Pull request description:
I'll update this as I figure out how everything is tied together, but I think it's a useful enough start.
Tree-SHA512: d96e5c9ba8ccc3a1b92a0894a8a8449317100eebb14e5d390b51793534458f50eac296cf2945fccf81b85aff23fa32d91d6015a0a76ada4f7091a400d7508ae5
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
Pull 0.16 translations before forking, to avoid having to do it twice.
Tree-SHA512: 9c093885f03783e0f64718985c5f9d385d2a8592e2acc87d922ca973d07c756a6b7fff585388094f0e1b673c41e792ce918c1f594b45e82a262acd93e1b91a8e
082a61c Move scheduler/threadGroup into common-init instead of per-app (Matt Corallo)
Pull request description:
This resolves#12229 which pointed out a shutdown deadlock due to
scheduler/checkqueue having been shut down while network message
processing is still running.
Tree-SHA512: 0c0a76113996b164b0610d3b8c40b396f3e384d165bf098768e31fe3701b00763d0d810ef24702387e2e936fefb9fb900a6225f7417bb0175b585f365d542660
ee11121 Add special error for genesis coinbase to gettransaction (MeshCollider)
Pull request description:
Suggested by sipa here: https://botbot.me/freenode/bitcoin-core-dev/2018-01-23/?msg=96069825&page=2
Just adds a special error message for the genesis block coinbase transaction when using `getrawtransaction`
Tree-SHA512: cd102c7983ec5457b299bff4b6db747d339fda157933a3ac54aec26b1e48b115aa68c1c9e6cb7a916f15c7786273ab558b2b20ab9768544d211e0ae9d1480e34
c6b6385651 [rpc] Mention that HD is enabled if hdmasterkeyid is present (fanquake)
Pull request description:
Also adds optional flag.
Replaces #12042
Tree-SHA512: 6aded2cf36799f697eacbe4f51dff7c8110281092ca3c4fd73e26f57d788ec95a4fcbaf6976ef144e64f2e52759dbe241ac218a95e5ca2233cae1b98bfa5872e
6eddd43 Fix warnings when building with DEBUG_ADDRMAN (practicalswift)
Pull request description:
Fix warnings when building with `DEBUG_ADDRMAN`.
Warnings prior to this commit:
```
addrman.cpp:390:24: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
if (vRandom.size() != nTried + nNew)
~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~
addrman.cpp:411:52: warning: comparison of integers of different signs: 'int' and 'size_type' (aka 'unsigned long') [-Wsign-compare]
if (info.nRandomPos < 0 || info.nRandomPos >= vRandom.size() || vRandom[info.nRandomPos] != n)
~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
addrman.cpp:419:25: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
if (setTried.size() != nTried)
~~~~~~~~~~~~~~~ ^ ~~~~~~
addrman.cpp:421:23: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
if (mapNew.size() != nNew)
~~~~~~~~~~~~~ ^ ~~~~
4 warnings generated.
```
Tree-SHA512: 0316faecfe95066d2c9a0b6b3960086e43824f21a67086a895ea45fbce1327f8d6df5945fe923c2dbe4efce430bc1384d515d317c3930d97d24965e507cf734d
a73aab7 Use the character based overload for std::string::find. (Alin Rus)
Pull request description:
std::string::find has a character based overload as can be seen here
(4th oveload): http://www.cplusplus.com/reference/string/string/find/
Use that instead of constantly allocating temporary strings.
Tree-SHA512: dc7684b1551e6d779eb989e9a74363f9b978059a7c0f3db09d01744c7e6452961f9e671173265e71efff27afbcb80c0fe2c11b6dff2290e54a49193fa25a5679
30ded3e [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor)
Pull request description:
As per PR title, this var is no longer required
Tested by doing a successful compile.
Tree-SHA512: 845e5e0a250cb99e353e8584e9af2df44d97683174e8caa1069bd192b72ab622063765f29676272aecaef87596d72c63ae628404e2f42555136ec2c26d419f2d
34328b4 Use PACKAGE_NAME instead of hardcoding application name in log message (Wladimir J. van der Laan)
0c74e2e Log debug build status and warn when running benchmarks (Wladimir J. van der Laan)
Pull request description:
Log whether the starting instance of bitcoin core is a debug or release build (--enable-debug).
Also warn when running the benchmarks with a debug build, to prevent mistakes comparing debug to non-debug results.
Tree-SHA512: f612dcb7d0a8435016cff0df8aef4942144dfb88be8a00df45cc8830d2aba4b167f6d397b83f8f57d57685888babd04ba88d4dac5a202d3dbd91bcbea3708ef0
bde8bcd Update defaultAssumeValid according to release-process.md. (Gregory Maxwell)
Pull request description:
Update defaultAssumeValid according to release-process.md.
Updated for block 506067 (0000000000000000005214481d2d96f898e3d5416e43359c145944a909d242e0).
Tree-SHA512: 38e6440a11609e56df816e295ceff697d29830a1d85009f920250320c474f8b04a182635b2f1ab44f54d0cdb8b4e4378336a788400e513d7482514434e53b3ff
1e90544 net: Update hardcoded seeds (Wladimir J. van der Laan)
55f52bd contrib: Update ATTERN_AGENT to include 0.15.x (Wladimir J. van der Laan)
Pull request description:
Update the hardcoded node addresses, after changing the pattern to include 0.15.x.
Tree-SHA512: 58b997393d77dcee3dcaffba0c6f185ca46c24d766b33d3a8c9d9efe5dd2e01f086b894a23e185120eee5054697e409b64736e53ca8e42b2315f82355c5f5d5c
This resolves#12229 which pointed out a shutdown deadlock due to
scheduler/checkqueue having been shut down while network message
processing is still running.
ba490d2460 qt: Bump BLOCK_CHAIN_SIZE to 200GB (Wladimir J. van der Laan)
Pull request description:
Part of the release process for 0.16.
Value is open for discussion, my blocks/ directory is 163GB but this leaves some slack.
Tree-SHA512: 4dff81740992bf9de90427934afeb223ea5216f5682c9f07cb5c47aea33980a4c682fe3fd43c3dfa2c4d66ad0e7434dbce6cb252e56d63b36df605e12af9b10a
16f6f59dc [qa] Test fundrawtransaction with change_type option (João Barbosa)
536ddeb17 [rpc] Add change_type option to fundrawtransaction (João Barbosa)
31dbd5af4 [wallet] Add change type to CCoinControl (João Barbosa)
Pull request description:
Adds a new option `change_type` to `fundrawtransaction` RPC. This is useful to override the node `-changetype` argument.
The new option is exclusive to `changeAddress` option, setting both raises a RPC error.
See also #11403, #12119.
Tree-SHA512: 654686444f6125e37015a62f167064d54ec335701534988447be4687fa5ef9c7980a8a07cc0a03fff6ea6c4c1abf0f77a8843d535c4f3fe0bf93f968a4e676e6
f523c6bec [qa] Use address type in addmultisigaddress to avoid addwitnessaddress (João Barbosa)
886a92f25 [rpc] Add address type option to addmultisigaddress (João Barbosa)
Pull request description:
Adds the option `address_type` to `addmultisigaddress` and `createmultisg` RPC. This also allows to avoid `addwitnessaddress` to obtain an `p2sh-segwit` or `bech32` multsig address.
Related to #12210 as this reduces `addwitnessaddress` usage.
Tree-SHA512: 8f8f85dfcff66bb6c7e1e9865e37c285dead1d6dadb9672a89b92fa209d03cc35817ca1d656588c6c2146b728daaf7540b851929b640294653c62836cbefe7ee
04ededf Make CKey::Load references const (Russell Yanofsky)
Pull request description:
No change in behavior, this just prevents CKey::Load arguments from looking
like outputs.
Tree-SHA512: 6d93bce109318e88ddd5c21ad626571344707ae0e6d46e898c76fd95a7afd1c32202a6b3dfab47d6a787c84dfcbb35343cdec898bcf8f668574aa224f2eed977
596c446 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (Sjors Provoost)
Pull request description:
If `-changetype` is not explicitly set, then regardless of `-addresstype`, the wallet will use a ~`bech32` change address~ `P2WPKH` change output if any destination is `P2WPKH` or `P2WSH`.
This seems more intuitive to me and more in line with the spirit of [BIP-69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki).
When combined with #11991 a QT user could opt to use `bech32` exclusively without having to figure out how to launch with `-changetype=bech32`, although so would #11937.
Tree-SHA512: 9238d3ccd1f3be8dfdd43444ccf45d6bdc6584ced3172a3045f3ecfec4a7cc8999db0cdb76ae49236492a84e6dbf3a1fdf18544d3eaf6d518e1f8bd241db33e7
1df206f Disallow using addresses in createmultisig (Andrew Chow)
Pull request description:
This PR should be the last part of #7965.
This PR makes createmultisig only accept public keys and marks the old functionality of accepting addresses as deprecated.
It also splits `_createmultisig_redeemscript` into two functions, `_createmultisig_getpubkeys` and `_createmultisig_getaddr_pubkeys`. `_createmultisig_getpubkeys` retrieves public keys from the RPC parameters and `_createmultisig_getaddr_pubkeys` retrieves addresses' public keys from the wallet. `_createmultisig_getaddr_pubkeys` requires the wallet and is only used by `addwitnessaddress` (except when `createmultisig` is used in deprecated mode).
`addwitnessaddress`'s API is also changed. Instead of returning just an address, it now returns the same thing as `createmultisig`: a JSON object with two fields, address and redeemscript.
Tree-SHA512: a5796e41935ad5e47d8165ff996a8b20d5112b5fc1a06a6d3c7f5513c13e7628a4fd37ec30fde05d8b15abfed51bc250710140f6834b13f64d0a0e47a3817969
2b839ab Update chainparams comment for more info on service bits per dnsseed (Matt Corallo)
62e7642 Fall back to oneshot for DNS Seeds which don't support filtering. (Matt Corallo)
51ae766 Use GetDesireableServiceFlags in static seeds, document this. (Matt Corallo)
fb6f6b1 bluematt's testnet-seed now supports x9 (and is just a static list) (Matt Corallo)
Pull request description:
4440710 broke inserting entries into addrman from dnsseeds which
did not support service bits, as well as static seeds. Static seeds
were already being filtered by UA for 0.13.1+ (ie NODE_WITNESS), so
simply changing the default service bits to include NODE_WITNESS
(and updating docs appropriately) is sufficient. For DNS Seeds, not
supporting NODE_WITNESS is no longer useful, so instead use
non-filtering seeds as oneshot hosts irrespective of named proxy.
I've set my testnet-seed to also support x9, though because it is simply a static host, it may be useful to leave the support off so that it is used as a oneshot to get addresses from a live node instead. I'm fine with either.
Tree-SHA512: 3f17d4d2b0b84d876981c962d2b44cb0c8f95f52c56a48c6b35fd882f6d7a40805f320ec452985a1c0b34aebddb1922709156c3ceccd1b9f8363fd7cb537d21d
7f81250 Mention that other RPC calls report keys as "imported" while txns are still missing (Jonas Schnelli)
ccd8ef6 Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock (Jonas Schnelli)
bc356b4 Make sure WalletRescanReserver has successfully reserved the rescan (Jonas Schnelli)
dbf8556 Add RAII wallet rescan reserver (Jonas Schnelli)
8d0b610 Avoid pemanent cs_main/cs_wallet lock during wallet rescans (Jonas Schnelli)
Pull request description:
Right now, we are holding `cs_main`/`cs_wallet` during the whole rescan process (which can take a couple of hours).
This was probably only done because of laziness and it is an important show-stopper for #11200 (GUI rescan abort).
Tree-SHA512: 0fc3f82d0ee9b2f013e6bacba8d59f7334306660cd676cd64c47bb305c4cb7c7a36219d6a6f76023b74e5fe87f3ab9fc7fd2439e939f71aef653fddb0a1e23b1
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.
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
This allows us to not have to update the chainparams whenever a
DNS Seed changes its filtering support, as well fixes a bug
introduced in 44407100f where returned nodes will never be
attempted.
44407100f broke inserting entries into addrman from static seeds
(as well as dnsseeds which did not support service bits). Static
seeds were already being filtered by UA for 0.13.1+ (ie
NODE_WITNESS), so simply changing the default service bits to
include NODE_WITNESS (and updating docs appropriately) is
sufficient.
For DNS Seeds, we will later fix by falling back to oneshot if a
seed does not support filtering.
Also warn if bitcoind is configured to use a relative -datadir path.
Specifying paths relative to the current working directory in a daemon process
can be dangerous, because files can fail to be located even if the
configuration doesn't change, but the daemon is started up differently.
Specifying a relative -datadir now adds a warning to the debug log. It would
not be backwards-compatible to forbid relative -datadir paths entirely, and it
could also be also inconvenient for command line testing.
Specifying a relative -walletdir now results in a startup error. But since the
-walletdir option is new in 0.16.0, there should be no compatibility issues.
Another reason not to use working directory paths for -walletdir specifically
is that the default -walletdir is a "wallets" subdirectory inside the datadir,
so it could be surprising that setting -walletdir manually would choose a
directory rooted in a completely different location.
cc90a4f46 Avoid potential null dereference in ReceiveCoinsDialog constructor (Russell Yanofsky)
Pull request description:
Not a bug in practice because current `WalletModel::getDefaultAddressType()` implementation does not dereference its `this` pointer.
Encountered issue while rebasing #10244 after #11991 was merged.
Tree-SHA512: d76afc410d4a436ec62936196fdac1af89c221d8c0d6e73349024afe55bbf8820f843177a8fe8210aa8021d45a17a0ecd9b6f693381e3edb234d9897cece29d7
fa1e69e qa: Sync with validationinterface queue in sync_mempools (MarcoFalke)
Pull request description:
Commit e545dedf72 moved `TransactionAddedToMempool` to the background scheduler thread. Thus, adding a transaction to the mempool will no longer add it to the wallet immediately. Functional tests, that `sync_mempools` and then call into wallet rpcs will race against the scheduler thread.
Fix that race by flushing the scheduler queue.
Fixes#12205; Fixes#12171;
References #9584;
Tree-SHA512: 14d99cff9c4756de9fad412f04e6d8e25bb9a0938f24ed8348de79df5b4ee67763dac5214b1a69e77e60787d81ee642976d1482b1b5637edfc4892a238ed22af
134cdc7 Test walletpassphrase timeout bounds and clamping (Andrew Chow)
0b63e3c Clamp walletpassphrase timeout to 2^(30) seconds and check its bounds (Andrew Chow)
Pull request description:
Fixes#12100
Makes the timeout be clamped to 2^30 seconds to avoid the issue with sign flipping with large timeout values and thus relocking the wallet instantly. Unlocking for at most ~34 years should be sufficient.
Also checks that the timeout is not negative to avoid instant relocks.
Tree-SHA512: 426922f08c54e323d259e25dcdbebc2cd560708a65111ce6051493a7e7c61e79d9da1ea4026cc0d68807d728f5d7c0d7c58168c6ef4167b94cf6c2877af88794
49e5f3f rpc: Add deprecation error for `getinfo` (Wladimir J. van der Laan)
Pull request description:
Add a short informative deprecation message when users use `getinfo`, that points them to the new calls
here to get the different information fields.
This is meant to be temporary, for one release only.
Tree-SHA512: 4fccd8853762d0740d051d9e74cdea5ad6f8d5c0ba67d69e8dd2ac8a1538d8270c1a1fab755d9f052ff3b3677753b09138c8c5ca0bc92d156de90413cd5c1814
63ac8907c [qt] receive tab: bech32 address opt-in checkbox (Sjors Provoost)
Pull request description:
<img width="647" alt="schermafbeelding 2018-01-12 om 18 34 48" src="https://user-images.githubusercontent.com/10217/34887691-a6a796fe-f7c7-11e7-8b89-87ce07c61ce3.png">
Checkbox does what you would expect. Press tab from the amount field to get there.
It's unchecked by default.
When launched with `-addresstype=bech32` it's checked by default. When launched with `-addresstype=legacy` it unchecked and disabled.
The change in `receivecoinsdialog.ui` is smaller than it looks, due to the way git handles XML diffs. I had to add a horizontal spacer to make it look decent, see https://github.com/bitcoin/bitcoin/issues/11950#issuecomment-352870909. This causes column numbers to change in the rest of the grid.
I recommend testing on at least one other OS than OSX to be on the safe side.
Tree-SHA512: ec4b733b796d9a94278a5d8040a69d9574ef50021e68f94f61f2da75d1bb57f39272cbc9f1f7d34f733a19640daf666a23844fcd132f83bfdaf327d9d1d6f105
5f911c5cc2 trivial: fix address_type help text of getnewaddress and getrawchangeaddress (mruddy)
Pull request description:
"p2sh" in the help messages should have been "p2sh-segwit".
The messages before this patch:
`help getnewaddress`
"address_type" (string, optional) The address type to use. Options are "legacy", "**p2sh**", and "bech32". Default is set by -addresstype.
`help getrawchangeaddress`
"address_type" (string, optional) The address type to use. Options are "legacy", "**p2sh**", and "bech32". Default is set by -changetype.
Tree-SHA512: 6dfc0bebe577995f5521b83a12854045ac3eda4e65c9b92fc581da4ee68ab1218e05af82f2154bb2640a0813c5f79e010cd9e5ada449494c8831b3757bda854c
Log whether the starting instance of bitcoin core is a debug or release
build (--enable-debug).
Also warn when running the benchmarks with a debug build, to prevent
mistakes comparing debug to non-debug results.
0a22a52 Use mempool's ancestor sort in transaction selection (Suhas Daftuar)
7abfa53 Add test for new ancestor feerate sort behavior (Suhas Daftuar)
9a51319 Sort mempool by min(feerate, ancestor_feerate) (Suhas Daftuar)
6773f92 Refactor CompareTxMemPoolEntryByDescendantScore (Suhas Daftuar)
Pull request description:
This more closely approximates the desirability of a given transaction for
mining, and should result in less re-sorting when transactions get removed from
the mempool after being mined.
I measured this as approximately a 5% speedup in removeForBlock.
Tree-SHA512: ffa36b567c5dfe3e8908c545a459b6a5ec0de26e7dc81b1050dd235cac9046564b4409a3f8c5ba97bd8b30526e8fec8f78480a912e317979467f32305c3dd37b
8e617e3 Remove unused mempool index (Suhas Daftuar)
Pull request description:
We haven't used the "mining_score" index since 0.12, so remove it.
Tree-SHA512: ae37b8663194986eaeecfc2bbeca7ecb4ae6f0d8384515fa218cbc939a580d4b9f7f997c5297c3f1b3c3a0651edb092f373ac9a4808aaec30d38cb99d5f3ed70
Transaction selection for mining tracks ancestor feerates that are
modified based on transactions that have already been selected. This
commit de-duplicates the code so that the ancestor feerate sorting used
by the mempool can also be directly applied to the miner.
Clamps the timeout of walletpassphrase to 2^(30) seconds, which is
~34 years. Any number greater than that will be forced to be
2^(30). This avoids the sign flipping problem with large values which
can result in a negative time used.
Also perform bounds checks to ensure that the timeout is positive
to avoid immediate relocking of the wallet.
Make createmultisig only accept public keys with the old functionality
marked as deprecated.
Splits _createmultisig_redeemscript into two functions, one for
getting public keys from UniValue and one for getting addresses
from UniValue and then their respective public keys. The one for
retrieving address's public keys is located in rpcwallet.cpp
Changes addwitnessaddress's output to be a JSON object with
two fields, address and redeemscript.
Adds a test to deprecated_rpc.py for testing the deprecation.
Update the tests to use addwitnessaddress or give only public keys
to createmultisig. Anything that used addwitnessaddress was also
updated to reflect the new API.
18be3ab139 Adding test case for SINGLE|ANYONECANPAY hash type in tx_valid.json (Chris Stewart)
Pull request description:
We are missing a test vector for SINGLE|ANYONECANPAY inside of tx_valid.json. This addresses the issue #12060
Tree-SHA512: e3526113477dbf575c4a844cf489dcfa2c037c6d928af6f97413edc1a8d29cdf2143da96471cdfd3de08bf5ed178117ed67926fd70fd42ca391ac0bb0d08f3fd
New global variables were introduced in #11403 and not setting them causes:
test_bitcoin: wallet/wallet.cpp:4199: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed.
unknown location(0): fatal error in "ListCoins": signal: SIGABRT (application abort requested)
It's possible to reproduce the failure reliably by running:
src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins
Failures happen nondeterministically because boost test framework doesn't run
tests in a specified order, and tests that run previously can set the global
variables and mask the bug.
b224a47a1 Add address_types test (Pieter Wuille)
7ee54fd7c Support downgrading after recovered keypool witness keys (Pieter Wuille)
940a21932 SegWit wallet support (Pieter Wuille)
f37c64e47 Implicitly know about P2WPKH redeemscripts (Pieter Wuille)
57273f2b3 [test] Serialize CTransaction with witness by default (Pieter Wuille)
cf2c0b6f5 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille)
37c03d3e0 Support P2WPKH addresses in create/addmultisig (Pieter Wuille)
3eaa003c8 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille)
30a27dc5b Expose method to find key for a single-key destination (Pieter Wuille)
985c79552 Improve witness destination types and use them more (Pieter Wuille)
cbe197470 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille)
0c8ea6380 Abstract out IsSolvable from Witnessifier (Pieter Wuille)
Pull request description:
This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089.
Two new configuration options are added:
* `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`.
* `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used.
All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version.
The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key.
To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:
* All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date.
* All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software.
* All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work.
These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented.
`dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now.
Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959
This introduces two command line flags (-addresstype and -changetype) which control
the type of addresses/outputs created by the GUI and RPCs. Certain RPCs allow
overriding these (`getnewaddress` and `getrawchangeaddress`). Supported types
are "legacy" (P2PKH and P2SH-multisig), "p2sh-segwit" (P2SH-P2WPKH and P2SH-P2WSH-multisig),
and "bech32" (P2WPKH and P2WSH-multisig).
A few utility functions are added to the wallet to construct different address type
and to add the necessary entries to the wallet file to be compatible with earlier
versions (see `CWallet::LearnRelatedScripts`, `GetDestinationForKey`,
`GetAllDestinationsForKey`, `CWallet::AddAndGetDestinationForScript`).
73041c3c99 RPC Docs: addmultisigaddress is intended for non-watchonly addresses (Gregory Sanders)
Pull request description:
Spent a couple hours debugging why my p2sh watchonly funds were not appearing in various accounting calls when address was imported via `addmultisigaddress`.
Tree-SHA512: 0673e276e5ca8cdc4c9357bd835a29bd5a994520a78179600944932c700917142930288bf179f5e89b0874beaf1a88bd70129f3a297a46df42a10bab847017bb
c99a3c32c8 [tests] util_tests.cpp: actually check ignored args (Anthony Towns)
Pull request description:
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.
Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
aad3090 [rpc] Adding ::minRelayTxFee amount to getmempoolinfo and updating mempoolminfee help description (Jeff Rade)
Pull request description:
These are RPC document changes from #11475 which is now merged. Took into consideration comments from #11475 and #6941 for this PR.
Biggest change here is when calling `getmempoolinfo`, will now show the `minrelaytxfee` in the JSON reponse (see below):
```
$ bitcoin-cli getmempoolinfo
{
"size": 50,
"bytes": 13102,
"usage": 70480,
"maxmempool": 300000000,
"mempoolminfee": 0.00001000,
"minrelaytxfee": 0.00001000
}
```
Fixes#8953
Tree-SHA512: 5ca583961365ee1cfe6e0d19afb0b41d542e179efee3b3c5f3fcf7d3ebca9cc3eedfd1434a0da40c5eed84fba98b35646fda201e6e61c689b58bee9cbea44b9e
ebcee1de2 bips: add bip176 (Bits Denomination) (William Casarin)
275b2eeed [qt] change µBTC to bits (William Casarin)
Pull request description:
Now that we have bip176, change "µBTC" to the more colloquial "bits"
Tree-SHA512: eba5e5f89c392728a4f0a3bd81a9779a117b8d72a490390fd031d4e7cc56c2bfee0016aba7ef9535903e8cf2262ce46497283424e378906d0e3bf5b0d2d981c7
6dda059bd [qt] Simplifies boolean expression model && model->haveWatchOnly() (251)
Pull request description:
This PR optimizes the boolean expression `model && model->haveWatchOnly()` to `model->haveWatchOnly()`.
The boolean expression can be optimized because the method `TransactionView::exportClicked` already guards against a potential dereferenced null pointer by returning early if `model` is null.
63a4dc1087/src/qt/transactionview.cpp (L351-L353)
Tree-SHA512: 8bdd0d05bf879745fa39d3ca7524471720ae08ceee9427d5a08776e7b56d18542ae87a6991cd6779e232305f504fdfc77223702b72ecbe231f5f5e98453456dd
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 adds new fields 'pubkeys' and 'embedded' to the RPC's output, and improves the
documentation for previously added 'witness_version' and 'witness_program' fields.
