When a node is configured with --blockfilterindex=basic and
-peerblockfilters it can serve compact block filters to its peers.
This commit adds the configuration option handling. Future commits
add compact block serving and service bits signaling.
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
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 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.
-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-
fa69ac7614 doxygen: Fix member comments (MarcoFalke)
Pull request description:
Trailing comments must be indicted with the caret `//!<`.
Not all places do this right now, see for example https://dev.visucore.com/bitcoin/doxygen/txmempool_8h.html#a2bc6653552b5871101b6cbefdbaf251f, but they can be fixed with an almost-scripted-diff:
```
sed -i --regexp-extended -e 's/((,|;) *\/\/!) /\1< /g' $(git grep --extended-regexp -l '(,|;)\s*//!\s')
```
(Same as [doxygen] Fix member comments #7793)
Tree-SHA512: 451077008353ccc6fcc795f34094b2d022feb7a171b562a07ba4de0dcb0aebc137e12b03970764bd81e2da386751d042903db4c4831900f43c0cfde804c81b2b
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
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
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.
* Make PeerLogicValidation final to prevent deriving from it [1]
* Prevent deletions of NetEventsInterface and CValidationInterface
objects via a base class pointer
[1] silences the following compiler warning (from Clang 7.0.0):
/usr/include/c++/v1/memory:2285:5: error: delete called on non-final 'PeerLogicValidation' that has
virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
delete __ptr;
^
/usr/include/c++/v1/memory:2598:7: note: in instantiation of member function
'std::__1::default_delete<PeerLogicValidation>::operator()' requested here
__ptr_.second()(__tmp);
^
init.cpp:201:15: note: in instantiation of member function 'std::__1::unique_ptr<PeerLogicValidation,
std::__1::default_delete<PeerLogicValidation> >::reset' requested here
peerLogic.reset();
^
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
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).
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.
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.
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 simplifies fixing the wallet-returns-stale-info issue as we
can now hold cs_wallet across an entire block instead of only
per-tx (though we only actually do so in the next commit).
This change also removes the NOT_IN_BLOCK constant in favor of only
passing the CBlockIndex* parameter to SyncTransactions when a new
block is being connected, instead of also when a block is being
disconnected.
This change adds a parameter to BlockConnectedDisconnected which
lists the transactions which were removed from mempool due to
confliction as a result of this operation. While its somewhat of a
shame to make block-validation-logic generate a list of mempool
changes to be included in its generated callbacks, fixing this isnt
too hard.
Further in this change-set, CValidationInterface starts listening
to mempool directly, placing it in the middle and giving it a bit
of logic to know how to route notifications from block-validation,
mempool, etc (though not listening for conflicted-removals yet).
02ee4eb Make most_recent_compact_block a pointer to a const (Matt Corallo)
73666ad Add comment to describe callers to ActivateBestChain (Matt Corallo)
962f7f0 Call ActivateBestChain without cs_main/with most_recent_block (Matt Corallo)
0df777d Use a temp pindex to avoid a const_cast in ProcessNewBlockHeaders (Matt Corallo)
c1ae4fc Avoid holding cs_most_recent_block while calling ReadBlockFromDisk (Matt Corallo)
9eb67f5 Ensure we meet the BIP 152 old-relay-types response requirements (Matt Corallo)
5749a85 Cache most-recently-connected compact block (Matt Corallo)
9eaec08 Cache most-recently-announced block's shared_ptr (Matt Corallo)
c802092 Relay compact block messages prior to full block connection (Matt Corallo)
6987219 Add a CValidationInterface::NewPoWValidBlock callback (Matt Corallo)
180586f Call AcceptBlock with the block's shared_ptr instead of CBlock& (Matt Corallo)
8baaba6 [qa] Avoid race in preciousblock test. (Matt Corallo)
9a0b2f4 [qa] Make compact blocks test construction using fetch methods (Matt Corallo)
8017547 Make CBlockIndex*es in net_processing const (Matt Corallo)
In order to sleep accurately, the message handler needs to know if _any_ node
has more processing that it should do before the entire thread sleeps.
Rather than returning a value that represents whether ProcessMessages
encountered a message that should trigger a disconnnect, interpret the return
value as whether or not that node has more work to do.
Also, use a global fProcessWake value that can be set by other threads,
which takes precedence (for one cycle) over the messagehandler's decision.
Note that the previous behavior was to only process one message per loop
(except in the case of a bad checksum or invalid header). That was changed in
PR #3180.
The only change here in that regard is that the current node now falls to the
back of the processing queue for the bad checksum/invalid header cases.
Sebastian Falbesoner