Commit Graph

11036 Commits

Author SHA1 Message Date
Russell Yanofsky a128bdc9e1 [wallet] Construct CWalletTx objects in CommitTransaction
Construct CWalletTx objects in CWallet::CommitTransaction, instead of having
callers do it. This ensures CWalletTx objects are constructed in a uniform way
and all fields are set.

This also makes it possible to avoid confusing and wasteful CWalletTx copies in
https://github.com/bitcoin/bitcoin/pull/9381

There is no change in behavior.
2018-03-07 21:12:47 -05:00
Wladimir J. van der Laan a34ac6ae07
Merge #9598: Improve readability by removing redundant casts to same type (on all platforms)
06edc23f7 Improve readability by removing redundant casts to same type (on all platforms) (practicalswift)

Pull request description:

  Same binaries check under Linux:

  ```
  $ ../bitcoin-maintainer-tools/build-for-compare.py 874f13821f4193bd037cd37d005ee76b5a849398 82274c02ed --executables "src/bitcoind,src/bitcoin-cli,src/bitcoin-tx"

  $ sha256sum /tmp/compare/*.stripped
  1fe1a8827474f7f24475ce3dc851e7ac658d4ed0ae38d11e67f5a810671eaa15  /tmp/compare/bitcoin-cli.82274c02ed2d82537dc55f008a29edb1bc09bbc4.stripped
  1fe1a8827474f7f24475ce3dc851e7ac658d4ed0ae38d11e67f5a810671eaa15  /tmp/compare/bitcoin-cli.874f13821f4193bd037cd37d005ee76b5a849398.stripped
  342c2ed0e60b60990a58cbf5845b256a4f9e3baff9db074baba5e34a620a60ea  /tmp/compare/bitcoind.82274c02ed2d82537dc55f008a29edb1bc09bbc4.stripped
  342c2ed0e60b60990a58cbf5845b256a4f9e3baff9db074baba5e34a620a60ea  /tmp/compare/bitcoind.874f13821f4193bd037cd37d005ee76b5a849398.stripped
  e4b2a80b2361d5cefd67a47eeb9298b8b712c26c7779d979348be8b2c7e3ec93  /tmp/compare/bitcoin-tx.82274c02ed2d82537dc55f008a29edb1bc09bbc4.stripped
  e4b2a80b2361d5cefd67a47eeb9298b8b712c26c7779d979348be8b2c7e3ec93  /tmp/compare/bitcoin-tx.874f13821f4193bd037cd37d005ee76b5a849398.stripped

  $ git diff -W --word-diff /tmp/compare/874f13821f4193bd037cd37d005ee76b5a849398 /tmp/compare/82274c02ed2d82537dc55f008a29edb1bc09bbc4

  $
  ```

Tree-SHA512: 13ca5862fbb03771682b04a7523e581a7fe62e73620fa0e141cf1bc0a3b3f4e2e66bf14b46d1228e2b11b4960153545e7476f3295713a69b5cf5a28a7c2b358d
2018-03-07 17:47:46 +01:00
Wladimir J. van der Laan efa18a230d
Merge #12626: Limit the number of IPs addrman learns from each DNS seeder
46e7f800b Limit the number of IPs we use from each DNS seeder (e0)

Pull request description:

  A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.

  As discussed with @theuni

Tree-SHA512: 949e870765b1470200f2c650341d9e3308a973a7d1a6e557b944b0a2b8ccda49226fc8c4ff7d2a05e5854c4014ec0b67e37a3f2287556fe7dfa2048ede1f2e6f
2018-03-07 17:43:35 +01:00
Wladimir J. van der Laan 842f61a675
Merge #11900: [script] simplify CheckMinimalPush checks, add safety assert
0749808a7 CheckMinimalPush comments are prescriptive (Gregory Sanders)
176db6147 simplify CheckMinimalPush checks, add safety assert (Gregory Sanders)

Pull request description:

  the two conditions could simply never be hit as `true`, as those opcodes have a push payload of size 0 in `data`.

  Added the assert for clarity for future readers(matching the gating in the interpreter) and safety for future use.

  This effects policy only.

