Commit graph

16490 commits

Author SHA1 Message Date
Andrew Chow
27b2766384 walletdb: Move BerkeleyDatabase::Flush(true) to Close()
Instead of having Flush optionally shutdown the database and
environment, add a Close() function that does that.
2020-07-14 11:07:16 -04:00
MarcoFalke
834ac4c0f5
Merge #19323: gui: Fix regression in *txoutset* in GUI console
314b49bd50 gui: Fix regression in GUI console (Hennadii Stepanov)

Pull request description:

  The regression was introduced in #19056: if the GUI is running without `-server=1`, the `*txoutset*` call in the console returns "Shutting down".

  Fix #19255.

ACKs for top commit:
  ryanofsky:
    Code review ACK 314b49bd50. Only change since last review is rebase

Tree-SHA512: 8ff85641a5c249858fecb1ab69c7a1b2850af651ff2a94aa41ce352b5b5bc95bc45c41e1767e871b51e647612d09e4d54ede3e20c313488afef5678826c51b62
2020-07-14 16:33:01 +02:00
MarcoFalke
a924f61cae
Merge #19325: wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class
b82f0ca4d5 walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow)
eac9200814 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow)

Pull request description:

  In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`.

  The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`.

  Part of #18971

  Requires #19308 and #19324

ACKs for top commit:
  Sjors:
    re-utACK b82f0ca4d5
  MarcoFalke:
    ACK b82f0ca4d5 🌘
  meshcollider:
    LGTM, utACK b82f0ca4d5

Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a
2020-07-14 16:21:01 +02:00
MarcoFalke
b26d62c49a
Merge #18990: log: Properly log txs rejected from mempool
fa9f20b647 log: Properly log txs rejected from mempool (MarcoFalke)

Pull request description:

  Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:

  * A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
  * A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally.
  * A rejected orphan transaction will log no failure.

  Fix all issues by simply returning the state to the caller, like it is done for all other rejections.

  The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.

ACKs for top commit:
  naumenkogs:
    utACK fa9f20b647
  rajarshimaitra:
    Concept ACK. Compiled and ran tests. `fa9f20b`
  jnewbery:
    code review ACK fa9f20b647

Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7
2020-07-14 16:15:07 +02:00
Sebastian Falbesoner
0c8461a88e refactor: replace CConnman pointers by references in net_processing.cpp 2020-07-14 16:00:24 +02:00
Jon Atack
fa108d6a75
test: update tests for peer discouragement 2020-07-14 14:15:01 +02:00
John Newbery
ca3585a483 [net/net processing] check banman pointer before dereferencing
Although we currently don't do this, it should be possible to create a
CConnman or PeerLogicValidation without a Banman instance. Therefore
always check that banman exists before dereferencing the pointer.

Also add comments to the m_banman members of CConnman and
PeerLogicValidation to document that these may be nullptr.
2020-07-14 10:24:43 +01:00
Jon Atack
1a9f462caa
gui, doc: rm Ban Score in GUI Peers window/release notes updates 2020-07-14 10:18:17 +02:00
MarcoFalke
07c83ce039
Merge #19268: doc: Add non-thread-safe note to FeeFilterRounder::round()
d842e6ac96 doc: Add non-thread-safe note to FeeFilterRounder::round() (Hennadii Stepanov)

Pull request description:

  The `FastRandomContext` class is documented as not thread-safe.
  This PR adds a relevant note to the `FeeFilterRounder::round()` function declaration.

  Close #19254

ACKs for top commit:
  MarcoFalke:
    self ACK d842e6ac96
  practicalswift:
    ACK d842e6ac96: explicit is better than implicit
  naumenkogs:
    ACK d842e6a

Tree-SHA512: 538508f24b9cb29baece6a64108e2c5fc3960768c6475c4f2baf48a4a7bdb96dcef1a74d21a4822e1f8635e1375362986da4e3a20f5644129046a354c4b0a8a0
2020-07-14 09:30:02 +02:00
MarcoFalke
b93c4244b9
Merge #19464: net: remove -banscore configuration option
06059b0c2a net: rename DEFAULT_BANSCORE_THRESHOLD to DISCOURAGEMENT_THRESHOLD (Jon Atack)
1d4024bca8 net: remove -banscore configuration option (Jon Atack)

