Commit graph

25621 commits

Author SHA1 Message Date
Russell Yanofsky 7bf6dfbb48 wallet: Remove path checking code from bitcoin-wallet tool
This commit does not change behavior except for error messages which now
include more complete information.
2020-09-03 12:24:32 -04:00
Russell Yanofsky 77d5bb72b8 wallet: Remove path checking code from createwallet RPC
This commit does not change behavior except for error messages which now
include more complete information.
2020-09-03 12:24:32 -04:00
Russell Yanofsky a987438e9d wallet: Remove path checking code from loadwallet RPC
This commit does not change behavior except for error messages which now
include more complete information.
2020-09-03 12:24:32 -04:00
Russell Yanofsky 8b5e7297c0 refactor: Pass wallet database into CWallet::Create
No changes in behavior
2020-09-03 12:24:32 -04:00
Russell Yanofsky 3c815cfe54 wallet: Remove Verify and IsLoaded methods
Checks are now consolidated in MakeBerkeleyDatabase function instead of
happening in higher level code.

This commit does not change behavior except for error messages which now
include more complete information.
2020-09-03 12:24:32 -04:00
Russell Yanofsky 0d94e60625 refactor: Use DatabaseStatus and DatabaseOptions types
No changes in behavior. Just replaces arguments and return types
2020-09-03 12:24:32 -04:00
Russell Yanofsky b5b414151a wallet: Add MakeDatabase function
New function is not currently called but will be called in upcoming commits. It
moves database path checking, and existence checking, and already-loaded
checking, and verification into a single function so this logic does not need
to be repeated all over higher level wallet code, and so higher level code does
not need to change when SQLite support is added in
https://github.com/bitcoin/bitcoin/pull/19077. This also lets higher level
wallet code make fewer assumptions about the contents of wallet directories.

This commit just adds the new function and does not change behavior in any way.
2020-09-03 12:24:32 -04:00
Russell Yanofsky 288b4ffb6b Remove WalletLocation class
This removes a source of complexity and indirection that makes it harder to
understand path checking code. Path checks will be simplified in upcoming
commits.