Tree-SHA512: f49028a1d5e907ef697b9bf5104c81ba8f6a331dbe5d60d8d8515ac17d2d6bfdc9dcc856a7e3dbd54814871b7d0695584d28da6553e2d9d7715430223f0b3690
2018-03-07 17:18:14 +01:00
Wladimir J. van der Laan 98bc27fb59
Merge #11687: External wallet files
be8ab7d08 Create new wallet databases as directories rather than files (Russell Yanofsky)
26c06f24e Allow wallet files not in -walletdir directory (Russell Yanofsky)
d8a99f65e Allow wallet files in multiple directories (Russell Yanofsky)

Pull request description:

  This change consists of three commits:

  * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory.
  * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory.
  * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up.

  All three commits should be straightforward:

  *  The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper).
  * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test.
  * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths.

    ---

  **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at https://github.com/bitcoin/bitcoin/pull/11687#issuecomment-345625565. Comments before  _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit.

Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
2018-03-07 17:11:54 +01:00
Gregory Sanders 0749808a7f CheckMinimalPush comments are prescriptive 2018-03-07 10:57:00 -05:00
Wladimir J. van der Laan 8a43bdcffd
Merge #12620: Remove TransactionTableModel::TxIDRole
3b26b6af7 qt: Remove TransactionTableModel::TxIDRole (João Barbosa)

Pull request description:

  The role `TxIDRole` is a duplicate of `TxHashRole`. This change favours `TxHashRole`.

Tree-SHA512: ad35933eae1cb6b242b25b8940d662c2c79c766732d76fdd410c80230ec084969294a8e5a126794707992a566076ef4452b592050f7af6c4fa7742891090803d
2018-03-07 16:49:59 +01:00
Wladimir J. van der Laan da9a2f5cd9
Merge #11630: Simplify Base32 and Base64 conversions
b3ea8ccb7 Simplify Base32 and Base64 conversions (Pieter Wuille)
3296a3bb7 Generalize ConvertBits (Pieter Wuille)

Pull request description:

  Generalize `ConvertBits` a bit to also be usable for the existing Base32 and Base64 convertions (rather than just for Bech32).

Tree-SHA512: 3858247f9b14ca4766c08ea040a09b1d6d70caaccc75c2436a54102d6d526f499ec07f5bdfcbbe16cbde5aae521cd16e9aa693e688a97e6c5e74b8e58ee55a13
2018-03-07 16:28:53 +01:00
Wladimir J. van der Laan 4ca7c1e4ac
Merge #9991: listreceivedbyaddress Filter Address
f08761371 Add tests of listreceivedbyaddress address filtering (Jeremy Rubin)
8ee08120d Add address filtering to listreceivedbyaddress (Jeremy Rubin)

Pull request description:

  Supersede https://github.com/bitcoin/bitcoin/pull/9503 created by @JeremyRubin , I will maintain it.

Tree-SHA512: 2accaed493b7e1c2eb5cb5270180f100f8c718b6585b9574f294191c318dc622a79e42ac185300f291f82d3b2a6f1c00850b6b17e4ff2dbab94d71df695acbfe
2018-03-07 16:07:37 +01:00
Pieter Wuille b3ea8ccb7a Simplify Base32 and Base64 conversions 2018-03-07 07:04:07 -08:00
Wladimir J. van der Laan 3fa24bb217
Merge #12204: Fix overly eager BIP30 bypass
5b8b38775 Fix overly eager BIP30 bypass (Alex Morcos)

Pull request description:

  In #6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30.  Unfixed, this could break consensus after block height about 1.9M.  Explained in code comment.

  h/t @sdaftuar

Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
2018-03-07 16:00:46 +01:00
Jeremy Rubin 8ee08120de Add address filtering to listreceivedbyaddress 2018-03-07 08:31:30 -05:00
João Barbosa 3b26b6af72 qt: Remove TransactionTableModel::TxIDRole 2018-03-07 13:08:20 +00:00
Pieter Wuille 3296a3bb7f Generalize ConvertBits 2018-03-06 20:28:08 -08:00
e0 46e7f800bd Limit the number of IPs we use from each DNS seeder
A risk exists where a malicious DNS seeder eclipses a node by returning an enormous number of IP addresses. In this commit we mitigate this risk by limiting the number of IP addresses addrman learns to 256 per DNS seeder.
2018-03-06 18:26:29 -05:00
Wladimir J. van der Laan b225010a80
Merge #11372: Address encoding cleanup
92f1f8b31 Split off key_io_tests from base58_tests (Pieter Wuille)
119b0f85e Split key_io (address/key encodings) off from base58 (Pieter Wuille)
ebfe217b1 Stop using CBase58Data for ext keys (Pieter Wuille)
32e69fa0d Replace CBitcoinSecret with {Encode,Decode}Secret (Pieter Wuille)

