50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
- Mempool tracks locally submitted transactions (wallet or rpc)
- Transactions are removed from set when the node receives a GETDATA request
from a peer, or if the transaction is removed from the mempool.
a9ecbdfcaa test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034996 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)
Pull request description:
This PR fixes https://github.com/bitcoin/bitcoin/issues/18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:
c0b389b335/src/net.h (L812)
and after a loaded filter is removed again through a `filterclear` message:
c0b389b335/src/net_processing.cpp (L3201)
The behaviour was introduced by commit 37c6389c5a (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/18515), according to gmaxwell).
This default match-everything filter leads to some unintended side-effects:
1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue #18483 (strictly speaking, this is a violation of BIP37) c0b389b335/src/net_processing.cpp (L1504-L1507)
2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) c0b389b335/src/net_processing.cpp (L3182-L3186)
This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.
Here is a before/after comparison in behaviour:
| code part / scenario | master branch | PR branch |
| --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
| `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload` |
| `filteradd` processing, no filter was loaded | nothing | peer's banscore increases by 100 (i.e. disconnect) |
On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.
ACKs for top commit:
MarcoFalke:
re-ACK a9ecbdfcaa, only change is in test code 🕙
Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6
fa1da3d4bf test: Add basic addr relay test (MarcoFalke)
fa1793c1c4 net: Pass connman const when relaying address (MarcoFalke)
fa47a0b003 net: Make addr relay mockable (MarcoFalke)
Pull request description:
As usual:
* Switch to std::chrono time to be type-safe and mockable
* Add basic test that relies on mocktime to add code coverage
ACKs for top commit:
naumenkogs:
utACK fa1da3d
promag:
ACK fa1da3d4bf (fabe56e44b6f683e24e37246a7a8851190947cb3 before https://github.com/bitcoin/bitcoin/pull/18454#issuecomment-607866453), fa5bf23d527a450e72c2bf13d013e5393b664ca3 was dropped since last review.
Tree-SHA512: 0552bf8fcbe375baa3cab62acd8c23b2994efa47daff818ad1116d0ffaa0b9e520dc1bca2bbc68369b25584e85e54861fe6fd0968de4f503b95439c099df9bd7
Previously, a default match-everything bloom filter was set for every peer,
i.e. even before receiving a 'filterload' message and after receiving a
'filterclear' message code branches checking for the existence of the filter
by testing the pointer "pfilter" were _always_ executed.
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
In PeerLogicValidation::PeerLogicValidation() we would schedule a lambda
function to execute later, capturing the local variable
`consensusParams` by reference.
Presumably this was considered safe because `consensusParams` is a
reference itself to a global variable which is not supposed to change,
but it can in tests.
Fixes https://github.com/bitcoin/bitcoin/issues/18372
fa36f3a295 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cf scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)
Pull request description:
Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`
ACKs for top commit:
vasild:
ACK fa36f3a (code review, not tested)
ajtowns:
ACK fa36f3a295
jonatack:
ACK fa36f3a
Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
a029e18c2b Use rolling bloom filter of recent block tx's for AlreadyHave() check (Suhas Daftuar)
Pull request description:
In order to determine whether to download or process a relayed transaction, we first try to check whether we already have the transaction -- either in the mempool, in our filter of recently rejected transactions, in our orphan pool, or already confirmed in a block.
Prior to this commit, the heuristic for checking whether a transaction was confirmed in a block is based on whether there's a coin cache entry corresponding to the 0- or 1-index vout of the tx. While that is a quick check, it is very imprecise (eg if those outputs were already spent in another block, we wouldn't detect that the transaction has already been confirmed) -- we can do better by just keeping a rolling bloom filter of the transactions in recent blocks, which will better capture the case of a transaction which has been confirmed and then fully spent.
This should reduce the bandwidth that we waste by requesting transactions which will not be accepted to the mempool.
To avoid relay problems for transactions which have been included in a recent block but then reorged out of the chain, we clear the bloom filter whenever a block is disconnected.
ACKs for top commit:
MarcoFalke:
re-ACK a029e18c2b only stylistic and comment fixups 🍴
sipa:
utACK a029e18c2b
jonatack:
Code review ACK a029e18c2b also built/ran tests and am running bitcoind with mempool debug logging and custom logging. Looked a bit into CRollingBloomFilter and also the mempool median time past checks mentioned above; I don't have a deep understanding of those areas yet but the concept here and changes LGTM. Tests and other optimisations could be added as a follow-up. In favor of seeing this move forward if no major immediate concerns.
Tree-SHA512: 784c9a35bcd3af5db469063ac7d26b4bac430e451e5637a34d8a538c3ffd1433abdd3f06e5584e7a84bfa9e791449e61819397b5a6c7890fa59d78ec3ba507b2
In order to determine whether to download or process a relayed transaction, we
try to determine if we already have the transaction, either in the mempool, in
our recently rejected filter, in our orphan pool, or already confirmed in the
chain itself.
Prior to this commit, the heuristic for checking the chain is based on whether
there's an output corresponding to the 0- or 1-index vout in our coin cache.
While that is a quick check, it is very imprecise (say if those outputs were
already spent in a block) -- we can do better by just keeping a rolling bloom
filter of the transactions in recent blocks, which will capture the case of a
transaction which has been confirmed and then fully spent already.
To avoid relay problems for transactions which have been included in a recent
block but then reorged out of the chain, we clear the bloom filter whenever a
block is disconnected.
Identified via -Wdocumentation, e.g.:
./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
* @param prevTxs Array of previous txns outputs that tx depends on but may not yet be in the block chain
^~~~~~~
prevTxsUnival
netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
* @param outProxyConnectionFailed[out] Whether or not the connection to the
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
outProxyConnectionFailed
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
4bdd68f301 Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb6 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
Pull request description:
Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
ACKs for top commit:
MarcoFalke:
re-ACK 4bdd68f301 (only change is adding includes) 🕕
promag:
ACK 4bdd68f301, could squash.
Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).
1a8f0d5a74 [tools] update nNextInvSend to use mockable time (Amiti Uttarwar)
4de630354f [tools] add PoissonNextSend method that returns mockable time (Amiti Uttarwar)
Pull request description:
Introduce a Poisson helper method that wraps the existing method to return `std::chrono::duration` type, which is mockable.
Needed for https://github.com/bitcoin/bitcoin/pull/16698.
ACKs for top commit:
ajtowns:
ACK 1a8f0d5a74
MarcoFalke:
re-ACK 1a8f0d5a74
naumenkogs:
ACK 1a8f0d5, and let's merge it and come back to it later.
Tree-SHA512: 7e2325d7c55fc0b4357cb86b83e0c218ba269f678c1786342d8bc380bfd9696373bc24ff124b9ff17a6e761c62b2b44ff5247c3911e2afdc7cc5c20417e8290b
b6d2183858 Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c added asserts to check m_addr_known when it's used (User)
090b75c14b p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d
amitiuttarwar:
code review ACK 3004d5a12d. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
ed2dc5e48a Add override/final modifiers to V1TransportDeserializer (Pieter Wuille)
f342a5e61a Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille)
6a91499496 Remove oversized message detection from log and interface (Pieter Wuille)
b0e10ff4df Force CNetMessage::m_recv to use std::move (Jonas Schnelli)
efecb74677 Use adapter pattern for the network deserializer (Jonas Schnelli)
1a5c656c31 Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli)
6294ecdb8b Refactor: split network transport deserializing from message container (Jonas Schnelli)
Pull request description:
**This refactors the network message deserialization.**
* It transforms the `CNetMessage` into a transport protocol agnostic message container.
* A new class `TransportDeserializer` (unique pointer of `CNode`) is introduced, handling the network buffer reading and the decomposing to a `CNetMessage`
* **No behavioral changes** (in terms of disconnecting, punishing)
* Moves the checksum finalizing into the `SocketHandler` thread (finalizing was in `ProcessMessages` before)
The **optional last commit** makes the `TransportDeserializer` following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.
Intentionally not touching the sending part.
Pre-Requirement for BIP324 (v2 message transport protocol).
Replacement for #14046 and inspired by a [comment](https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-431528330) from sipa
ACKs for top commit:
promag:
Code review ACK ed2dc5e48a.
marcinja:
Code review ACK ed2dc5e48a
ryanofsky:
Code review ACK ed2dc5e48a. 4 cleanup commits added since last review. Unaddressed comments:
ariard:
Code review and tested ACK ed2dc5e.
Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d
168b781fe7 Continue relaying transactions after they expire from mapRelay (Anthony Towns)
Pull request description:
This change allows peers to request transactions even after they've expired from mapRelay and even if they're not doing mempool requests. This is intended to allow for CPFP of old transactions -- if parent tx P wasn't relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.
ACKs for top commit:
MarcoFalke:
re-ACK 168b781fe7 (only change is comment fixup)
sdaftuar:
re-ACK 168b781fe7
sipa:
ACK 168b781fe7
Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
9075d13153 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34 [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99 [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
Because the call to MaybePunishNode() in
PeerLogicValidation::BlockChecked() only previously happened if the
REJECT code was > 0 and < REJECT_INTERNAL, then there are cases were
MaybePunishNode() can get called where it wasn't previously:
- when AcceptBlockHeader() fails with CACHED_INVALID.
- when AcceptBlockHeader() fails with BLOCK_MISSING_PREV.
Note that BlockChecked() cannot fail with an 'internal' reject code. The
only internal reject code was REJECT_HIGHFEE, which was only set in
ATMP.
This change restores the behaviour pre-commit
5d08c9c579 which did punish nodes that
sent us CACHED_INVALID and BLOCK_MISSING_PREV blocks.
fa25f43ac5 p2p: Remove BIP61 reject messages (MarcoFalke)
Pull request description:
Reject messages (BIP 61) appear in the following settings:
* Parsing of reject messages (in case `-debug=net` is set, off by default). This has only been used for a single `LogPrint` call for several releases now. Such logging is completely meaningless to us and should thus be removed.
* The sending of reject messages (in case `-enablebip61` is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use `-printtoconsole` to have it as stream, or read from the `debug.log` file like our python function `assert_debug_log` in the test framework does)
Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.
ACKs for top commit:
jnewbery:
utACK fa25f43ac5
laanwj:
I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5.
ryanofsky:
Code review ACK fa25f43ac5
Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
Recent questions have come up regarding dynamic service registration
(see https://github.com/bitcoin/bitcoin/pull/16442#discussion_r308702676
and the assumeutxo project, which needs to dynamically flip NODE_NETWORK).
While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
0ba08020c9 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bb Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd42 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba822 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29 Remove unused variable (Suhas Daftuar)
Pull request description:
Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.
(b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)
ACKs for top commit:
sipa:
ACK 0ba08020c9
ajtowns:
ACK 0ba08020c9 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
TheBlueMatt:
re-utACK 0ba08020c9. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
jnewbery:
utACK 0ba08020c9
jamesob:
ACK 0ba08020c9
Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.
Network topology is better kept private for (at least) two reasons:
(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.
(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).
We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.
After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
582d2cd747 Cover UTXO set access with lock annotations (James O'Beirne)
5693530685 refactor: have CCoins* data managed under CChainState (James O'Beirne)
fae6ab6aed refactor: pcoinsTip -> CChainState::CoinsTip() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change encapsulates UTXO set data within CChainState instances, removing global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a CCoins* hierarchy.
This commit could be broken into smaller pieces, but it would require more ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor invocations.
ACKs for top commit:
Sjors:
reACK 582d2cd747
MarcoFalke:
ACK 582d2cd747
Tree-SHA512: ec9d904fe5dca8cd2dc4b7916daa5d8bab30856dd4645987300f905e0a19f9919fce4f9d1ff03eda982943ca73e6e9a746be6cf53b46510de36e8c81a1eafba1
fab3658356 [qa] Test that getdata requests work as expected (Suhas Daftuar)
fa883ab35a net: Use mockable time for tx download (MarcoFalke)
Pull request description:
Two commits:
* First commit changes to mockable time for tx download (refactoring, should only have an effect on regtest)
* Second commit adds a test that uses mocktime to test tx download
ACKs for top commit:
laanwj:
code review ACK 16197/commits/fab365835639a3da03f8ad9a58a0db6c6c4c2314
jamesob:
ACK fab3658356
Tree-SHA512: 3a64a3e283ec4bab1f6e506404b11f0a564a5b61d2a7508ae738a61f035e57220484c66e0ae47d847fe9f7e3ff5cc834909d7b34a9bbcea6abe01f8742806908
dddd9270f8 net: Document what happens to getdata of unknonw type (MarcoFalke)
Pull request description:
Any getdata of unknown type will never be processed and blocks all future messages from a peer. This isn't obviously clear from reading the code, so document it.
Top commit has no ACKs.
Tree-SHA512: 4f8e43bbe6534242facfcfffae28b7a6aa2d228841fa2146a87d494e69f614b0da23cf7a5f3d4367358a7c1981fe2ec196a21c437ae1653f1c7e0351be22598a
0959d37e3e Don't use global (external) symbols for symbols that are used in only one translation unit (practicalswift)
Pull request description:
Don't use global (external) symbols for symbols that are used in only one translation unit.
Before:
```
$ for SYMBOL in $(nm src/bitcoind | grep -E ' [BD] ' | c++filt | cut -f3- -d' ' | grep -v @ | grep -v : | sort | grep '[a-z]' | sort -u | grep -vE '(^_|typeinfo|vtable)'); do
REFERENCES=$(git grep -lE "([^a-zA-Z]|^)${SYMBOL}([^a-zA-Z]|\$)" -- "*.cpp" "*.h")
N_REFERENCES=$(wc -l <<< "${REFERENCES}")
if [[ ${N_REFERENCES} > 1 ]]; then
continue
fi
echo "Global symbol ${SYMBOL} is used in only one translation unit: ${REFERENCES}"
done
Global symbol g_chainstate is used in only one translation unit: src/validation.cpp
Global symbol g_ui_signals is used in only one translation unit: src/ui_interface.cpp
Global symbol instance_of_cmaincleanup is used in only one translation unit: src/validation.cpp
Global symbol instance_of_cnetcleanup is used in only one translation unit: src/net.cpp
Global symbol instance_of_cnetprocessingcleanup is used in only one translation unit: src/net_processing.cpp
Global symbol pindexBestForkBase is used in only one translation unit: src/validation.cpp
Global symbol pindexBestForkTip is used in only one translation unit: src/validation.cpp
$
```
After:
```
$ for SYMBOL in $(nm src/bitcoind | grep -E ' [BD] ' | c++filt | cut -f3- -d' ' | grep -v @ | grep -v : | sort | grep '[a-z]' | sort -u | grep -vE '(^_|typeinfo|vtable)'); do
REFERENCES=$(git grep -lE "([^a-zA-Z]|^)${SYMBOL}([^a-zA-Z]|\$)" -- "*.cpp" "*.h")
N_REFERENCES=$(wc -l <<< "${REFERENCES}")
if [[ ${N_REFERENCES} > 1 ]]; then
continue
fi
echo "Global symbol ${SYMBOL} is used in only one translation unit: ${REFERENCES}"
done
$
```
♻️ Think about future generations: save the global namespace from unnecessary pollution! ♻️
ACKs for commit 0959d3:
Empact:
ACK 0959d37e3e
MarcoFalke:
ACK 0959d37e3e
hebasto:
ACK 0959d37e3e
promag:
ACK 0959d37.
Tree-SHA512: 722f66bb50450f19b57e8a8fbe949f30cd651eb8564e5787cbb772a539bf3a288c048dc49e655fd73ece6a46f6dafade515ec4004729bf2b3ab83117b7c5d153
308b76732f Fix bug around transaction requests (Suhas Daftuar)
f635a3ba11 Expire old entries from the in-flight tx map (Suhas Daftuar)
e32e08407e Remove NOTFOUND transactions from in-flight data structures (Suhas Daftuar)
23163b7593 Add an explicit memory bound to m_tx_process_time (Suhas Daftuar)
218697b645 Improve NOTFOUND comment (Suhas Daftuar)
Pull request description:
#14897 introduced several bugs that could lead to a node no longer requesting transactions from one or more of its peers. Credit to ajtowns for originally reporting many of these bugs along with an originally proposed fix in #15776.
This PR does a few things:
- Fix a bug in NOTFOUND processing, where the in-flight map for a peer was keeping transactions it shouldn't
- Eliminate the possibility of a memory attack on the CNodeState `m_tx_process_time` data structure by explicitly bounding its size
- Remove entries from a peer's in-flight map after 10 minutes, so that we should always eventually resume transaction requests even if there are other bugs like the NOTFOUND one
- Fix a bug relating to the coordination of request times when multiple peers announce the same transaction
The expiry mechanism added here is something we'll likely want to remove in the future, but is belt-and-suspenders for now to try to ensure we don't have other bugs that could lead to transaction relay failing due to some unforeseen conditions.
ACKs for commit 308b76:
ajtowns:
utACK 308b76732f
morcos:
light ACK 308b767
laanwj:
Code review ACK 308b76732f
jonatack:
Light ACK 308b76732f.
jamesob:
ACK 308b76732f
MarcoFalke:
ACK 308b76732f (Tested two of the three bugs this pull fixes, see comment above)
jamesob:
Concept ACK 308b76732f
MarcoFalke:
ACK 308b76732f
Tree-SHA512: 8865dca5294447859d95655e8699085643db60c22f0719e76e961651a1398251bc932494b68932e33f68d4f6084579ab3bed7d0e7dd4ac6c362590eaf9414eda
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
403e677c9 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne)
3ccbc376d refactoring: FlushStateToDisk -> CChainState (James O'Beirne)
4d6688603 refactoring: introduce ChainstateActive() (James O'Beirne)
d7c97edee move-only: make the CChainState interface public (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This changeset starts moving functionality intimately related to CChainState into methods. Parameterizing these functions by a particular CChainState is necessary for the use of multiple chainstates simultaneously (e.g. for asynchronous background validation).
In this change, we
- make the CChainState interface public - since other units will start to invoke its methods directly,
- introduce `::ChainstateActive()`, the CChainState equivalent for `::ChainActive()`,
- and move `IsInitialBlockDownload()` and `FlushStateToDisk()` into methods on CChainState.
Independent of assumeutxo, these changes better encapsulate chainstate behavior and allow easier use from a testing context.
There are more methods that we'll move in the future, but they require other substantial changes (i.e. moving ownership of the `CCoinsView*` hierarchy into CChainState) so we'll save them for future PRs.
---
The first move-only commit is most easily reviewed with `git diff ... --color-moved=dimmed_zebra`.
ACKs for commit 403e67:
Empact:
utACK 403e677c9e no need to address my nits herein
Sjors:
utACK 403e677
ryanofsky:
utACK 403e677c9e. Only change since previous review is removing global state comment as suggested.
MarcoFalke:
utACK 403e677c9e, though the diff still seems a bit bloated with some unnecessary changes in the second commit.
promag:
utACK 403e677 and rebased with current [master](c7cfd20a7).
Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257
This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:
CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
If a transaction is already in-flight when a peer announces a new tx to us, we
schedule a time in the future to reconsider whether to download. At that future
time, there was a bug that would prevent transactions from being rescheduled
for potential download again (ie if the transaction was still in-flight at the
time of reconsideration, such as from some other peer). Fix this.
If a peer hasn't responded to a getdata request, eventually time out the request
and remove it from the in-flight data structures. This is to prevent any bugs in
our handling of those in-flight data structures from filling up the in-flight
map and preventing us from requesting more transactions (such as the NOTFOUND
bug, fixed in a previous commit).
Co-authored-by: Anthony Towns <aj@erisian.com.au>
fa2b52af32 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 fa2b52af32
ryanofsky:
utACK fa2b52af32. 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
486c1eea86 refactoring: remove unused chainActive (James O'Beirne)
631940aab2 scripted-diff: replace chainActive -> ::ChainActive() (James O'Beirne)
a3a609079c refactoring: introduce unused ChainActive() (James O'Beirne)
1b6e6fcfd2 rename: CChainState.chainActive -> m_chain (James O'Beirne)
Pull request description:
This is part of the assumeutxo project:
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change refactors the `chainActive` reference into a `::ChainActive()` call. It also distinguishes `CChainState`'s `CChain` data member as `m_chain` instead of the current `chainActive`, which makes it easily confused with the global data.
The active chain must be obtained via function because its reference will be swapped at some point during runtime after loading a UTXO snapshot.
This change, though lengthy, should be pretty easy to review since most of it is contained within a scripted-diff. Once merged, the parent PR should be easier to review.
ACKs for commit 486c1e:
Sjors:
utACK 486c1ee
promag:
utACK 486c1ee.
practicalswift:
utACK 486c1eea86
Tree-SHA512: 06ed8f9e77f2d25fc9bea0ba86436d80dbbce90a1e8be23e37ec4eeb26060483e60b4a5c4fba679cb1867f61e3921c24abeb9cabdfb4d0a9b1c4ddd77b17456a
Though at the moment ChainActive() simply references `g_chainstate.m_chain`,
doing this change now clears the way for multiple chainstate usage and allows
us to script the diff.
-BEGIN VERIFY SCRIPT-
git grep -l "chainActive" | grep -E '(h|cpp)$' | xargs sed -i '/chainActive =/b; /extern CChain& chainActive/b; s/\(::\)\{0,1\}chainActive/::ChainActive()/g'
-END VERIFY SCRIPT-
This is a first step towards cleaning up our DoS interface - make
validation return *why* something is invalid, and let net_processing
figure out what that implies in terms of banning/disconnection/etc.
Behavior change: peers will now be banned for providing blocks
with premature coinbase spends.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
Isolate the decision of whether to ban a peer to one place in the
code, rather than having it sprinkled throughout net_processing.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Suhas Daftuar <sdaftuar@gmail.com>
John Newbery <john@johnnewbery.com>
This prevents a bug where the in-flight queue for our peers will not be
drained, resulting in not downloading any new transactions from our peers.
Thanks to ajtowns for reporting this bug.
Previously there was an implicit bound based on the handling of m_tx_announced,
but that approach is error-prone (particularly if we start automatically
removing things from that set).
Adds the following util units and adds them to libbitcoin_util:
- `util/url.cpp` takes `urlDecode` from `httpserver.cpp`
- `util/error.cpp` takes `TransactionErrorString` from
`node/transaction.cpp` and `AmountHighWarn` and `AmountErrMsg` from
`ui_interface.cpp`
- `util/fees.cpp` takes `StringForFeeReason` and `FeeModeFromString` from `policy/fees.cpp`
- `util/rbf.cpp` takes `SignalsOptInRBF` from `policy/rbf.cpp`
- 'util/validation.cpp` takes `FormatStateMessage` and `strMessageMagic` from 'validation.cpp`
Removes the now-unused Broadcast/ResendWalletTransactions interface from
validationinterface.
The wallet_resendwallettransactions.py needs a sleep added at the start
to make sure that the rebroadcast scheduler is warmed up before the next
block is mined.
fa8548c5d1 net: Remove unused unsanitized user agent string CNode::strSubVer (MarcoFalke)
Pull request description:
I fail to see a use case for this unsanitized byte array. In fact this can easily be confused with `cleanSubVer` and be displayed to the user (or logged) by a simple typo that is hard to find in review.
Further reading: https://btcinformation.org/en/developer-reference#version
ACKs for commit fa8548:
promag:
utACK fa8548c, good catch.
practicalswift:
utACK fa8548c5d1
sipa:
utACK fa8548c5d1
Tree-SHA512: 3c3ff1504d1583ad099df9a6aa761458a82ec48a58ef7aaa9b5679a5281dd1b59036ba2932ed708488951a565b669a3083ef70be5a58472ff8677b971162ae2f
This makes orphan processing work like handling getdata messages:
After every actual transaction validation attempt, interrupt
processing to deal with messages arriving from other peers.
ef0019e054 Generate log entry when blocks messages are received unexpectedly. (Patrick Strateman)
Pull request description:
Currently these are incorrectly logged as an unknown command.
Tree-SHA512: dd272388a90b79897f8c1ea6d4c949323fcf75493f3a5b2ec9a26a2cf6a8ee743b497941702f21df8fae0f5b9481444363643379832dbd5053b0cc0b0363de04
7257353b93 Select orphan transaction uniformly for eviction (Pieter Wuille)
Pull request description:
The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.
Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
These are separate events which need to be carried out by separate subsystems.
This also cleans up some whitespace and tabs in qt to avoid getting flagged by
the linter.
Current behavior is preserved.
b191c7dfb7 doc: add comment explaining recentRejects-DoS behavior (James O'Beirne)
Pull request description:
When we receive invalid txs for the first time, we mark the sender as
misbehaving. If we receive the same tx before a new block is seen, we *don't*
punish the second sender (in the same way we do the original sender). It wasn't
initially clear to me that this is intentional, so add a clarifying comment.
Tree-SHA512: d12c674db137ed3ad83e0b941bffe6ddcd2982238048742afa574a4235881f0e58cfc0a4a576a0503e74c5c5240c270b9520fa30221e8b43a371fb3e0b37066b
When we receive invalid txs for the first time, we mark the sender as
misbehaving. If we receive the same tx before a new block is seen, we *don't*
punish the second sender (in the same way we do the original sender). It wasn't
initially clear to me that this is intentional, so add a clarifying comment.
66b3fc5437 Skip stale tip checking if outbound connections are off or if reindexing. (Gregory Maxwell)
Pull request description:
I got tired of the pointless stale tip notices in reindex and on nodes with connections disabled.
Tree-SHA512: eb07d9c5c787ae6dea02cdd1d67a48a36a30adc5ccc74d6f1c0c7364d404dc8848b35d2b8daf5283f7c8f36f1a3c463aacb190d70a22d1fe796a301bb1f03228
f34c8c466a Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)
Pull request description:
Make objects in range declarations immutable by default.
Rationale:
* Immutable objects are easier to reason about.
* Prevents accidental or hard-to-notice change of value.
Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
fa6c3dea42 p2p: Clarify control flow in ProcessMessage() (MarcoFalke)
Pull request description:
`ProcessMessage` is effectively a massive switch case construct. In the past there were attempts to clarify the control flow in `ProcessMessage()` by moving each case into a separate static function (see #9608). It was closed because it wasn't clear if moving each case into a function was the right approach.
Though, we can quasi treat each case as a function by adding a return statement to each case. (Can be seen as a continuation of bugfix #13162)
This patch does exactly that.
Also note that this patch is a subset of previous approaches such as #9608 and #10145.
Review suggestion: `git diff HEAD~ --function-context`
Tree-SHA512: 91f6106840de2f29bb4f10d27bae0616b03a91126e6c6013479e1dd79bee53f22a78902b631fe85517dd5dc0fa7239939b4fefc231851a13c819458559f6c201
e254ff5d53 Introduce a maximum size for locators. (Gregory Maxwell)
Pull request description:
The largest sensible size for a locator is log in the number of blocks.
But, as noted by Coinr8d on BCT a maximum size message could encode a
hundred thousand locators. If height were used to limit the messages
that could open new attacks where peers on long low diff forks would
get disconnected and end up stuck.
Ideally, nodes first first learn to limit the size of locators they
send before limiting what would be processed, but common implementations
back off with an exponent of 2 and have an implicit limit of 2^32
blocks, so they already cannot produce locators over some size.
Locators are cheap to process so allowing a few more is harmless,
so this sets the maximum to 64-- which is enough for blockchains
with 2^64 blocks before the get overhead starts increasing.
Tree-SHA512: da28df9c46c988980da861046c62e6e7f93d0eaab3083d32e408d1062f45c00316d5e1754127e808c1feb424fa8e00e5a91aea2cc3b80326b71c148696f7cdb3
The largest sensible size for a locator is log in the number of blocks.
But, as noted by Coinr8d on BCT a maximum size message could encode a
hundred thousand locators. If height were used to limit the messages
that could open new attacks where peers on long low diff forks would
get disconnected and end up stuck.
Ideally, nodes first first learn to limit the size of locators they
send before limiting what would be processed, but common implementations
back off with an exponent of 2 and have an implicit limit of 2^32
blocks, so they already cannot produce locators over some size.
This sets the limit to an absurdly high amount of 101 in order to
maximize compatibility with existing software.
fa4bf92be9 Remove dead service bits code (MarcoFalke)
Pull request description:
Seems fine to remove for the upcoming 0.17 release
Fixes#10993
Tree-SHA512: 3a4664b787e3da399bcaaba693619bd384826df14f469dbdfbbfffc540d9da3f2b322cda262b43388376785f77907c2540541c239ab0fca82bd7eb69d02b6b7a
d45b344ffd Bucket for inbound when scheduling invs to hide tx time (Gleb)
Pull request description:
It has been brought up to my attention that current random delays mechanism (originally intended to obfuscate transaction metadata) allows to easily estimate the time a transaction was received by a node.
It may be done by connecting multiple observer nodes to the same node. Each of those nodes will generate its own schedule of delays. Combined metadata regarding those events from different sources allows an observer to estimate transaction time.
After this patch a spy won't gain additional information by just creating multiple connections to a target.
Tree-SHA512: c71dae5ff350b614cb40a8e201fd0562d3e03e3e72a5099718cd451f0d84c66d5e52bbaf0d5b4b75137514c8efdedcc6ef4df90142b360153f04ad0721545ab1
3339ba28e9 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen)
6690a28606 Restrict as much as possible in net_processing to translation unit (Jesse Cohen)
1d4df02b7e [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen)
02bbc05310 Rescope g_enable_bip61 to net_processing (Jesse Cohen)
Pull request description:
As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I've moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing.
There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet.
This is somewhat related to other prs #12934#13413#13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing
Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082
I thought we had removed this a long time ago, TBH, its really
confusing feedback to users that we display whether a tx was
broadcast to immediate neighbor nodes, given that has little
indication of whether the tx propagated very far.
2f1a30c63 Fix MAX_STANDARD_TX_WEIGHT check (Johnson Lau)
Pull request description:
As suggested by the constant name and its comment in policy.h, a transaction with a weight of exactly MAX_STANDARD_TX_WEIGHT should be allowed. Users could be confused.
Tree-SHA512: af417de1c6a2e6796ebbb39aa0caad8764302ded155cb1bbfbe457e4567c199cc53256189832b17d4aeec369e190b3edd4c6116d5f0b8cf0ede6dfb4ed83bdd3
These were entirely unused, as based on successful compilation
and a grep for:
\bStartShutdown\(\)|\bShutdownRequested\(\)|\bInterrupt\(\)|\bShutdown\(\)|\bInitLogging\(\)|\bInitParameterInteraction\(\)|\bAppInitBasicSetup\(\)|\bAppInitParameterInteraction\(\)|\bAppInitSanityChecks\(\)|\bAppInitLockDataDirectory\(\)|\bAppInitMain\(\)|\bSetupServerArgs\(\)|\bLicenseInfo\(\)|g_wallet_init_interface|init.h
e56771365b Do not use uppercase characters in source code filenames (practicalswift)
419a1983ca docs: Add a note about the source code filename naming convention (practicalswift)
Pull request description:
Add a note about the source code filename naming convention.
Tree-SHA512: 8d329bd9e19bcd26e74b0862fb0bc2369b46095dbd3e69d34859908632763abd7c3d00ccc44ee059772ad4bae4460c2bcc1c0e22fd9d8876d57e5fcd346cea4b
87fe292d89 doc: Mention disabling BIP61 in bips.md (Wladimir J. van der Laan)
fe16dd8226 net: Add option `-enablebip61` to configure sending of BIP61 notifications (Wladimir J. van der Laan)
Pull request description:
This commit adds a boolean option `-peersendreject`, defaulting to `1`, that can be used to disable the sending of [BIP61](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki) `reject` messages. This functionality has been requested for various reasons:
- security (DoS): reject messages can reveal internal state that can be used to target certain resources such as the mempool more easily.
- bandwidth: a typical node sends lots of reject messages; this counts against upstream bandwidth. Also the reject messages tend to be larger than the message that was rejected.
On the other hand, reject messages can be useful while developing client software (I found them indispensable while creating bitcoin-submittx), as well as for our own test cases, so whatever the default becomes on the long run, IMO the functionality should be retained as option. But that's a discussion for later, for now it's simply a node operator decision.
Also adds a RPC test that checks the functionality.
Tree-SHA512: 9488cc53e13cd8e5c6f8eb472a44309572673405c1d1438c3488f627fae622c95e2198bde5ed7d29e56b948e2918bf1920239e9f865889f4c37c097c37a4d7a9
0bf431870e net: Serve blocks directly from disk when possible (Wladimir J. van der Laan)
Pull request description:
In `ProcessGetBlockData`, send the block data directly from disk if type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the on-disk format matches the network format.
This is expected to increase performance because a deserialization and subsequent serialization roundtrip is avoided.
Tree-SHA512: 9a9500b4c1354eaae1a6f1c6ef2416c1c1985029852589266f3a70e808f6c7482c135e9ab251a527566935378ab7c32dba4ed43ba5451e802d8e72b77d1ba472
In `ProcessGetBlockData`, send the block data directly from disk if
type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the
on-disk format matches the network format.
This is expected to increase performance because a deserialization and
subsequent serialization roundtrip is avoided.
This commit adds a boolean option `-enablebip61`, defaulting to `1`, that
can be used to disable the sending of BIP61 `reject` messages. This
functionality has been requested for various reasons:
- security (DoS): reject messages can reveal internal state that can be
used to target certain resources such as the mempool more easily.
- bandwidth: a typical node sends lots of reject messages; this counts
against upstream bandwidth. Also the reject messages tend to be larger
than the message that was rejected.
On the other hand, reject messages can be useful while developing client
software (I found them indispensable while creating bitcoin-submittx),
as well as for our own test cases, so whatever the default becomes on the
long run, IMO the functionality should be retained as option. But that's
a discussion for later.
fad63eb [logging] Don't incorrectly log that REJECT messages are unknown. (John Newbery)
Pull request description:
Reject messages are logged to debug.log if NET debug logging is enabled.
Because of the way the `ProcessMessages()` function is structured,
processing for REJECT messages will also drop through to the default
branch and incorrectly log `Unknown command "reject" from peer-?`. Fix
that by exiting from `ProcessMessages()` early.
without this PR:
```
2018-05-03T17:37:00.930600Z received: reject (21 bytes) peer=0
2018-05-03T17:37:00.930620Z Reject message code 16: spammy spam
2018-05-03T17:37:00.930656Z Unknown command "reject" from peer=0
```
with this PR:
```
2018-05-03T17:35:04.751246Z received: reject (21 bytes) peer=0
2018-05-03T17:35:04.751274Z Reject message code 16: spammy spam
```
Tree-SHA512: 5c84c98433ab99e0db2dd481f9c2db6f87ff0d39022ff317a791737e918714bbcb4a23e81118212ed8e594ebcf098ab7f52f7fd5e21ebc3f07b1efb279b9b30b
Reject messages are logged to debug.log if NET debug logging is enabled.
Because of the way the `ProcessMessages()` function is structured,
processing for REJECT messages will also drop through to the default
branch and incorrectly log `Unknown command "reject" from peer-?`. Fix
that by exiting from `ProcessMessages()` early.
without this PR:
```
2018-05-03T17:37:00.930600Z received: reject (21 bytes) peer=0
2018-05-03T17:37:00.930620Z Reject message code 16: spammy spam
2018-05-03T17:37:00.930656Z Unknown command "reject" from peer=0
```
with this PR:
```
2018-05-03T17:35:04.751246Z received: reject (21 bytes) peer=0
2018-05-03T17:35:04.751274Z Reject message code 16: spammy spam
```
Seems providing at least minimal visibility to the failure is a good practice.
The only remaining ignored state is in LoadExternalBlockFile, where logging
would likely be spammy.
a5bca13 Bugfix: Include <memory> for std::unique_ptr (Luke Dashjr)
Pull request description:
Not sure why all these includes were missing, but it's breaking builds for some users:
https://bugs.gentoo.org/show_bug.cgi?id=652142
(Added to all files with a reference to `std::unique_ptr`)
Tree-SHA512: 8a2c67513ca07b9bb52c34e8a20b15e56f8af2530310d9ee9b0a69694dd05e02e7a3683f14101a2685d457672b56addec591a0bb83900a0eb8e2a43d43200509
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
eb91835 Add setter for g_initial_block_download_completed (Jonas Schnelli)
3f56df5 [QA] add NODE_NETWORK_LIMITED address relay and sync test (Jonas Schnelli)
158e1a6 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
fa999af [QA] Allow addrman loopback tests (add debug option -addrmantest) (Jonas Schnelli)
6fe57bd Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD (Jonas Schnelli)
31c45a9 Accept addresses with NODE_NETWORK_LIMITED flag (Jonas Schnelli)
Pull request description:
Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD.
Accept and relay NODE_NETWORK_LIMITED peers in addrman.
Tree-SHA512: 8a238fc97f767f81cae1866d6cc061390f23a72af4a711d2f7158c77f876017986abb371d213d1c84019eef7be4ca951e8e6f83fda36769c4e1a1d763f787037
This resolves a bug introduced in
66aa1d58a1 where, if when responding
to a series of transaction requests in a getdata we hit the send
buffer limit and set fPauseSend, we will skip one transaction per
call to ProcessGetData.
Bug found by Cory Fields (@theuni).
9ad6746ccd Use static_cast instead of C-style casts for non-fundamental types (practicalswift)
Pull request description:
A C-style cast is equivalent to try casting in the following order:
1. `const_cast(...)`
2. `static_cast(...)`
3. `const_cast(static_cast(...))`
4. `reinterpret_cast(...)`
5. `const_cast(reinterpret_cast(...))`
By using `static_cast<T>(...)` explicitly we avoid the possibility of an unintentional and dangerous `reinterpret_cast`. Furthermore `static_cast<T>(...)` allows for easier grepping of casts.
For a more thorough discussion, see ["ES.49: If you must use a cast, use a named cast"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: bd6349b7ea157da93a47b8cf238932af5dff84731374ccfd69b9f732fabdad1f9b1cdfca67497040f14eaa85346391404f4c0495e22c467f26ca883cd2de4d3c
This moves the error messages for misbehavior (when available) into the
line that reports the misbehavior, as well as moves the logging to the
`net` category.
This is a continuation of #11583 and avoids serious-looking errors due
to misbehaving peers.
To do this, Misbehaving() gains an optional `message` argument.
E.g. change:
2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED
2018-01-18 16:02:27 ERROR: non-continuous headers sequence
to
2018-01-18 16:02:27 Misbehaving: x.x.x.x:62174 peer=164603 (80 -> 100) BAN THRESHOLD EXCEEDED: non-continuous headers sequence
fbf327b Minimal code changes to allow msvc compilation. (Aaron Clauson)
Pull request description:
These changes are required to allow the Bitcoin source to build with Microsoft's C++ compiler (#11562 is also required).
I looked around for a better place for the typedef of ssize_t which is in random.h. The best candidate looks like src/compat.h but I figured including that header in random.h is a bigger change than the typedef. Note that the same typedef is in at least two other places including the OpenSSL and Berkeley DB headers so some of the Bitcoin code already picks it up.
Tree-SHA512: aa6cc6283015e08ab074641f9abdc116c4dc58574dc90f75e7a5af4cc82946d3052370e5cbe855fb6180c00f8dc66997d3724ff0412e4b7417e51b6602154825
a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift)
Pull request description:
Remove includes in .cpp files for things the corresponding .h file already included.
Example case:
* `addrdb.cpp` includes `addrdb.h` and `fs.h`
* `addrdb.h` includes `fs.h`
Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`.
In line with the header include guideline (see #10575).
Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346
be9f38c Do not make it trivial for inbound peers to generate log entries (Matt Corallo)
Pull request description:
Based on #11580 because I'm lazy.
We should generally avoid writing to debug.log unconditionally for
inbound peers which misbehave (the peer being about to be banned
being an exception, since they cannot do this twice).
Tree-SHA512: 8e59c8d08d00b1527951b30f4842d010a4c2fc440503ade112baa2c1b9afd0e0d1c5c2df83dde25183a242af45089cf9b9f873b71796771232ffb6c5fc6cc0cc
Static analyzer (and humans!) will see ...
```
else if (state.m_chain_sync.m_timeout == 0 || (state.m_chain_sync.m_work_header != nullptr && ...
```
... and infer that state.m_chain_sync.m_work_header might be set to nullptr,
and thus flag `state.m_chain_sync.m_work_header->GetBlockHash().ToString()`
as a potential null pointer dereference.
This commit makes the tacit assumption (m_work_header != nullptr) explicit.
Code introduced in 5a6d00 ("Permit disconnection of outbound peers on
bad/slow chains") which was merged into master four days ago.
We should generally avoid writing to debug.log unconditionally for
inbound peers which misbehave (the peer being about to be banned
being an exception, since they cannot do this twice).
To avoid removing logs for outbound peers, a new log is added to
notify users when a new outbound peer is connected which mimics
the version print.
a357293 Use MakeUnique<Db>(...) (practicalswift)
3e09b39 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (practicalswift)
8617989 Add MakeUnique (substitute for C++14 std::make_unique) (practicalswift)
d223bc9 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (practicalswift)
b45c597 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift)
29ab96d Use unique_ptr for dbenv (DbEnv) (practicalswift)
f72cbf9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift)
8ccf1bb Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift)
73db063 Use unique_ptr for upnp_thread (boost::thread) (practicalswift)
0024531 Use unique_ptr for dbw (CDBWrapper) (practicalswift)
fa6d122 Use unique_ptr:s for {fee,short,long}Stats (TxConfirmStats) (practicalswift)
5a6f768 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift)
860e912 Use unique_ptr for pwalletMain (CWallet) (practicalswift)
Pull request description:
Use `std::unique_ptr` (C++11) where possible.
Rationale:
1. Avoid resource leaks (specifically: forgetting to `delete` an object created using `new`)
2. Avoid undefined behaviour (specifically: double `delete`:s)
**Note to reviewers:** Please let me know if I've missed any obvious `std::unique_ptr` candidates. Hopefully this PR should cover all the trivial cases.
Tree-SHA512: 9fbeb47b800ab8ff4e0be9f2a22ab63c23d5c613a0c6716d9183db8d22ddbbce592fb8384a8b7874bf7375c8161efb13ca2197ad6f24b75967148037f0f7b20c
725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky)
3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo)
Pull request description:
Nowhere else in the protocol do we send headers which are for
blocks we have not fully validated except in response to getheaders
messages with a null locator. On my public node I have not seen any
such request (whether for an invalid block or not) in at least two
years of debug.log output, indicating that this should have minimal
impact.
Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a
76ea17c79 Add mutex requirement for AddToCompactExtraTransactions(…) (practicalswift)
4616c825a Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror) (practicalswift)
7e319d639 Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost. (Matt Corallo)
Pull request description:
* Add mutex requirement for `AddToCompactExtraTransactions(…)`.
* Use `-Wthread-safety-analysis` if available.
* Rebased on top of https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923 - now includes: Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost.
Tree-SHA512: fb7365f85daa2741c276a1c899228181a8d46af51db7fbbdffceeaff121a3eb2ab74d7c8bf5e7de879bcc5042d00d24cb4649c312d51caba45a3f6135fd8b38f
6262915 Add unit test for stale tip checking (Suhas Daftuar)
83df257 Add CConnmanTest to mutate g_connman in tests (João Barbosa)
ac7b37c Connect to an extra outbound peer if our tip is stale (Suhas Daftuar)
db32a65 Track tip update time and last new block announcement from each peer (Suhas Daftuar)
2d4327d net: Allow connecting to extra outbound peers (Suhas Daftuar)
Pull request description:
This is an alternative approach to #11534. Rather than disconnect an outbound peer when our tip looks stale, instead try to connect to an additional outbound peer.
Periodically, check to see if we have more outbound peers than we target (ie if any extra peers are in use), and if so, disconnect the one that least recently announced a new block (breaking ties by choosing the newest peer that we connected to).
Tree-SHA512: 8f19e910e0bb36867f81783e020af225f356451899adfc7ade1895d6d3bd5afe51c83759610dfd10c62090c4fe404efa0283b2f63fde0bd7da898a1aaa7fb281
If our tip hasn't updated in a while, that may be because our peers are
not relaying blocks to us that we would consider valid. Allow connection
to an additional outbound peer in that circumstance.
Also, periodically check to see if we are exceeding our target number of
outbound peers, and disconnect the one which has least recently
announced a new block to us (choosing the newest such peer in the case
of tie).
f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo)
00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo)
015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo)
932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo)
3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo)
3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo)
Pull request description:
@sdaftuar pointed out that the version in #11487 was somewhat DoS-able as someone could feed you a valid chain that forked off the the last checkpoint block and force you to do lots of work just walking backwards across blocks for each new block they gave you. We came up with a few proposals but settled on the one implemented here as likely the simplest without obvious DoS issues. It uses our existing on-load mapBlockIndex walk to make sure everything that descends from an invalid block is marked as such, and then simply caches blocks which we attempted to connect but which were found to be invalid. To avoid DoS issues during IBD, this will need to depend on #11458.
Includes tests from #11487.
Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1
There is no reason to wish to store blocks on disk always just
because a peer is whitelisted. This appears to be a historical
quirk to avoid breaking things when the accept limits were added.
Nowhere else in the protocol do we send headers which are for
blocks we have not fully validated except in response to getheaders
messages with a null locator. On my public node I have not seen any
such request (whether for an invalid block or not) in at least two
years of debug.log output, indicating that this should have minimal
impact.
Reading the variable mapBlockIndex requires holding the mutex cs_main.
The new "Disconnect outbound peers relaying invalid headers" code
added in commit 37886d5e2f and merged
as part of #11568 two days ago did not lock cs_main prior to accessing
mapBlockIndex.
e065249 Add unit test for outbound peer eviction (Suhas Daftuar)
5a6d00c Permit disconnection of outbound peers on bad/slow chains (Suhas Daftuar)
c60fd71 Disconnecting from bad outbound peers in IBD (Suhas Daftuar)
Pull request description:
The first commit will disconnect an outbound peer that serves us a headers chain with insufficient work while we're in IBD.
The second commit introduces a way to disconnect outbound peers whose chains fall out of sync with ours:
For a given outbound peer, we check whether their best known block (which is known from the blocks they announce to us) has at least as much work as our tip. If it doesn't, we set a 20 minute timeout, and if we still haven't heard about a block with as much work as our tip had when we set the timeout, then we send a single getheaders message, and wait 2 more minutes. If after two minutes their best known block has insufficient work, we disconnect that peer.
We protect 4 of our outbound peers (who provide some "good" headers chains, ie a chain with at least as much work as our tip at some point) from being subject to this logic, to prevent excessive network topology changes as a result of this algorithm, while still ensuring that we have a reasonable number of nodes not known to be on bogus chains.
We also don't require our peers to be on the same chain as us, to prevent accidental partitioning of the network in the event of a chain split. Note that if our peers are ever on a more work chain than our tip, then we will download and validate it, and then either reorg to it, or learn of a consensus incompatibility with that peer and disconnect. This PR is designed to protect against peers that are on a less work chain which we may never try to download and validate.
Tree-SHA512: 2e0169a1dd8a7fb95980573ac4a201924bffdd724c19afcab5efcef076fdbe1f2cec7dc5f5d7e0a6327216f56d3828884f73642e00c8534b56ec2bb4c854a656
Currently we have no rotation of outbound peers. If an outbound peer
stops serving us blocks, or is on a consensus-incompatible chain with
less work than our tip (but otherwise valid headers), then we will never
disconnect that peer, even though that peer is using one of our 8
outbound connection slots. Because we rely on our outbound peers to
find an honest node in order to reach consensus, allowing an
incompatible peer to occupy one of those slots is undesirable,
particularly if it is possible for all such slots to be occupied by such
peers.
Protect against this by always checking to see if a peer's best known
block has less work than our tip, and if so, set a 20 minute timeout --
if the peer is still not known to have caught up to a chain with as much
work as ours after 20 minutes, then send a single getheaders message,
wait 2 more minutes, and if a better header hasn't been received by then,
disconnect that peer.
Note:
- we do not require that our peer sync to the same tip as ours, just an
equal or greater work tip. (Doing otherwise would risk partitioning the
network in the event of a chain split, and is also unnecessary.)
- we pick 4 of our outbound peers and do not subject them to this logic,
to be more conservative. We don't wish to permit temporary network
issues (or an attacker) to excessively disrupt network topology.
15f5d3b17 Switch DNSSeed-needed metric to any-automatic-nodes, not services (Matt Corallo)
5ee88b4bd Clarify docs for requirements/handling of addnode/connect nodes (Matt Corallo)
57edc0b0c Rename fAddnode to a more-descriptive "manual_connection" (Matt Corallo)
44407100f Replace relevant services logic with a function suite. (Matt Corallo)
Pull request description:
This was mostly written as a way to clean things up so that the NETWORK_LIMITED PR (#10387) can be simplified a ton, but its also a nice standalone cleanup that will also require a bit of review because it tweaks a lot of stuff across net. The new functions are fine in protocol.h right now since they're straight-forward, but after NETWORK_LIMITED will really want to move elsewhere after @theuni moves the nServices-based selection to addrman from connman.
Adds HasAllRelevantServices and GetRelevantServices, which check
for NETWORK|WITNESS.
This changes the following:
* Removes nRelevantServices from CConnman, disconnecting it a bit
more from protocol-level logic.
* Replaces our sometimes-connect-to-!WITNESS-nodes logic with
simply always requiring WITNESS|NETWORK for outbound non-feeler
connections (feelers still only require NETWORK).
* This has the added benefit of removing nServicesExpected from
CNode - instead letting net_processing's VERSION message
handling simply check HasAllRelevantServices.
* This implies we believe WITNESS nodes to continue to be a
significant majority of nodes on the network, but also because
we cannot sync properly from !WITNESS nodes, it is strange to
continue using our valuable outbound slots on them.
* In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag. This also allows outbound connections
to !NODE_NETWORK nodes for -connect nodes (which was already true
of addnodes).
* Has the (somewhat unintended) consequence of changing one of the
eviction metrics from the same
sometimes-connect-to-!WITNESS-nodes metric to requiring
HasRelevantServices.
This should make NODE_NETWORK_LIMITED much simpler to implement.
Tree-SHA512: 90606896c86cc5da14c77843b16674a6a012065e7b583d76d1c47a18215358abefcbab44ff4fab3fadcd39aa9a42d4740c6dc8874a58033bdfc8ad3fb5c649fc
Adds HasAllRelevantServices and GetRelevantServices, which check
for NETWORK|WITNESS.
This changes the following:
* Removes nRelevantServices from CConnman, disconnecting it a bit
more from protocol-level logic.
* Replaces our sometimes-connect-to-!WITNESS-nodes logic with
simply always requiring WITNESS|NETWORK for outbound non-feeler
connections (feelers still only require NETWORK).
* This has the added benefit of removing nServicesExpected from
CNode - instead letting net_processing's VERSION message
handling simply check HasAllRelevantServices.
* This implies we believe WITNESS nodes to continue to be a
significant majority of nodes on the network, but also because
we cannot sync properly from !WITNESS nodes, it is strange to
continue using our valuable outbound slots on them.
* In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag. This also allows outbound connections
to !NODE_NETWORK nodes for -connect nodes (which was already true
of addnodes).
* Has the (somewhat unintended) consequence of changing one of the
eviction metrics from the same
sometimes-connect-to-!WITNESS-nodes metric to requiring
HasRelevantServices.
This should make NODE_NETWORK_LIMITED much simpler to implement.
Sending a getheaders message with an empty locator and a stop hash
is a request for a single header by hash. The node will respond with
headers for blocks not in the main chain as well as those in the main
chain. To avoid fingerprinting, the node should, however, ignore
requests for headers on side branches that are too old.
A C-style cast is equivalent to try casting in the following order:
1. const_cast(...)
2. static_cast(...)
3. const_cast(static_cast(...))
4. reinterpret_cast(...)
5. const_cast(reinterpret_cast(...))
By using static_cast<T>(...) explicitly we avoid the possibility
of an unintentional and dangerous reinterpret_cast. Furthermore
static_cast<T>(...) allows for easier grepping of casts.
Combine fLimitFree and fOverrideMempoolLimit into a single boolean:
bypass_limits. This is used to indicate that mempool limiting based on feerate
should be bypassed. It is used when readding transactions from a reorg and then
the mempool is trimmed to size after all transactions are added and they can be
evaluated in the context of their descendants. No changes to behavior.
There are a few too many edge-cases here to make this a scripted diff.
The following commits will move a few functions into PeerLogicValidation, where
the local connman instance can be used. This change prepares for that usage.
c00199244 Fix potential null dereferences (MeshCollider)
Pull request description:
Picked up by the static analyzer [Facebook Infer](http://fbinfer.com/) which I was playing around with for another research project. Just adding some asserts before dereferencing potentially null pointers.
Tree-SHA512: 9c01dab2d21bce75c7c7ef867236654ab538318a1fb39f96f09cdd2382a05be1a6b2db0a1169a94168864e82ffeae0686a383db6eba799742bdd89c37ac74397
90d4d89 scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL (practicalswift)
Pull request description:
Since C++11 the macro `NULL` may be:
* an integer literal with value zero, or
* a prvalue of type `std::nullptr_t`
By using the C++11 keyword `nullptr` we are guaranteed a prvalue of type `std::nullptr_t`.
For a more thorough discussion, see "A name for the null pointer: nullptr" (Sutter &
Stroustrup), http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf
With this patch applied there are no `NULL` macro usages left in the repo:
```
$ git grep NULL -- "*.cpp" "*.h" | egrep -v '(/univalue/|/secp256k1/|/leveldb/|_NULL|NULLDUMMY|torcontrol.*NULL|NULL cert)' | wc -l
0
```
The road towards `nullptr` (C++11) is split into two PRs:
* `NULL` → `nullptr` is handled in PR #10483 (scripted, this PR)
* `0` → `nullptr` is handled in PR #10645 (manual)
Tree-SHA512: 3c395d66f2ad724a8e6fed74b93634de8bfc0c0eafac94e64e5194c939499fefd6e68f047de3083ad0b4eff37df9a8a3a76349aa17d55eabbd8e0412f140a297
3f8fa7f Make sure to clean up mapBlockSource if we've already seen the block (Cory Fields)
Pull request description:
Otherwise we may leave them dangling.
Credit TheBlueMatt.
Tree-SHA512: 8be77e08ebfc4f5b206d5ee7cfbe87f92c1eb5bc2b412471993658fe210306789aaf0f3d1454c635508a7d8effede2cf5ac144d622b0157b872733d9661d65c3
5a9b508 [trivial] Add end of namespace comments (practicalswift)
Tree-SHA512: 92b0fcae4d1d3f4da9e97569ae84ef2d6e09625a5815cd0e5f0eb6dd2ecba9852fa85c184c5ae9de5117050330ce995e9867b451fa8cd5512169025990541a2b
At startup, we choose one peer to serve us the headers chain, until
our best header is close to caught up. Disconnect this peer if more
than 15 minutes + 1ms/expected_header passes and our best header
is still more than 1 day away from current time.
This patch makes several related changes:
* Changes the CCoinsView virtual methods (GetCoins, HaveCoins, ...)
to be COutPoint/Coin-based rather than txid/CCoins-based.
* Changes the chainstate db to a new incompatible format that is also
COutPoint/Coin based.
* Implements reconstruction code for hash_serialized_2.
* Adapts the coins_tests unit tests (thanks to Russell Yanofsky).
A side effect of the new CCoinsView model is that we can no longer
use the (unreliable) test for transaction outputs in the UTXO set
to determine whether we already have a particular transaction.