Pull request description:

  per https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652684340, https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592. Edit: now split into 3 straightforward PRs:
  - net: remove -banscore configuration option (this PR)
  - rpc: deprecate banscore field in getpeerinfo (#19469, *merged*)
  - gui: no longer display banscores (TBA in the gui repo)

ACKs for top commit:
  MarcoFalke:
    review ACK 06059b0c2a 📙
  vasild:
    ACK 06059b0c

Tree-SHA512: 03fad249986e0896697033fbb8ba2cbfaae7d7603b1fb2a38b3d41db697630d238623f4d732b9098c82af249ce5a1767dd432b7ca0fec10544e23d24fbd57c50
2020-07-14 08:13:25 +02:00
fanquake
5550fa5983
Merge #19109: Only allow getdata of recently announced invs
f32c408f3a Make sure unconfirmed parents are requestable (Pieter Wuille)
c4626bcd21 Drop setInventoryTxToSend based filtering (Pieter Wuille)
43f02ccbff Only respond to requests for recently announced transactions (Pieter Wuille)
b24a17f039 Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille)
a9bc563803 Swap relay pool and mempool lookup (Pieter Wuille)

Pull request description:

  This implements the follow-up suggested here: https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627630111 . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.

  This:

  * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627627010).
  * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see https://github.com/bitcoin/bitcoin/pull/18861#discussion_r419695831).

  It adds 37 KiB of filter per peer.

  This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).

ACKs for top commit:
  jnewbery:
    reACK f32c408f3
  jonatack:
    re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review.
  ajtowns:
    re-ACK f32c408f3a

Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
2020-07-14 08:40:35 +08:00
MarcoFalke
fa8a992589
Work around memory-aliasing in descriptor ParsePubkey 2020-07-13 21:58:10 +02:00
Andrew Chow
d0ea9bab28 walletdb: Don't remove database transaction logs and instead error
Instead of removing the database transaction logs and retrying the
wallet loading, just return an error message to the user. Additionally,
specifically for DB_RUNRECOVERY, notify the user that this could be due
to different BDB versions. This error is pretty much only caused by
compiling with a newer version of BDB and then trying to open the wallet
with a version compiled with an older version of BDB.
2020-07-13 11:00:54 -04:00
Russell Yanofsky
edc316020e test: Remove duplicate NodeContext hacks
Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.

Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.

Non-test code is changing but non-test behavior is still the same as before.

Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
2020-07-13 04:34:27 -04:00
MarcoFalke
d52bfc4916
Merge bitcoin-core/gui#30: Disable the main window toolbar when the modal overlay is shown
d0cc1f6df7 qt: Disable toolbar when overlay is shown (Hennadii Stepanov)
e74cd2083d qt, refactor: Cleanup ModalOverlay slots (Hennadii Stepanov)