Pull request description:

  This PR contains some of the changes left as TODO in #11167 (and built on top of that PR). They are not intended for backporting.

  This removes the `CBase58`, `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey` classes, in favor of simple `Encode`/`Decode` functions. Furthermore, all Bitcoin-specific logic (addresses, WIF, BIP32) is moved to `key_io.{h,cpp}`, leaving `base58.{h,cpp}` as a pure utility that implements the base58 encoding/decoding logic.

Tree-SHA512: a5962c0ed27ad53cbe00f22af432cf11aa530e3efc9798e25c004bc9ed1b5673db5df3956e398ee2c085e3a136ac8da69fe7a7d97a05fb2eb3be0b60d0479655
2018-03-07 00:09:48 +01:00
Wladimir J. van der Laan 29088b14ee
Merge #12600: Add a test for large tx output scripts with segwit input.
5f8cc0df1 Add a test for large tx output scripts with segwit input. (Richard Kiss)

Pull request description:

  This test failed in pycoin but passed in bitcoin, so I thought I'd share it.

Tree-SHA512: 95dff4e03afea4d93ff5e99aa06004446c3df022c2e8a191cac8981107135a5ac2bd3ba1c3a9c4eda9f8f63f584cc1700b7ef57ee6ec2c66a72c699b51bdb61a
2018-03-06 23:57:43 +01:00
Wladimir J. van der Laan 9fb3898c3f
Merge #12432: [qt] send: Clear All also resets coin control options
f506c0a7f [qt] send: Clear All also resets coin control options (Sjors Provoost)

Pull request description:

  This change makes it so that a custom change address and manual input selection are removed if the user clicks Clear All in the send screen.

Tree-SHA512: 78746043a74c9c26ef476eb0df7ce95411683749d9f6b2747222eaac751e241ea7d4d7ce9e4e69ed0b19fa76754d8584e5bef5bba1ad6598f8e39c784b4264d2
2018-03-06 22:35:55 +01:00
Wladimir J. van der Laan e1d6e2af6d
Merge #12622: net: Correct addrman logging
b4db76c55 net: Correct addrman logging (Wladimir J. van der Laan)

