9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift)
Pull request description:
`LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise).
Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises.
This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`).
Issues like the one described in #16028 will be discovered immediately with this PR merged.
Changes in this PR:
* Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code)
* Move `LockAnnotation` in `wallet_tests` to make it reflect the truth
* Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`)
* Rename `LockAnnotation` to `LockAssertion`
ACKs for commit 9f85e9:
ryanofsky:
utACK 9f85e9cb3d687862128ddf464d2bc2462b8627f0. No changes at all since last review except clean rebase after base PR #16033 was merged
Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
e23809a05b [rpc] deriveaddresses: Correct descriptor checksum in RPCExamples (Chris Capobianco)
Pull request description:
Trvial: This fixes the descriptor checksum found the in the deriveaddresses RPC example.
The current checksum value does work, but only if the "h" used for the hardened derivation key origin are replaced with "'".
Given the discussion to switch from "'" to "h" [here](https://github.com/bitcoin/bitcoin/issues/15740), I thought it made more sense to update the checksum rather then changing all the "h" to "'" in this example.
ACKs for commit e23809:
instagibbs:
tACK e23809a05b
Tree-SHA512: 06a2b9f3e714ecde9b9a80b3b7a4082eb072e71d8abcc455ff5387e470d48839f22a70b78bbae1cf9122cb133fee46830819b6f39d67aec8c3c8d5889ae94e04
1609809fb2 validation: Hold cs_main when reading chainActive in RewindBlockIndex (practicalswift)
Pull request description:
Fixes#15980.
Hold `cs_main` when reading `chainActive` (via `::ChainActive()`) in `RewindBlockIndex`.
ACKs for commit 160980:
MarcoFalke:
utACK 1609809fb2a4c2ec15b7c26dc328e2e666bd5d57
Tree-SHA512: 54f180ab391f92f04950735c2bb337f0b7495826d2096f7a0f9a2da50bc29d08747f404a0495e33ca4edd4c842efbab4c4730d5e1a8b9da3e1249cf884268f4b
2c448d6bc7 parameterize hard coded numbers referring to miner conf window (Jordan Baczuk)
Pull request description:
Replace hard coded values (eg. 2016) with `mainnetParams.nMinerConfirmationWindow` where appropriate. This parameterizes hard coded values in the unit test that refer to the `Miner Confirmation Window`, which currently is `2016`. This includes values not exactly 2016 but which were derived from it. Also changed `int` to `uint32_t` where appropriate to avoid compiler warnings. This makes one source of truth, and also helps people who might be adjusting this value in testing so the unit tests don't break.
ACKs for commit 2c448d:
Tree-SHA512: 9262e0b89c1baf7857b49fe2221b2b00f948f61317b321c4871a9182a86d6f8aadeb59d6b133e8a213cc9b31b4a417888fb1ad31caef16ccbbab1de33c4b8459
fa2b52af32f6a4b9c22c270f36e92960c29ef364 Disallow extended encoding for non-witness transactions (take 3) (MarcoFalke)
Pull request description:
(previous title "p2p: Disallow extended encoding for non-witness transactions (take 3)")
Remote peers can send us illegally encoded txs and thus have us write to stderr. Fix that by not writing to stderr.
This is a follow up to the previous (incomplete) attempts at this:
* Disallow extended encoding for non-witness transactions #14039
* Add test for superfluous witness record in deserialization #15893
ACKs for commit fa2b52:
laanwj:
utACK fa2b52af32f6a4b9c22c270f36e92960c29ef364
ryanofsky:
utACK fa2b52af32f6a4b9c22c270f36e92960c29ef364. Would change title to something like "Avoid logging transaction decode errors to stderr" instead of "Disallow extended encoding for non-witness transactions." The current title is confusing because this PR isn't really allowing or disallowing anything, just logging the condition differently. "Disallow" also seems to contradict the "Allow exceptions from..." comments in the actual code.
Tree-SHA512: c66990e69b432d00dc1c5510bf976a1188664d0890a32d1e5c6459094e7e27da82a5d227627afcbc203676f5540eec74b7d9b1d71d2c62d3b2069e1781824b4d
0b09a57ae Give WalletModel::UnlockContext move semantics (Pieter Wuille)
Pull request description:
WalletModel::UnlockContext seems to implement "move upon copy" semantics; with C++11 this can be done more safely using move semantics (making attempts to actually copy fail instead).
Not a big deal if this isn't worth review time.
ACKs for commit 0b09a5:
Empact:
utACK 0b09a57aec
jonasschnelli:
utACK 0b09a57aec4c56712711585a4314d73d4d9b6877
jb55:
utACK 0b09a57aec4c56712711585a4314d73d4d9b6877
Tree-SHA512: f827856586afd03666c2d9f50320776afb3dd511ac1bcd293b330f015acd1588551b163dccc97b1351301e3295f4c74d90e5754bcee89faeadf6437d7db165c8
c3ef63a52 Show loaded wallets as disabled in open menu instead of nothing (MeshCollider)
Pull request description:
Fixes the confusing behavior reported in #15952
ACKs for commit c3ef63:
jonasschnelli:
Tested ACK c3ef63a52f304a600fff1f9c7caa5cb804d41d43
kristapsk:
tACK c3ef63a52f304a600fff1f9c7caa5cb804d41d43
Tree-SHA512: fc2b94936ca32b89e8146c65e3629785883d78660afc8838818df652a4df9185ddca6b36ebf140a7159ab42b0fa5aa72867558d4572a009be06f0831fa813d1f
3cb9ce85d0c6d01217babf0df7efc2eabde1b12f Document strenghtening (Pieter Wuille)
1d207bc46f995ad3b5ae89bb504affaca09d10b1 Add hash strengthening to the RNG (Pieter Wuille)
Pull request description:
This patch improves the built-in RNG using hash strengthening.
At startup, and once every minute, 32 bytes of entropy are produced from the RNG, repeatedly hashed using SHA512 for 10ms, and then fed back into the RNG, together with high-precision timestamps obtained every 1000 iterations.
ACKs for commit 3cb9ce:
pstratem:
utACK 3cb9ce85d0c6d01217babf0df7efc2eabde1b12f
Tree-SHA512: 4fb6f61639b392697beb81c5f0903f79f10dd1087bed7f34de2abb5c22704a671e37b2d828ed141492491863efb1e7d1fa04408a1d32c9de2f2cc8ac406bbe57
fa86c8aec6 init: Remove dead code in LoadChainTip (MarcoFalke)
Pull request description:
`LoadChainTip` sets `::ChainActive()` based on `pcoinsTip`'s best block. `LoadChainTip` is never called when that block is null, so we can remove all code from within that method that is only executed when that block is null.
Fixes#15967 Inconsistent locking behavior in LoadChainTip
ACKs for commit fa86c8:
promag:
utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d.
practicalswift:
utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
Empact:
utACK fa86c8aec6
laanwj:
utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d
ryanofsky:
utACK fa86c8aec611a9b9d2f53960c92419cf2a8bb92d. LoadChainTip isn't called currently when pcoinsTip best block is null due to this line:
jamesob:
utACK fa86c8aec6
Tree-SHA512: 8961c0e579800a52038ac5655478468852faac055299b64d6cfdf0c213d3bf09669c4889467d09d93457f6c8b073967bb0475a137f77ddd3a3a3c03ad90001c4
8794a4b3ae4d34a4cd21a7dee9f694eef7726a4f QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli)
551d489416339dae8f9d896013cd060a21406e2b Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli)
3b64f852e400c552f031697d6a86829dc6e74bd6 QA: add test for CKey::Negate() (Jonas Schnelli)
463921bb649d644f79f9d7f0f96f10aa0d165f76 CKey: add method to negate the key (Jonas Schnelli)
Pull request description:
This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256 and length output of 32 bytes (will be required for v2 transport protocol).
This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`.
Including tests.
This is a subset of #14032 and a pre-requirement for the v2 transport protocol.
ACKs for commit 8794a4:
Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a
fa8ced32a60dea37ac169241cf9a1f708ef46c4b doc: Mention blocksonly in reduce-traffic.md, unhide option (MarcoFalke)
fa320de79faaca2b088fcbe7f76701faa9bff236 test: Add test for p2p_blocksonly (MarcoFalke)
fa3872e7b4540857261aed948b94b6b2bfdbc3d1 test: Format predicate source as multiline on error (MarcoFalke)
fa1dce7329d3e74d46ab98b93772b1832a3f1819 net: Rename ::fRelayTxes to ::g_relay_txes (MarcoFalke)
Pull request description:
This is de-facto no longer hidden
ACKs for commit fa8ced:
jamesob:
utACK fa8ced32a6
Tree-SHA512: 474fbdee6cbd035ed9068a066b6056c1f909ec7520be0417820fcd1672ab3069b53f55c5147968978d9258fd3a3933fe1a9ef8e4f6e14fb6ebbd79701a0a1245
662d1171d9e29964b039ba4c5bc8a2304426c003 Add option to create an encrypted wallet (Andrew Chow)
Pull request description:
This PR adds a new `passphrase` argument to `createwallet` which will create a wallet that is encrypted with that passphrase.
This is built on #15226 because it needs to first create an empty wallet, then encrypt the empty wallet and generate new keys that have only been stored in an encrypted state.
ACKs for commit 662d11:
laanwj:
utACK 662d1171d9e29964b039ba4c5bc8a2304426c003
jnewbery:
Looks great. utACK 662d1171d9e29964b039ba4c5bc8a2304426c003
Tree-SHA512: a53fc9a0f341eaec1614eb69abcf2d48eb4394bc89041ab69bfc05a63436ed37c65ad586c07fd37dc258ac7c7d5e4f7f93b4191407f5824bbf063b4c50894c4a
fa7e311e16 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c2aa scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729242 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)
Pull request description:
This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
* that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
* that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.
ACKs for commit fa7e31:
promag:
utACK fa7e311e169349bfcf1dab8b980724e8ddf4e749.
jnewbery:
utACK fa7e311e169349bfcf1dab8b980724e8ddf4e749
Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8
c01c065b9ded3399a6a480f15543827dd5e8eb4d Do not construct out-of-bound pointers in SHA512/SHA1/RIPEMD160 code (Pieter Wuille)
Pull request description:
This looks like an issue in the current SHA256/512 code, where a pointer outside of the area pointed to may be constructed (this is UB in theory, though in practice every supported platform treats pointers as integers).
I discovered this while investigating #14580. Sadly, it does not fix it.
ACKs for commit c01c06:
practicalswift:
utACK c01c065b9ded3399a6a480f15543827dd5e8eb4d
Tree-SHA512: 47660e00f164f38c36a1ab46e52dd91cd33cfda6a6048d67541c2f8e73c050d4d9d81b5c149bfad281212d52f204f57bebf5b19879dc7a6a5f48aa823fbc2c02
1b05dff080fe50937a02cfde661db58ace21c553 Fix portability issue with pthreads (grim-trigger)
Pull request description:
This change resolves the following issue:
https://github.com/bitcoin/bitcoin/issues/15951
Only tested on OpenBSD 6.5/amd64
ACKs for commit 1b05df:
fanquake:
tACK 1b05dff. Tested on OpenBSD6.4 (`vagrant`).
laanwj:
utACK 1b05dff080fe50937a02cfde661db58ace21c553
Tree-SHA512: af48581af32820d5adc9ae5abb44f8f1b592c323f86fe2484108b81629389f6ef347598f9a087aa6476ac553e59828cd7927bb4ab11dc70e7c9a944a92fc54ae
f1a77b0c51 [docs] Add doxygen comment for CReserveKey (John Newbery)
37796b2dd4 [docs] Add doxygen comment for CKeyPool (John Newbery)
ef2d515af3 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery)
Pull request description:
Docs/move-only
Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg https://github.com/bitcoin/bitcoin/pull/15557#discussion_r271956393).
These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level.
ACKs for commit f1a77b:
jonatack:
Thanks, John. Re-ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281, doc-only changes with respect to previous review.
jb55:
ACK f1a77b0c5176306ca9f6f30211e32d3502ed4281
Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b
f6bb11fd37 Add test for ArgsManager::GetChainName (Russell Yanofsky)
4b331159df Add unit test NextString, ForEachNoDup functions (Russell Yanofsky)
05bfee3451 util_SettingsMerge test cleanup (Russell Yanofsky)
Pull request description:
There was some test coverage previously, but it was limited and didn't test conflicting and negated arguments.
ACKs for commit f6bb11:
MarcoFalke:
re-utACK f6bb11fd37f8a2c985786b688ea07699ba75780e
Tree-SHA512: d03596614dc48584c7a9440117b107c6abb23fd4c7fa15fb4015351ec3de08b2656bc956ce05310663675672343d7a6aff35421657f29172080c7005045680b0
ccc27bdcd2 doc: Clarify -blocksdir usage (Daniel McNally)
Pull request description:
This PR attempts to clarify and correct the `-blocksdir` argument description and default value. `-blocksdir` does not refer to the full path to the actual `blocks` directory, but rather the root/parent directory which contains the `blocks` directory. Accordingly, the default value is `<datadir>` and not `<datadir>/blocks` - this behavior of defaulting to the datadir can also be seen in init.cpp:
```cpp
if (gArgs.IsArgSet("-blocksdir")) {
path = fs::system_complete(gArgs.GetArg("-blocksdir", ""));
if (!fs::is_directory(path)) {
path = "";
return path;
}
} else {
path = GetDataDir(false);
}
```
It also attempts to clarify that only the `.dat` files containing block data are impacted by `-blocksdir`, not the index files.
I believe this would close#12828.
ACKs for commit ccc27b:
hebasto:
utACK ccc27bdcd2d91fe2c023ad004019d5b970f21dbf
Tree-SHA512: 7b65f66b0579fd56e8c8cd4f9f22d6af56181817762a68deccd7fca51820ad82d9a0c48f5f1f012e746c67bcdae7af4555fad867cb620a9ca538d465c9d86c2b
d20d756752 rpc: faster getblockstats using BlockUndo data (Felix Weis)
Pull request description:
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.
```
# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200 0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats 0.21s user 0.19s system 17% cpu 2.302 total
# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200 0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats 0.24s user 0.22s system 0% cpu 3:19.42 total
```
ACKs for commit d20d75:
MarcoFalke:
re-utACK d20d7567528e216badb8475df298bb3cec008985
Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.