Pull request description:

  Keeping the main window toolbar activated while the modal overlay is shown could create the appearance of the non-responsive GUI.

  Fixes #22.

  ---

  On master (ca055885c6):

  ![Screenshot from 2020-07-11 13-07-00](https://user-images.githubusercontent.com/32963518/87221791-7504e100-c377-11ea-9689-ddd4b21b98f9.png)

  With this PR:

  ![Screenshot from 2020-07-11 13-07-39](https://user-images.githubusercontent.com/32963518/87221803-8817b100-c377-11ea-92c8-3602dc4d2451.png)

ACKs for top commit:
  harding:
    Tested ACK d0cc1f6df7.  Tested on Linux/X11 as much as I could given it's a pretty small change; seems like a nice improvement.  I'm not experienced in Qt, but I don't see anything obviously problematic about the code.
  jonatack:
    ACK d0cc1f6 tested on Debian 5.7.6-1 (2020-06-24) x86_64 GNU/Linux
  LarryRuane:
    ACK d0cc1f6df7 tested on Ubuntu 18.04.4 LTS

Tree-SHA512: e371b34231c01e77118deb100e0f280ba1cdef54e317f7f7d6ac322598bda811bd1bfe3035e90d87f8267f4f5d2095d34a8136911159db63694fd1b1b11335a1
2020-07-13 10:34:27 +02:00
MarcoFalke
631284f09a
Merge #19486: Remove unused constants CADDR_TIME_VERSION and GETHEADERS_VERSION
7bb6f9bfdb [protocol] Remove unused GETHEADERS_VERSION (John Newbery)
37a934e6b3 [protocol] Remove unused CADDR_TIME_VERSION (John Newbery)

Pull request description:

  These constants are no longer required and can be removed.

  Additional code comments are added to explain CAddress serialization.

ACKs for top commit:
  MarcoFalke:
    ACK 7bb6f9bfdb already an improvement, but maybe getting rid of INIT_PROTO_VERSION here would be an even stronger improvement (can be done later)
  jonatack:
    ACK 7bb6f9bfdb
  vasild:
    ACK 7bb6f9bf

Tree-SHA512: 5382562c60fd677c86583754eca11aad3719064efe2e5ef4f307d693b583422ca8d385926c2582aaab899f502b151f2eb87a7ac23363b15f4fceaa06296f98e3
2020-07-13 10:32:19 +02:00
Samuel Dobson
4db44acf2d
Merge #18202: refactor: consolidate sendmany and sendtoaddress code
08fc6f6cfc [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)

Pull request description:

  I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.

  Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.

  Salvaged from #18201.

ACKs for top commit:
  fjahr:
    Code review ACK 08fc6f6cfc
  jonatack:
    ACK 08fc6f6cfc
  meshcollider:
    Code review & functional test run ACK 08fc6f6cfc

Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
2020-07-12 14:42:35 +12:00
Samuel Dobson
32302e5c88
Merge #19490: wallet: Fix typo in comments; Simplify assert
facd7dd3d1 wallet: Fix typo in comments; Simplify assert (MarcoFalke)

Pull request description:

  Follow up to https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443783101 and https://github.com/bitcoin/bitcoin/pull/19046#discussion_r443793690

ACKs for top commit:
  practicalswift:
    ACK facd7dd3d1
  jonatack:
    ACK facd7dd3d1
  hebasto:
    ACK facd7dd3d1, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive.

Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
2020-07-12 13:34:37 +12:00
Jon Atack
06059b0c2a
net: rename DEFAULT_BANSCORE_THRESHOLD to DISCOURAGEMENT_THRESHOLD
and move it from validation to net processing.
2020-07-11 19:41:24 +02:00
Jon Atack
1d4024bca8
net: remove -banscore configuration option 2020-07-11 19:41:21 +02:00
MarcoFalke
42fe6aad32
Merge #19493: wallet: Fix clang build in Mac
1e58bcc9af wallet: Fix clang build in Mac (Anthony Fieroni)

Pull request description:

  Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>

Top commit has no ACKs.

Tree-SHA512: 19312929af14dab97c37cf4547fbd6589a6de960f1a499c2118bb684240639af4b127cf8dc4d201b41d253cfbb645614a0606d4ecce29f300b10c210d38a961b
2020-07-11 19:12:43 +02:00
Anthony Fieroni
1e58bcc9af wallet: Fix clang build in Mac
Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
2020-07-11 19:33:57 +03:00
MarcoFalke
fa53635381
util: Make Assert work with any value 2020-07-11 15:02:07 +02:00
MarcoFalke
facd7dd3d1
wallet: Fix typo in comments; Simplify assert 2020-07-11 14:24:36 +02:00
Samuel Dobson
160800ac10
Merge #19441: walletdb: don't reinitialize desc cache with multiple cache entries
a66a7a1a70 walletdb: don't reinitialize desc cache with multiple cache entries (Andrew Chow)

Pull request description:

  When loading descriptor caches, we would accidentally reinitialize the descriptor cache when seeing that one already exists. This should have only been initializing the cache when one does not exist. However this code itself is unnecessary as the act of looking up the cache to add to it will initialize it if it didn't already exist.

  This issue could be hit by trying to load a wallet that had imported a multisig descriptor. The wallet would fail to load.

  A test has been added to wallet_importdescriptors.py to catch this case. Another test case has also been added to check that loading a wallet with only single key descriptors works.

ACKs for top commit:
  hugohn:
    tACK [a66a7a1](a66a7a1a70)
  jonatack:
    ACK a66a7a1a70
  meshcollider:
    Code review ACK a66a7a1a70

Tree-SHA512: 3df746421a008708eaa3bbbdd12b9ddd3e2ec111d54625a212dca7414b971cc1f6e2b1757b3232c31a2f637d1b1ef43bf3ffa4ac4216646cf1e92db5f79954f1
2020-07-12 00:14:27 +12:00
Samuel Dobson
5f96bce9b7
Merge #18923: wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off
fa73493930 refactor: Use C++11 range-based for loop (MarcoFalke)
fa7b164d62 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off (MarcoFalke)
faf8401c19 wallet: Pass unused args to StartWallets (MarcoFalke)
fa6c186436 gui tests: Limit life-time of dummy testing setup (MarcoFalke)
fa28a61897 test: Add smoke test to check that wallets are flushed by default (MarcoFalke)

Pull request description:

  User-facing, this is a refactor. Internally, the scheduler does not have to call a mostly empty function every half a second.

ACKs for top commit:
  jnewbery:
    utACK fa73493930
  meshcollider:
    utACK fa73493930
  ryanofsky:
    Code review ACK fa73493930. Just rebased since last review

Tree-SHA512: 99e1fe1b2c22a3f4b19de3e566241d38693f4fd8d5a68ba1838d86740aa6c08e3325c11a072e30fd262a8861af4278bed52eb9374c85179b8f536477f528247c
2020-07-11 23:23:28 +12:00
Samuel Dobson
89899a3448
Merge #19046: Replace CWallet::Set* functions that use memonly with Add/Load variants
3a9aba21a4 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow)
d9cd095b59 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow)
0122fbab4c Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow)

