| Age | Commit message (Collapse) | Author |
|---|
|
c316fdf [qt] Add support to search the address book (João Barbosa)
Pull request description:
This PR adds support to search the address book for both receiving and sending addresses.
A specialisation of the `QSortFilterProxyModel` is added to implement the custom filtering.
<img width="757" alt="screen shot 2018-01-03 at 16 05 57" src="https://user-images.githubusercontent.com/3534524/34528196-0347d61e-f0a0-11e7-9bd3-535e9e34ceb8.png">
<img width="759" alt="screen shot 2018-01-03 at 16 00 58" src="https://user-images.githubusercontent.com/3534524/34528202-07c99f24-f0a0-11e7-8e34-cff6a1ba2364.png">
Closes #623.
Tree-SHA512: 316e646015c858fc70db6be72dc7922d5bb10a3399e7fa327c992e184cc37a124f11cffefab2dbe0d16bda790c7c0437db364686e66c40b4054b8250b4be15d0
|
|
* Make PeerLogicValidation final to prevent deriving from it [1]
* Prevent deletions of NetEventsInterface and CValidationInterface
objects via a base class pointer
[1] silences the following compiler warning (from Clang 7.0.0):
/usr/include/c++/v1/memory:2285:5: error: delete called on non-final 'PeerLogicValidation' that has
virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
delete __ptr;
^
/usr/include/c++/v1/memory:2598:7: note: in instantiation of member function
'std::__1::default_delete<PeerLogicValidation>::operator()' requested here
__ptr_.second()(__tmp);
^
init.cpp:201:15: note: in instantiation of member function 'std::__1::unique_ptr<PeerLogicValidation,
std::__1::default_delete<PeerLogicValidation> >::reset' requested here
peerLogic.reset();
^
|
|
b4bc32a451 [wallet] Get rid of CWalletTx default constructor (Russell Yanofsky)
a128bdc9e1 [wallet] Construct CWalletTx objects in CommitTransaction (Russell Yanofsky)
Pull request description:
Two commits:
- `Construct CWalletTx objects in CommitTransaction` moves a bunch of CWalletTx initialization into CWallet::CommitTransaction to dedup some code and avoid future inconsistencies in how wallet transactions are created.
- `Get rid of CWalletTx default constructor` does what is described and eliminates the possibility of empty transaction entries being inadvertently created by mapWallet[hash] accesses.
Both of these changes were originally part of #9381
Tree-SHA512: af3841c4f0539e0662d81b33c5369fc70aa06ddde1c59cb00fb21c9e4c7d9ff47f1edc5040cb463af1333838802c56b3ef875b939e2b804ee45b8e0294a4371c
|
|
Failure looks like:
Entering test case "ComputeTimeSmart"
test_bitcoin: sync.cpp:100: void potential_deadlock_detected(const std::pair<void*, void*>&, const LockStack&, const LockStack&): Assertion `false' failed.
unknown location(0): fatal error in "ComputeTimeSmart": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp(566): last checkpoint
Reproducible with:
./configure --enable-debug
make -C src test/test_bitcoin && src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ComputeTimeSmart
Happens due to "92fabcd443 Add LookupBlockIndex function" which acquires
cs_main from inside CWallet::ComputeTimeSmart.
|
|
92fabcd44 Add LookupBlockIndex function (João Barbosa)
43a32b739 Add missing cs_lock in CreateWalletFromFile (João Barbosa)
f814a3e8f Fix cs_main lock in LoadExternalBlockFile (João Barbosa)
c651df8b3 Lock cs_main while loading block index in AppInitMain (João Barbosa)
02de6a6bc Assert cs_main is held when accessing mapBlockIndex (João Barbosa)
Pull request description:
Replace all `mapBlockIndex` lookups with the new `LookupBlockIndex()`. In some cases it avoids a second lookup.
Tree-SHA512: ca31118f028a19721f2191d86f2dd398144d04df345694575a64aeb293be2f85785201480c3c578a0ec99690516205708558c0fd4168b09313378fd4e60a8412
|
|
42343c748 Split up and sanitize CAccountingEntry serialization (Pieter Wuille)
029ecac1b Split up and sanitize CWalletTx serialization (Pieter Wuille)
Pull request description:
This is a small subset of changes taken from #10785, fixing a few of the craziest constness violations in the serialization code.
`CWalletTx` currently serializes some of its fields by embedding them in a key-value `mapValue`, which is modified (and then fixed up) even from the `Serialize` method (for which `mapValue` is const). `CAccountingEntry` goes even further in that it stores such a map by appending it into `strComment` after a null char, which is again later fixed up again.
Fix this by splitting the serialization and deserialization code, and making the serialization act on a copy of `mapValue` / `strComment`.
Tree-SHA512: 487e04996dea6aba5b9b8bdaf2c4e680808f111a15afc557b8d078e14b01e4f40f8ef27588869be62f9a87052117c17e0a0c26c59150f83472a9076936af035e
|
|
fac70134a rpc: Update createrawtransaction examples (MarcoFalke)
fa06dfce0 [rpc] createrawtransaction: Accept sorted outputs (MarcoFalke)
8acd25d85 rpc: Allow typeAny in RPCTypeCheck (MarcoFalke)
Pull request description:
The second parameter of the `createrawtransaction` is a dictionary of the outputs. This comes with at least two drawbacks:
* In case of duplicate keys, either of them might silently disappear, with no user feedback at all. A user needs to make other mistakes, but this could eventually lead to abnormal tx fees.
* A dictionary does not guarantee that keys are sorted. Again, a user needs to keep this in mind, as it could eventually lead to excessive tx fees.
Even though my scenario of loss-of-funds is unlikely to happen, I see it as a inconvenience that should be fixed.
Tree-SHA512: cd562f34f7f9f79c7d3433805971325c388c2035611be283980f4049066a622df4f0afdc11d7ac96662260ec0115147cb65e1ab5268f5a1b063242f3fe425f77
|
|
f4b68b3f8f Log fatal LevelDB errors more verbosely (Evan Klitzke)
Pull request description:
The `leveldb::Status` class logs the filename of corrupted files, which might be useful when looking at error reports from usres. In theory this is already logged via the `LogPrintf()` statement in `HandleError()`, but that may not always be close to where the final error message is logged, e.g. see https://github.com/bitcoin/bitcoin/issues/11355#issuecomment-340340542 where the log trace provided by the user does not contain that information (and other user comments in the same issue).
This also adds a log message instructing the user to run the process with `-debug=leveldb`, which provides much more verbose error messages about LevelDB internals. This may not really help much, but improving the error messages here can't hurt.
Tree-SHA512: bbdc52f0ae50e77e4d74060f9f77c6a0b10d5fad1da371eec1ad38a499af5fde3a3b34dd915e721f6bbe779a1f9693ab04fd9cdbcfa95c28f2979b4c0df181c9
|
|
const std::string&)
b1149ee4c Remove redundant code in MutateTxSign(CMutableTransaction&, const std::string&) (practicalswift)
Pull request description:
Remove redundant code in `MutateTxSign(CMutableTransaction&, const std::string&)`.
Tree-SHA512: 0f0aba4def18b9a4ca73f2bd676881bc05d852d1d34b564416b2b979056263e5471c5d8ce743af44ef6bce11d77b74d18151c983b735cdf47c36f6591ab4b3fb
|
|
|
|
|
|
|
|
|
|
* Z is the zone designator for the zero UTC offset.
* T is the delimiter used to separate date and time.
This makes it clear for the end-user that the date/time logged is
specified in UTC and not in the local time zone.
|
|
No change in behavior in the normal case. But buggy mapWallet lookups with
invalid txids will now throw exceptions instead of inserting dummy entries into
the map, and potentially causing segfaults and other failures.
This also makes it a compiler error to use the mapWallet[hash] syntax which
could create dummy entries.
|
|
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.
|
|
|
|
(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 82274c02ed2d82537dc55f008a29edb1bc09bbc4 --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
|
|
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
|
|
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
|
|
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
|
|
|
|
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
|
|
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
|
|
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
|
|
|
|
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
|
|
|
|
|
|
|
|
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.
|
|
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
|
|
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
|
|
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
|
|
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
|
|
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
|
|
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
|
|
These were introduced in #9037.
Found by @theuni.
|
|
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
|
|
|
|
When accessing mapBlockIndex cs_main must be held.
|
|
|
|
|
|
determine available cores
937bf4335 Use std::thread::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::thread::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
|
|
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
|