Pull request description:

  These were introduced in #9037.

  Found by @theuni (https://github.com/bitcoin/bitcoin/pull/9037#pullrequestreview-101704656).

Tree-SHA512: 9b5153da8a8e5d4ddf9513a5c453f9609cffd4df2924fd48c7b36c1b1055748c7077d4fc0e70be62ca36af87df7f621a744bb374a234baba271ce4982a240825
2018-03-06 22:16:06 +01:00
Wladimir J. van der Laan cd5e4381d4
Merge #12479: RPC: Add child transactions to getrawmempool verbose output
1dfb4e7d7 [Tests] Check output of parent/child tx list from getrawmempool, getmempooldescendants, getmempoolancestors, and REST interface (Conor Scott)
fc44cb108 [RPC] Add list of child transactions to verbose output of getrawmempool (Conor Scott)

Pull request description:

  `bitcoin-cli getrawmempool true` only lists a transaction's parents in the `depends` field. This change adds a `spentby` field to the json response, which lists the transaction's children in the mempool.

  Currently the only way to find child transactions is to use `getrawmempool` or make another call to `getmempooldescendants` and search the response for transactions that list the parent_txid in the `depends` list, which is inefficient.

  This change allows direct lookup of children.

  Example Output
  ```
    "9a9b5733c0d89f207908cfa3fe17809bee71f629aa095c9f8754524e29e98ba4": {
      ...other geterawmempool data...
      "wtxid": "9a9b5733c0d89f207908cfa3fe17809bee71f629aa095c9f8754524e29e98ba4",
      "depends": [
        "bdd92851d5766a42aeb62af667bb422a116cab4e032bba5e3dd6efe5b4b40aa0"
      ],
      "spentby": [
        "dc5d3ec388a9121421208738a041ac30a22163bc2e17758f2275b6c51a15ba7b"
      ]
    },
  ```

Tree-SHA512: 83da7d421c9799a40ef65af3b7fdb586d6d87385f3f2ede3afd2c311725444b858f9d91cc110422a0fa31905779934fee07211ca6fe6b746792b83692c94b3ce
2018-03-06 22:15:18 +01:00
Wladimir J. van der Laan b7c8812ad0
Merge #12564: [arith_uint256] Avoid unnecessary this-copy using prefix operator
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
2018-03-06 21:55:37 +01:00
Wladimir J. van der Laan b4db76c550 net: Correct addrman logging
These were introduced in #9037.

Found by @theuni.
2018-03-06 21:52:53 +01:00
Wladimir J. van der Laan 85424d79ed
Merge #12603: [docs] PeerLogicValidation interface
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
2018-03-06 21:46:10 +01:00
Wladimir J. van der Laan a36834f10b
Merge #9037: net: Add test-before-evict discipline to addrman
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).

  ![attackergraph40000-10-1000short-line](https://cloud.githubusercontent.com/assets/274814/17366828/372af458-595b-11e6-81e5-2c9f97282305.png)

  **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.

  ![attacker graph long view](https://cloud.githubusercontent.com/assets/274814/17367108/96f46d64-595c-11e6-91cd-edba160598e7.png)

Tree-SHA512: fdad4d26aadeaad9bcdc71929b3eb4e1f855b3ee3541fbfbe25dca8d7d0a1667815402db0cb4319db6bd3fcd32d67b5bbc0e12045c4252d62d6239b7d77c4395
2018-03-06 21:36:56 +01:00
James O'Beirne b7cd08b717 Add documentation to PeerLogicValidation interface and related functions 2018-03-06 14:43:32 -05:00
Wladimir J. van der Laan 20e3b9a485
Merge #12617: gui: Show messages as text not html
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
2018-03-06 20:27:15 +01:00
Wladimir J. van der Laan f13d756cdd
Merge #12373: Build: Add build support for profiling.
cfaac2a60 Add build support for 'gprof' profiling. (murrayn)

Pull request description:

  Support for profiling build: `./configure --enable-profiling`

Tree-SHA512: ea983cfce385f1893bb4ab7f94ac141b7d620951dc430da3bbc92ae1357fb05521eac689216e66dc87040171a8a57e76dd7ad98036e12a2896cfe5ab544347f0
2018-03-06 20:21:44 +01:00
Wladimir J. van der Laan bc679829e2
Merge #10271: Use std:🧵:hardware_concurrency, instead of Boost, to determine available cores
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
2018-03-06 19:21:34 +01:00
Wladimir J. van der Laan cff95a6325
Merge #12616: Set modal overlay hide button as default
cfdd89589 qt: Set modal overlay hide button as default (João Barbosa)

Pull request description:

  Without this change the only way to close the modal overlay is to click the hide button. Setting the button to default allows to activate it with the ENTER key.

  Before:
  <img width="849" alt="screen shot 2018-03-06 at 15 14 23" src="https://user-images.githubusercontent.com/3534524/37040276-58af9ce0-2151-11e8-8c55-50acdea669d9.png">

  After:
  <img width="848" alt="screen shot 2018-03-06 at 15 12 41" src="https://user-images.githubusercontent.com/3534524/37040294-650d1c9c-2151-11e8-8245-2da250a71b3d.png">

Tree-SHA512: a93ef440a507843ed7870fd07a693af93dd97c8fce2fb6824c69a227b5dee258f340bf1ae344da32a9dd6e6cb2330f72db9dac9635bbd34184c3e7f8476a472e
2018-03-06 18:52:10 +01:00
Ethan Heilman e68172ed9f Add test-before-evict discipline to addrman
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.
2018-03-06 11:21:01 -05:00
Wladimir J. van der Laan 6fbc0986fa gui: Show messages as text not html
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.
2018-03-06 16:47:31 +01:00
Wladimir J. van der Laan 9903537750
Merge #12604: Add DynamicMemoryUsage() to CDBWrapper to estimate LevelDB memory use
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
2018-03-06 16:28:00 +01:00
João Barbosa cfdd89589b qt: Set modal overlay hide button as default 2018-03-06 15:10:58 +00:00
Evan Klitzke 741f0177c5 Add DynamicMemoryUsage() to LevelDB
This adds a DynamicMemoryUsage() method similar to the existing methods
of the same name, and adds logging of memory usage to
CDBWrapper::WriteBatch.
2018-03-06 00:25:39 -05:00
Wladimir J. van der Laan ed6ae8059c
Merge #12568: Allow dustrelayfee to be set to zero
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
2018-03-05 22:37:47 +01:00
Wladimir J. van der Laan d59b8d6aa1
Merge #11880: Stop special-casing phashBlock handling in validation for TBV
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
2018-03-05 20:09:55 +01:00
Wladimir J. van der Laan c8ea91aa1d
Merge #12501: [qt] Improved "custom fee" explanation in tooltip
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
2018-03-05 20:04:26 +01:00
Wladimir J. van der Laan 47a7666dbe
Merge #11889: Drop extra script variable in ProduceSignature
9360f5032 Drop extra script variable in ProduceSignature (Russell Yanofsky)

Pull request description:

  Was slightly confusing.

Tree-SHA512: 1d18f92c133772ffc8eb71826c8d778988839a14bcefc50f9c591111b0a5f81ebc12bca0f1ab25d5fdd02d3d50c2325c04cbfcbdcd18a7b80ca112d049c2327d
2018-03-05 19:12:37 +01:00
Wladimir J. van der Laan 7f99964321
Merge #12516: Avoid unintentional unsigned integer wraparounds in tests
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
2018-03-05 18:56:40 +01:00
Wladimir J. van der Laan cbc1fcf576
Merge #12573: Fix compilation when compiler do not support __builtin_clz*
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
2018-03-05 18:38:17 +01:00
Wladimir J. van der Laan 21e2670de3
Merge #12434: [doc] dev-notes: Members should be initialized
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
2018-03-05 17:38:59 +01:00
MarcoFalke ce56fdd2e8
Merge #12580: Show a transaction's virtual size in its details dialog
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
2018-03-05 10:41:26 -05:00
MarcoFalke 480f42630c
Merge #12543: Fix typos
d918eb7864 Fix typos (practicalswift)

Pull request description:

  Fix typos.

Tree-SHA512: c790e49be6e01c8d70ebd872ef61cc210c1de15c4a1e5a98280169f32dc8a14cd68f4dd1c23afc76758b28ef355ab12ded2ff7504562dc9b69a11839ad3cd7e3
2018-03-05 08:41:25 -05:00
Richard Kiss 5f8cc0df12 Add a test for large tx output scripts with segwit input. 2018-03-04 17:24:43 -08:00
Russell Yanofsky be8ab7d082 Create new wallet databases as directories rather than files
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.
2018-03-03 10:26:55 -05:00
Russell Yanofsky 26c06f24e5 Allow wallet files not in -walletdir directory
Remove restriction that -wallet filenames can only refer to files in the
-walletdir directory.
2018-03-03 10:26:55 -05:00
Russell Yanofsky d8a99f65e5 Allow wallet files in multiple directories
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).
2018-03-03 10:26:55 -05:00
Russell Yanofsky 7ba2d57852 Fix ListCoins test failure due to unset g_wallet_allow_fallback_fee
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.
2018-03-02 14:50:49 -05:00
Chris Moore ee041196fc Show a transaction's virtual size in its details dialog. 2018-03-01 13:27:13 -08:00
Wladimir J. van der Laan 90a0aed511
Merge #12182: Remove useless string initializations
19ac86e Remove useless string initialization. (Alin Rus)

Pull request description:

Tree-SHA512: 4273dd7e8ed083cc9d05fc70967465e405085b630c000f829648dd44dd0cfe2249f6af1498b02f54b4ca73833130b802488bae8eca0d4d0b803a6f0122b19e8f
2018-03-01 20:51:05 +01:00