There is no change in behavior in this commit other than a slightly more
descriptive error message in `loadwallet` if the default "" wallet can't be
found. (The error message is improved more in upcoming commit "wallet: Remove
path checking code from loadwallet RPC".)
2020-09-03 12:24:32 -04:00
Jonas Schnelli a0a422c34c
Merge #19754: wallet, gui: Reload previously loaded wallets on startup
f1ee37319a wallet: Reload previously loaded wallets on GUI startup (Andrew Chow)

Pull request description:

  Enable the GUI to also use the load_on_startup feature. Wallets loaded in the GUI always have load_on_startup=true. When they are unloaded from the GUI, load_on_startup=false.

  To facilitate this change, UpdateWalletSetting is moved into the wallet module and called from within LoadWallet, RemoveWallet, and Createwallet. This change does not actually touch the GUI code but rather the wallet functions that are shared between the GUI and RPC.

ACKs for top commit:
  jonasschnelli:
    Tested ACK f1ee37319a - works as expected. Wallets loaded via bitcoin-cli (in `-server` mode) or through the RPC console won't be loaded on startup but wallets loaded via the GUI menu will.
  kristapsk:
    ACK f1ee37319a, I have tested the code.

Tree-SHA512: f5b44aa763cf761d919015c5fbc0600b72434aa71e3b57007fd7530a29c3da1a9a0c98c4f22cb6cdffba61150a31170056a7d4737625e7b76f6958f3d584da8c
2020-09-03 18:24:32 +02:00
Wladimir J. van der Laan bd60a9a8ed
Merge #19818: p2p: change CInv::type from int to uint32_t, fix UBSan warning
7984c39be1 test framework: serialize/deserialize inv type as unsigned int (Jon Atack)
407175e0c2 p2p: change CInv::type from int to uint32_t (Jon Atack)

Pull request description:

  Fixes UBSan implicit-integer-sign-change issue per https://github.com/bitcoin/bitcoin/pull/19610#issuecomment-680686460.

  Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in https://github.com/bitcoin/bitcoin/pull/19590#pullrequestreview-455788826.

  Closes #19678.

ACKs for top commit:
  laanwj:
    ACK 7984c39be1
  MarcoFalke:
    ACK 7984c39be1 🌻
  vasild:
    ACK 7984c39be

Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5
2020-09-03 17:23:52 +02:00
Wladimir J. van der Laan 69a13eb246
Merge #19670: Protect localhost and block-relay-only peers from eviction
752e6ad533 Protect localhost and block-relay-only peers from eviction (Suhas Daftuar)

Pull request description:

  Onion peers are disadvantaged under our eviction criteria, so prevent eventual
  eviction of them in the presence of contention for inbound slots by reserving
  some slots for localhost peers (sorted by longest uptime).

  Block-relay-only connections exist as a protection against eclipse attacks, by
  creating a path for block propagation that may be unknown to adversaries.
  Protect against inbound peer connection slot attacks from disconnecting such
  peers by attempting to protect up to 8 peers that are not relaying transactions
  but have provided us with blocks.

  Thanks to gmaxwell for suggesting these strategies.

ACKs for top commit:
  laanwj:
    Code review ACK 752e6ad533

Tree-SHA512: dbf089c77c1f747aa1dbbbc2e9c2799c628028b0918d0c336d8d0e5338acedd573b530eb3b689c7f603a17221e557268a9f5c3f585f204bfb12e5d2e76de39a3
2020-09-03 17:20:47 +02:00
Wladimir J. van der Laan 4053de04e2
Merge #19859: qa: Fixes failing functional test by changing version
6de9429087 qa: Changes v0.17.1 to v0.17.2 (nthumann)

Pull request description:

  As of 0374e821bd v0.17.2 is downloaded instead of v0.17.1 for functional testing. This causes `test/functional/feature_backwards_compatibility.py` to fail, because it [requires](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_backwards_compatibility.py#L57) v0.17.1.

  Steps to reproduce:
  Run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2`. It cannot be downloaded at all because the sha256sum is missing [here](c1e0c2ad3b/test/get_previous_releases.py (L23)).
  Or adjust the command and run `test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2`, then run `test/functional/test_runner.py feature_backwards_compatibility`. It´ll fail because the test is missing v0.17.1.

  This PR changes v0.17.1 to v0.17.2 in this test and in a few comments.

ACKs for top commit:
  laanwj:
    ACK 6de9429087
  fanquake:
    ACK 6de9429087 - looks correct. Surprised this wasn't caught/part of #19813. In future you could add any explanations & extra info as part of your commit message as well (even though PR descriptions are included as part of the merge).

Tree-SHA512: bbe50c4fd5c1aedd6dc1cdc3d93ef9005db1c67adca3f263b6b0d869c40b495a3221e706c9389fedea4748e31911dbd591062f60ce9836e58099fbdd9515b4d9
2020-09-03 13:38:21 +02:00
Wladimir J. van der Laan 620ac8c475
Merge #19724: [net] Cleanup connection types- followups
eb1c5d090f [doc] Follow developer notes, add comment about missing default. (Amiti Uttarwar)
d5a57cef62 [doc] Describe connection types in more depth. (Amiti Uttarwar)
4829b6fcc6 [refactor] Simplify connection type logic in ThreadOpenConnections (Amiti Uttarwar)
1e563aed78 [refactor] Simplify check for block-relay-only connection. (Amiti Uttarwar)
da3a0be61b [test] Add explicit tests that connection types get set correctly (Amiti Uttarwar)
1d74fc7df6 [trivial] Small style updates (Amiti Uttarwar)
ff6b9081ad [doc] Explain address handling logic in process messages (Amiti Uttarwar)
dff16b184b [refactor] Restructure logic to check for addr relay. (Amiti Uttarwar)
a6ab1e81f9 [net] Remove unnecessary default args on OpenNetworkConnection (Amiti Uttarwar)
8d6ff46f55 scripted-diff: Rename `OUTBOUND` ConnectionType to `OUTBOUND_FULL_RELAY` (Amiti Uttarwar)

Pull request description:

  This PR addresses outstanding review comments from #19316. It further simplifies `net.cpp` complexity and adds documentation about design goals about different connection types.

ACKs for top commit:
  naumenkogs:
    ACK eb1c5d090f
  laanwj:
    Code review ACK eb1c5d090f

Tree-SHA512: 2fe14e428404c95661e5518c8c90db07ab5b9ebb1bac921b3bdf82b181f444fae379f8fc0a2b619e6b4693f01c55bd246fbd8c8eedadd96849a30de3161afee5
2020-09-03 13:28:30 +02:00
fanquake 68f0ab26bc
Merge #19805: wallet: Avoid deserializing unused records when salvaging
0bbe26a1af wallet: filter for keys only before record deser in salvage (Andrew Chow)
544e12a4e8 walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow)

Pull request description:

  When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too.

  This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire.

ACKs for top commit:
  ryanofsky:
    Code review ACK 0bbe26a1af. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered
  laanwj:
    Code review ACK 0bbe26a1af

Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01
2020-09-03 12:47:01 +08:00
fanquake 136fe4c5e9
Merge #19816: test: Rename wait until helper to wait_until_helper
fa1cd9e1dd test: Remove unused lock arg from BitcoinTestFramework.wait_until (MarcoFalke)
fad2794e93 test: Rename wait until helper to wait_until_helper (MarcoFalke)
facb41bf1d test: Remove unused p2p_lock in VersionBitsWarningTest (MarcoFalke)

Pull request description:

  This avoids confusion with the `wait_until` member functions, which should be preferred because they take the appropriate locks and scale the timeout appropriately on their own.

ACKs for top commit:
  laanwj:
    Code review ACK fa1cd9e1dd
  hebasto:
    ACK fa1cd9e1dd, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 319d400085606a4c738e314824037f72998e6657d8622b363726842aba968744f23c56d27275dfe506b8cbbb6e97fc39ca1d325db05d4d67df0e8b35f2244d5c
2020-09-03 12:07:53 +08:00
fanquake 9876ab8c74
Merge #19844: remove usage of boost::bind
e36f802fa4 lint: add C++ code linter (fanquake)
c4be50fea3 remove usage of boost::bind (fanquake)

Pull request description:

  `boost::bind` usage was removed in #13743. However a new usage snuck in as
  part of 2bc4c3eaf9 (#15225).

ACKs for top commit:
  hebasto:
    ACK e36f802fa4
  practicalswift:
    ACK e36f802fa4 -- patch looks correct

Tree-SHA512: 2b0387c5443c184bcbf7df4849db1ed1296ff82c7b4ff0aff18334a400e56a472a972d18234d3866531a088d7a8da64688e58dc9f15daaad4048697c759d55ce
2020-09-03 11:43:10 +08:00
fanquake 2d4574aad8
Merge #19861: build: add /usr/local/ to LCOV_FILTER_PATTERN for macOS builds
9bdde3c802 build: add /usr/local/ to LCOV_FILTER_PATTERN for macOS builds (eugene)

Pull request description:

  With this commit, the files in /usr/local/ will not be included in
  `make cov` or `make cov_fuzz` coverage reports. This behavior could
  be observed when generating the reports on macOS with brew-installed
  clang.

ACKs for top commit:
  fanquake:
    ACK 9bdde3c802

Tree-SHA512: 15cbe8d514651448f3d7b8b0a00939fd6bd6f15e6812f1959fcaaab7364ca2ef4ee34f2ff8950b7fdc8ae64d043dc5f7185c0601dd94780b41331337e5e84c45
2020-09-03 11:36:45 +08:00
Amiti Uttarwar eb1c5d090f [doc] Follow developer notes, add comment about missing default. 2020-09-02 17:18:22 -07:00
Amiti Uttarwar d5a57cef62 [doc] Describe connection types in more depth. 2020-09-02 17:18:22 -07:00
Amiti Uttarwar 4829b6fcc6 [refactor] Simplify connection type logic in ThreadOpenConnections
Consolidate the logic to determine connection type into one conditional to
clarify how they are chosen.
2020-09-02 17:18:22 -07:00
Amiti Uttarwar 1e563aed78 [refactor] Simplify check for block-relay-only connection.
Previously we deduced it was a block-relay-only based on presence of the
m_tx_relay structure. Now we have the ability to identify it directly via a
connection type accessor function.
2020-09-02 17:18:22 -07:00
Amiti Uttarwar da3a0be61b [test] Add explicit tests that connection types get set correctly 2020-09-02 17:18:22 -07:00
Amiti Uttarwar 1d74fc7df6 [trivial] Small style updates 2020-09-02 17:18:21 -07:00
Amiti Uttarwar ff6b9081ad [doc] Explain address handling logic in process messages
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
2020-09-02 17:18:21 -07:00
Amiti Uttarwar dff16b184b [refactor] Restructure logic to check for addr relay.
We previously identified if we relay addresses to the connection by checking
for the existence of the m_addr_known data structure. With this commit, we
answer this question based on the connection type.

IsAddrRelayPeer() checked for the existence of the m_addr_known
2020-09-02 17:18:21 -07:00
Amiti Uttarwar a6ab1e81f9 [net] Remove unnecessary default args on OpenNetworkConnection 2020-09-02 13:36:29 -07:00
Amiti Uttarwar 8d6ff46f55 scripted-diff: Rename OUTBOUND ConnectionType to OUTBOUND_FULL_RELAY
-BEGIN VERIFY SCRIPT-
sed -i 's/OUTBOUND, /OUTBOUND_FULL_RELAY, /g' src/net.h
sed -i 's/ConnectionType::OUTBOUND/ConnectionType::OUTBOUND_FULL_RELAY/g' src/test/net_tests.cpp src/test/fuzz/process_message.cpp src/test/fuzz/process_messages.cpp src/net.cpp src/test/denialofservice_tests.cpp src/net.h src/test/fuzz/net.cpp
-END VERIFY SCRIPT-
2020-09-02 13:34:58 -07:00
eugene 9bdde3c802 build: add /usr/local/ to LCOV_FILTER_PATTERN for macOS builds
With this commit, the files in /usr/local/ will not be included in
`make cov` or `make cov_fuzz` coverage reports. This behavior could
be observed when generating the reports on macOS with brew-installed
clang.
2020-09-02 15:17:13 -04:00
Suhas Daftuar 752e6ad533 Protect localhost and block-relay-only peers from eviction
Onion peers are disadvantaged under our eviction criteria, so prevent eventual
eviction of them in the presence of contention for inbound slots by reserving
some slots for localhost peers (sorted by longest uptime).

Block-relay-only connections exist as a protection against eclipse attacks, by
creating a path for block propagation that may be unknown to adversaries.
Protect against inbound peer connection slot attacks from disconnecting such
peers by attempting to protect up to 8 peers that are not relaying transactions
but appear to be full-nodes, sorted by recency of last delivered block.

Thanks to gmaxwell for suggesting these strategies.
2020-09-02 09:21:33 -04:00
Wladimir J. van der Laan c157a50694
Merge #19840: Avoid callback when -blocknotify is empty
413e0d1d31 Avoid callback when -blocknotify is empty (João Barbosa)

Pull request description:

ACKs for top commit:
  MarcoFalke:
    ACK 413e0d1d31
  practicalswift:
    ACK 413e0d1d31 -- patch looks correct
  laanwj:
    Code review ACK 413e0d1d31

Tree-SHA512: 915e796666b4e74dbb029ba5436e5573a4b881aad9e118f737bcff4024528b7ff3b00dd035138f63d30963cfd66195f6e53a2dbe429ee28cb6f0b9cc47218ecf
2020-09-02 15:14:34 +02:00
fanquake 8845b38b59
Merge #19685: depends: CMake invocation cleanup
b893688357 depends: Specify LDFLAGS to cmake as well (Carl Dong)
b3f541f618 depends: Prepend CPPFLAGS to C{,XX}FLAGS for CMake (Carl Dong)
8e121e5509 depends: Cleanup CMake invocation (Carl Dong)
8c7cd0c6d9 depends: More robust cmake invocation (Carl Dong)
3ecf0eca63 depends: Use $($(package)_cmake) instead of cmake (Carl Dong)

Pull request description:

  - Use `$($(package)_cmake)` instead of invoking `cmake` directly
  - Use well-known env vars instead of overriding CMake variables

ACKs for top commit:
  ryanofsky:
    Code review ACK b893688357. Only changes since last review are new commits adding whitespace, cppflags and ldflags to cmake invocation

Tree-SHA512: cfcd8cc9dcd0b336cf48b82fca9fe4bbc7930ed397cb7a68a07066680eb4c1906a6a9b5bd2589b4b4999e8f16232fa30ee9b376b60f4456d0fff931fbf9cc19a
2020-09-02 21:03:05 +08:00
fanquake c17a003758
Merge #19857: net: improve nLastBlockTime and nLastTXTime documentation
d780293e1e net: improve nLastBlockTime and nLastTXTime documentation (Jon Atack)

Pull request description:

  Follow-up to #19731 to help alleviate confusion around `nLastBlockTime` and `nLastTXTime`, now also provided by the JSON-RPC API as `last_block` and `last_transaction` in `getpeerinfo` output.

  Thanks to John Newbery, credited in the commit, and to Dave Harding and Adam Jonas during discussions on how to best explain these in this week's Optech newsletter.

ACKs for top commit:
  practicalswift:
    ACK d780293e1e
  MarcoFalke:
    ACK d780293e1e
  harding:
    ACK d780293e1e .  The added documentation matches my reading of the code and answers a question I had after seeing #19731
  0xB10C:
    ACK d780293e1e

Tree-SHA512: 72d47cf50a099913c7e4753cb80e11785b26fb66fa3a8b6c382fde4ea725116f3d215f93d32a567246d269768e66159f8dcf017a1bbc6d5f2489a35f81c316fa
2020-09-02 20:35:25 +08:00
nthumann 6de9429087
qa: Changes v0.17.1 to v0.17.2 2020-09-02 13:51:36 +02:00
Wladimir J. van der Laan 505b39e72b
Merge #19610: p2p: refactor AlreadyHave(), CInv::type, INV/TX processing
fb56d37612 p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385e test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01ea p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453b p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd6642167 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b86 [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618ca [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc9445 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f0 p2p: add CInv block message helper methods (Jon Atack)

Pull request description:

  Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.

  Some of the diffs are best reviewed with `-w` to ignore spacing.

  Co-authored by John Newbery.

ACKs for top commit:
  laanwj:
    Code review ACK fb56d37612
  jnewbery:
    utACK fb56d37612
  vasild:
    ACK fb56d3761

Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
2020-09-02 13:45:40 +02:00
Jonas Schnelli 3a3e21dafb
Merge #14687: zmq: enable tcp keepalive
c276df7759 zmq: enable tcp keepalive (mruddy)

Pull request description:

  This addresses https://github.com/bitcoin/bitcoin/issues/12754.

  These changes enable node operators to address the silent dropping (by network middle boxes) of long-lived low-activity ZMQ TCP connections via further operating system level TCP keepalive configuration. For example, ZMQ sockets that publish block hashes can be affected in this way due to the length of time it sometimes takes between finding blocks (e.g.- sometimes more than an hour).

  Prior to this patch, operating system level TCP keepalive configurations would not take effect since the SO_KEEPALIVE option was not enabled on the underlying socket.

  There are additional ZMQ socket options related to TCP keepalive that can be set. However, I decided not to implement those options in this changeset because doing so would require adding additional bitcoin node configuration options, and would not yield a better outcome. I preferred a small, easily reviewable patch that doesn't add a bunch of new config options, with the tradeoff that the fine tuning would have to be done via well-documented operating system specific configurations.

  I tested this patch by running a node with:
  `./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=tcp://127.0.0.1:28332 &`
  and connecting to it with:
  `python3 ./contrib/zmq/zmq_sub.py`

  Without these changes, `ss -panto | grep 28332 | grep ESTAB | grep bitcoin` will report no keepalive timer information. With these changes, the output from the prior command will show keepalive timer information consistent with the configuration at the time of connection establishment, e.g.-: `timer:(keepalive,119min,0)`.

  I also tested with a non-TCP transport and did not witness any adverse effects:
  `./src/qt/bitcoin-qt -regtest -txindex -datadir=/tmp/node -zmqpubhashblock=ipc:///tmp/bitcoin.block &`

ACKs for top commit:
  adamjonas:
    Just to summarize for those looking to review - as of c276df7759 there are 3 tACKs (n-thumann, Haaroon, and dlogemann), 1 "looks good to me" (laanwj) with no NACKs or any show-stopping concerns raised.
  jonasschnelli:
    utACK c276df7759

Tree-SHA512: b884c2c9814e97e666546a7188c48f9de9541499a11a934bd48dd16169a900c900fa519feb3b1cb7e9915fc7539aac2829c7806b5937b4e1409b4805f3ef6cd1
2020-09-02 09:09:18 +02:00
Andrew Chow f1ee37319a wallet: Reload previously loaded wallets on GUI startup
Enable the GUI to also use the load_on_startup feature.
Wallets loaded in the GUI always have load_on_startup=true.
When they are unloaded from the GUI, load_on_startup=false.

To facilitate this change, UpdateWalletSetting is moved into the wallet
module and called from within LoadWallet, RemoveWallet, and
Createwallet. This change does not actually touch the GUI code but
rather the wallet functions that are shared between the GUI and RPC.
2020-09-01 12:13:50 -04:00
Jon Atack d780293e1e
net: improve nLastBlockTime and nLastTXTime documentation
Co-authored-by: John Newbery <john@johnnewbery.com>
2020-09-01 17:46:28 +02:00
Wladimir J. van der Laan 48c1083632
Merge #19105: Add Muhash3072 implementation in Python
36ec9801a4 test: Add chacha20 test vectors in muhash (Fabian Jahr)
0e2b400fea test: Add basic Python/C++ Muhash implementation parity unit test (Fabian Jahr)
b85543cb73 test: Add Python MuHash3072 implementation to test framework (Pieter Wuille)
ab30cece0e test: Move modinv to util and add unit test (Fabian Jahr)

Pull request description:

  This is the second in a [series of pull requests](https://github.com/bitcoin/bitcoin/pull/18000) to implement an Index for UTXO set statistics.

  This pull request adds a Python implementation of Muhash3072, a homomorphic hashing algorithm to be used for hashing the UTXO set. The Python implementation can then be used to compare behavior with the C++ version.

ACKs for top commit:
  jnewbery:
    utACK 36ec9801a
  laanwj:
    Code review ACK 36ec9801a4

Tree-SHA512: a3519c6e11031174f1ae71ecd8bcc7f3be42d7fc9c84c77f2fbea7cfc5ad54fcbe10b55116ad8d9a52ac5d675640eefed3bf260c58a02f2bf3bc0d8ec208baa6
2020-09-01 17:12:20 +02:00
fanquake e36f802fa4
lint: add C++ code linter
This currently only checks for boost::bind usage.

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
2020-09-01 14:23:08 +08:00
MarcoFalke bab4cce1b0
Merge #19668: Do not hide compile-time thread safety warnings
ea74e10acf doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743fe7 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d171e Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150857 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55a72 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)

Pull request description:

  On the way of transit from `RecursiveMutex` to `Mutex` (see #19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.

  On master (65e4ecabd5) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
  ```diff
  --- a/src/txmempool.h
  +++ b/src/txmempool.h
  @@ -607,7 +607,7 @@ public:
       void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);

       void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
  -    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
  +    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
       void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
       void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);

  ```
  Clang compiles the code without any thread safety warnings.

  See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.

ACKs for top commit:
  MarcoFalke:
    ACK ea74e10acf 🎙
  jnewbery:
    ACK ea74e10acf
  ajtowns:
    ACK ea74e10acf

Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447
2020-09-01 08:18:26 +02:00
fanquake a1d14f522c
Merge #19671: wallet: Remove -zapwallettxes
3340dbadd3 Remove -zapwallettxes (Andrew Chow)

Pull request description:

  It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.

  Alternative to #19700

ACKs for top commit:
  meshcollider:
    utACK 3340dbadd3
  fanquake:
    ACK 3340dbadd3 - remaining manpage references will get cleaned up pre-release.

Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
2020-09-01 09:26:28 +08:00
Wladimir J. van der Laan e796fdd4cb
Merge #19507: Expand functional zmq transaction tests
7356292e1d Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983 Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600c Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)

Pull request description:

  Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.

  Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.

  Remaining confusion:
  1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
  2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)

ACKs for top commit:
  laanwj:
    code review ACK 7356292e1d
  promag:
    Code review ACK 7356292e1d.

Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
2020-08-31 20:46:27 +02:00
Andrew Chow 3340dbadd3 Remove -zapwallettxes
-zapwallettxes is made a hidden option to inform users that it is
removed and they should be using abandontransaction to do the stuck
transaction thing.
2020-08-31 12:39:19 -04:00
MarcoFalke 89a8299a14
Merge #19717: rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump)
fa3d9ce325 rpc: Assert that RPCArg names are equal to CRPCCommand ones (rpcdump) (MarcoFalke)
fa32c1d5ec rpc: Assert that RPCArg names are equal to CRPCCommand ones (zmq) (MarcoFalke)
faaa46dc20 rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining) (MarcoFalke)
fa93bc14c7 rpc: Remove unused return type from appendCommand (MarcoFalke)