Pull request description:

  `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet.

  `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet.

  `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk.

ACKs for top commit:
  jnewbery:
    Code review ACK 3a9aba21a4
  ryanofsky:
    Code review ACK 3a9aba21a4. Only changes since last review tweaks making m_wallet_flags updates more safe
  meshcollider:
    utACK 3a9aba21a4

Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
2020-07-11 23:08:54 +12:00
Samuel Dobson
4fc9224ee7
Merge #18850: wallet: Fix ZapSelectTx to sync wallet spends
9c59f9c285 Fix ZapSelectTx to sync wallet spends (Anthony Fieroni)

Pull request description:

  Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>

ACKs for top commit:
  achow101:
    ACK 9c59f9c285
  ryanofsky:
    Code review ACK 9c59f9c285. Only change since last review tweaking the for loop as suggested
  jonatack:
    ACK 9c59f9c285 tested rebased on current master b33136b6ba and the new unit test does indeed fail without the change.
  meshcollider:
    utACK 9c59f9c285

Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
2020-07-11 22:20:43 +12:00
Hennadii Stepanov
d0cc1f6df7
qt: Disable toolbar when overlay is shown 2020-07-11 12:59:58 +03:00
Russell Yanofsky
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage.
Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to
the loadwallet RPC and maintain a dynamic list of wallets that should be loaded
on startup that also can be shared between bitcoind and bitcoin-qt.
2020-07-11 05:41:12 -04:00
MarcoFalke
a42631775a
Merge #19222: tests: Add fuzzing harness for BanMan
97846d7f5b tests: Add fuzzing harness for BanMan (practicalswift)
deba199f1c tests: Add ConsumeSubNet(...). Move and increase coverage in ConsumeNetAddr(...). (practicalswift)

Pull request description:

  Add fuzzing harness for `BanMan`.

  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 :)

Top commit has no ACKs.

Tree-SHA512: f4126c15bbb77638833367d73f58193c8f05d16bed0b1d6c33b39387d5b610ff34af78cd721adb51778062ce3ac5e79756d1c3895ef54c6c80c61dcf056e94ff
2020-07-11 11:41:12 +02:00
Russell Yanofsky
eb682c5700 util: Add ReadSettings and WriteSettings functions
Currently unused, but includes tests.
2020-07-11 05:41:12 -04:00
Hennadii Stepanov
e74cd2083d
qt, refactor: Cleanup ModalOverlay slots 2020-07-11 12:16:29 +03:00
MarcoFalke
ca055885c6
Merge #19474: doc: Use precise permission flags where possible
fab5586122 doc: Use precise permission flags where possible (MarcoFalke)

Pull request description:

  Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.

  This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.

  Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.

ACKs for top commit:
  jnewbery:
    reACK fab5586122
  fjahr:
    Code review ACK fab5586122
  jonatack:
    ACK fab5586122

Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
2020-07-11 10:23:09 +02:00
John Newbery
655b195747 [net processing] Continue SendMessages processing if not disconnecting peer
If we don't disconnect a peer in MaybeDiscourageAndDisconnect because it
has NOBAN permissions or it's a manual connection, continue SendMessages
processing rather than exiting early.

The previous behaviour was that we'd miss the SendMessages processing on
this iteration of the MessageHandler loop. That's not a problem since
SendMessages() would just be called again on the next iteration, but it
was slightly inefficient and confusing.
2020-07-11 07:13:05 +01:00
John Newbery
a49781e56d [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages
`nMisbehavior` is a tally in `CNodeState` that can be incremented from
anywhere. That almost always happens inside a `ProcessMessages()` call
(because we increment the misbehavior score when receiving a bad
messages from a peer), but not always. See, for example, the call to
`MaybePunishNodeForBlock()` inside `BlockChecked()`, which is an
asynchronous callback from the validation interface, executed on the
scheduler thread.

As long as `MaybeDiscourageAndDisconnect()` is called regularly for the
node, then the misbehavior score exceeding the 100 threshold will
eventually result in the peer being punished. It doesn't really matter
where that `MaybeDiscourageAndDisconnect()` happens, but it makes most
sense in `SendMessages()` which is where we do general peer
housekeeping/maintenance.

Therefore, remove the `MaybeDiscourageAndDisconnect()` call in
`ProcessMessages()` and move the `MaybeDiscourageAndDisconnect()` call
in `SendMessages()` to the top of the function. This moves it out of the
cs_main lock scope, so take that lock directly inside
`MaybeDiscourageAndDisconnect()`.

Historic note: `MaybeDiscourageAndDisconnect()` was previously
`SendRejectsAndCheckIfBanned()`, and before that was just sending
rejects.  All of those things required cs_main, which is why
`MaybeDiscourageAndDisconnect()` was called after the ping logic.
2020-07-11 07:06:20 +01:00
Jon Atack
92d94ffb8d
rpc: print useful help and error message for generate 2020-07-11 07:47:57 +02:00
John Newbery
7bb6f9bfdb [protocol] Remove unused GETHEADERS_VERSION
GETHEADERS_VERSION is unused. No need to keep it in the codebase
for just its historic comment.
2020-07-10 22:14:18 +01:00
John Newbery
37a934e6b3 [protocol] Remove unused CADDR_TIME_VERSION
Add comments to CAddress serialization code explaining why
it's no longer needed.
2020-07-10 22:14:18 +01:00
MarcoFalke
505b4eda55
Merge #19140: tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests
20d31bdd92 tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)

Pull request description:

  Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from #19074 as suggested by MarcoFalke.

  The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.

  The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.

  `" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.

  Before this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
  ==27905==The signal is caused by a READ memory access.
  ==27905==Hint: address points to the zero page.
      #0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
      #1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
      #2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
  …
  $ echo $?
  1
  ```

  After this PR:

  ```
  $ echo " http:// HTTP/1.1" > input
  $ src/test/fuzz/http_request input
  src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
  Running: input
  Executed input in 0 ms
  ***
  *** NOTE: fuzzing was not performed, you have only
  ***       executed the target code on a fixed set of inputs.
  ***
  $ echo $?
  0
  ```

  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 :)

Top commit has no ACKs.

Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
2020-07-10 22:51:56 +02:00
MarcoFalke
5802ea6bd3
Merge #19453: refactor: reduce DefaultRequestHandler memory allocations
f20b359bb9 cli: reduce DefaultRequestHandler memory allocations (Jon Atack)

Pull request description:

  per https://github.com/bitcoin/bitcoin/pull/16439#discussion_r443957125. Simpler code, fewer allocations. No change of behavior. The code has good test coverage in `interface_bitcoin_cli.py`.

ACKs for top commit:
  MarcoFalke:
    review ACK f20b359bb9
  fjahr:
    Code review ACK f20b359

Tree-SHA512: 745eab44dfdcc485ca2bbc1db8b8d364cbd3cf94982e46e033745ce05ab617c15320ee55da7fb930d365f4d26b172049d5f5efcf0b6d3af5b0a28185bdb93ea8
2020-07-10 22:41:31 +02:00
John Newbery
a1d5a428a2 [net processing] Fix bad indentation in SendMessages()
Hint for reviewers: review ignoring whitespace changes.
2020-07-10 18:20:07 +01:00
John Newbery
1a1c23f8d4 [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages()
This was changed to TRY_LOCK in #1117 to fix a potential deadlock
between cs_main and cs_vSend. cs_vSend was split into cs_vSend and
cs_sendProcessing in #9535 (and cs_sendProcessing was changed from a
TRY_LOCK to a LOCK in the same PR).

Since cs_vSend can no longer be taken before cs_main, revert this to a
LOCK().

This commit leaves part of the code with bad indentation. That is fixed
by the next (whitespace change only) commit.
2020-07-10 18:20:07 +01:00
MarcoFalke
c0b0b0240f
Merge #14033: p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater
57b0c0a93a Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater (Ben Woosley)

Pull request description:

  We do not connect to peers older than 31800

ACKs for top commit:
  sipa:
    Code reivew ACK 57b0c0a93a
  jnewbery:
    Code review ACK 57b0c0a93a
  vasild:
    ACK 57b0c0a9

Tree-SHA512: e1ca7c9203cbad83ab7c7a2312777ad07ed6a16119169b256648b8a8738c260a5168acdd4fb33f6e4b17f51ec7e033e110b76bde55b4e3b2d444dc02c01bc2b1
2020-07-10 19:16:48 +02:00
MarcoFalke
107b8559c5
Merge #18638: net: Use mockable time for ping/pong, add tests
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)

Pull request description:

  Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.

  Mockable time is also type-safe, since it uses `std::chrono`

ACKs for top commit:
  jonatack:
    Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
  troygiorshev:
    ACK fa3365430c

Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
2020-07-10 16:06:28 +02:00
MarcoFalke
fab5586122
doc: Use precise permission flags where possible 2020-07-10 15:37:42 +02:00
Vasil Dimov
bc74a40a56
net: improve encapsulation of CNetAddr
Do not access `CNetAddr::ip` directly from `CService` methods.

This improvement will help later when we change the type of
`CNetAddr::ip` (in the BIP155 implementation).

Co-authored-by: Carl Dong <contact@carldong.me>
2020-07-10 14:58:42 +02:00
fanquake
a4eb6a51a7
Merge #19469: rpc: deprecate banscore field in getpeerinfo
41d55d3057 doc: getpeerinfo banscore deprecation release note (Jon Atack)
dd54e3796e test: getpeerinfo banscore deprecation test (Jon Atack)
8c7647b3fb rpc: deprecate banscore field in rpc getpeerinfo (Jon Atack)

Pull request description:

  Per https://github.com/bitcoin/bitcoin/pull/19219#discussion_r443074487 and https://github.com/bitcoin/bitcoin/pull/19219#issuecomment-652699592, this PR deprecates returning the `banscore` field in the `getpeerinfo` RPC, updates the help, adds a test, and updates the release notes. Related to #19464.

ACKs for top commit:
  fanquake:
    ACK 41d55d3057

Tree-SHA512: 8eca08332581e2fe191a2aafff6ba89ce39413f0491ed0de8b86577739f0ec430b1a8fbff2914b0f3138a229563dfcc1981c0cf5b7dd6061b5c48680a28423bc
2020-07-10 13:45:48 +08:00
MarcoFalke
2aaff4813c
Merge #19283: refactor: Remove unused BlockAssembler::pblock member var
fa6d5ab674 refactor: Remove unused BlockAssembler::pblock member var (MarcoFalke)

Pull request description:

  It seems odd to have a confusing and fragile "convenience pointer" member variable to be able to write `pblock->vtx` instead of `pblocktemplate->block.vtx` in a single place.

ACKs for top commit:
  promag:
    Code review ACK fa6d5ab674.

Tree-SHA512: e9f032b5ab702dbefffd370db3768ebfb95c13acc732972b695281ea34c91d70cd0a1700bc2c6f106dbc9de68e81bc6bb06c68c2afd53c17cba8ebee4f9931b9
2020-07-09 20:24:17 +02:00
Andrew Chow
b82f0ca4d5 walletdb: Add MakeBatch function to BerkeleyDatabase and use it
Instead of having WalletBatch construct the BerkeleyBatch, have
BerkeleyDatabase do it and return a std::unique_ptr<BerkeleyBatch>
2020-07-09 11:43:54 -04:00