Pull request description:

  This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:

  ### Motivation

  RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.

  ### Changes

  The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.

  ### Future work

  > Here or follow up, makes sense to also assert type of returned UniValue?

  Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:

  * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
  * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
  * Auto-formatting and sanity checking the RPCExamples with RPCMan
  * Checking passed-in json in self-check. Removing redundant checks
  * Checking returned json against documentation to avoid regressions or false documentation
  * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static

  ### Bugs found

  * The assert identified issue #18607
  * The changes itself fixed bug #19250

ACKs for top commit:
  fjahr:
    tested ACK fa3d9ce325
  promag:
    Code review ACK fa3d9ce325.

Tree-SHA512: 068ade4b55cc195868d53b7f9a27151d45b440857bb069e261a49d102a49a38fdba5d68868516a1d66a54a73ba34681362f934ded7349e894042bde873b75719
2020-08-31 17:43:35 +02:00
MarcoFalke 068bc21188
Merge #19842: Update the vcpkg checkout commit ID in appveyor config
a104caeb40 Update the vcpkg checkout commit ID in appveyor config. (Aaron Clauson)

Pull request description:

  A recent appveyor vm update broke the build of the `berkeleydb` vcpkg dependency, see #19839. The temporary resolution was to switch back to the previous appveyor vm.

  This PR updates the pegged vcpkg commit ID to the most recent commit as of 31 Aug 2020. That commit ID has been tested against the latest appveyor vm and is able to build Bitcoin Core successfully.

  The vcpkg bump includes a [patch](https://github.com/microsoft/vcpkg/pull/12870) to the `berkeleydb` build config which allows it to be built on the latest appveyor vm.

ACKs for top commit:
  MarcoFalke:
    Concept ACK a104caeb40

Tree-SHA512: 6d363d1615c51bb3d4b324eb96d53950648fc97fc81ffaef91ee6e92f1336776d150d89f6e859f354ee75ce66afcef07aa19ed39b725dbb3f47ba67d26e111db
2020-08-31 17:02:20 +02:00
MarcoFalke c1e0c2ad3b
Merge #19813: util, ci: Hard code previous release tarball checksums
0374e821bd util: Hard code previous release tarball checksums (Hennadii Stepanov)
bd897ce79f scripted-diff: Move previous_release.py to test/get_previous_releases.py (Hennadii Stepanov)

Pull request description:

  #19205 introduced signature verifying for the downloaded `SHA256SUMS.asc`.
  This approach is brittle and does not work in CI environment for many reasons:
  - https://github.com/bitcoin/bitcoin/issues/19812#issuecomment-680760663
  - https://github.com/bitcoin/bitcoin/pull/19013#discussion_r459590779

  This PR:
  - implements **Sjors**' [idea](https://github.com/bitcoin/bitcoin/pull/19205#pullrequestreview-426080048):
  > Alternatively we might as well hard code the checksum for each `tar.gz` release in the source code, here.

  - is an alternative to 5a2c31e528e6bd60635096f233252f3c717f366d (#19013)

  - fixes #19812

  - updates v0.17.1 to v0.17.2

ACKs for top commit:
  MarcoFalke:
    cr ACK 0374e821bd
  Sjors:
    tACK 0374e821bd

Tree-SHA512: cacdcf9f5209eae7da357abb3445585ad2f980920fd5bf75527ce89974d3f531a4cf8b5b35edfc116b23bfdfb45c0437cb14cbc416d76ed2dc5b9e6d33cdad71
2020-08-31 16:18:29 +02:00
fanquake c4be50fea3
remove usage of boost::bind
boost::bind usage was removed in #13743. However a new usage snuck in as
part of 2bc4c3eaf9 (#15225).
2020-08-31 19:34:57 +08:00
Samuel Dobson f98872f127
Merge #18244: rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins
6d1f51343c [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost)

Pull request description:

  When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction.

  Note that when  creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC.

  See #7518 for historical background.

ACKs for top commit:
  meshcollider:
    Code review ACK 6d1f51343c
  fjahr:
    Code review ACK 6d1f51343c

Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d
2020-08-31 23:30:53 +12:00
Samuel Dobson 7721b31809
Merge #19773: wallet: Avoid recursive lock in IsTrusted
772ea4844c wallet: Avoid recursive lock in IsTrusted (João Barbosa)
819f10f671 wallet, refactor: Immutable CWalletTx::pwallet (João Barbosa)

Pull request description:

  This change moves `CWalletTx::IsTrusted` to `CWallet` in order to have TSAN. So now `CWallet::IsTrusted` requires `cs_wallet` and the recursive lock no longer happens.

  Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.

ACKs for top commit:
  meshcollider:
    utACK 772ea4844c
  hebasto:
    ACK 772ea4844c, reviewed and tested on Linux Mint 20 (x86_64).

Tree-SHA512: 702ffd928b2f42a8b90de398790649a5fd04e1ac3877558da928e94cdeb19134883f06c3a73a6826c11c912facf199173375a70200737e164ccaea1bec515b2a
2020-08-31 22:45:27 +12:00
MarcoFalke 61b8c04d78
Merge #19379: tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...)
46fcac1e4b tests: Add fuzzing harness for ec_seckey_import_der(...) and ec_seckey_export_der(...) (practicalswift)
b667a90389 tests: Add fuzzing harness for SigHasLowR(...) and ecdsa_signature_parse_der_lax(...) (practicalswift)

Pull request description:

  Add fuzzing harness for `SigHasLowR(...)` and `ecdsa_signature_parse_der_lax(...)`.

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  Crypt-iQ:
    ACK 46fcac1e4b

Tree-SHA512: 11a4856a1efd9a04030a8c8aee2413fd5be1ea248147e649a48a55bacdf732bb48a19ee1ce2761d47d4dd61c9598aec53061b961b319ad824d539dda11a8ccf4
2020-08-31 10:56:34 +02:00