Add new PeerManagerImpl::TryDownloadingHistoricalBlocks method and use it to
request background chain blocks in addition to blocks normally requested by
FindNextBlocksToDownload.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: James O'Beirne <james.obeirne@gmail.com>
fa626af3edbe8d98b2de91dd71729ceef90389fb Remove unused legacy CHashVerifier (MarcoFalke)
fafa3fc5a62702da72991497e3270034eb9159c0 test: add tests that exercise WithParams() (MarcoFalke)
fac81affb527132945773a5315bd27fec61ec52f Use serialization parameters for CAddress serialization (MarcoFalke)
faec591d64e40ba7ec7656cbfdda1a05953bde13 Support for serialization parameters (MarcoFalke)
fac42e9d35f6ba046999b2e3a757ab720c51b6bb Rename CSerAction* to Action* (MarcoFalke)
aaaa3fa9477eef9ea72e4a501d130c57b47b470a Replace READWRITEAS macro with AsBase wrapping function (MarcoFalke)
Pull request description:
It seems confusing that picking a wrong value for `ADDRV2_FORMAT` could have effects on consensus. (See the docstring of `ADDRV2_FORMAT`).
Fix this by implementing https://github.com/bitcoin/bitcoin/issues/19477#issuecomment-1147421608 .
This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327
ACKs for top commit:
TheCharlatan:
ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
ajtowns:
ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
This comment isn't in the right place, as detection of a tx in
recent_rejects would cause the function to exit much earlier.
Move the comment to the right place and tweak the first sentence for
accuracy.
- Whenever a tx is erased. Allows somebody to see which transactions
have been erased due to expiry/overflow, not just how many.
- Whenever a tx is added to a peer's workset.
- AcceptToMemoryPool when a tx is accepted, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transactions that are accepted to mempool just by looking for ATMP logs.
- MEMPOOLREJ when a tx is rejected, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transaction rejections by looking at MEMPOOLREJ logs.
This change makes IsInitialBlockDownload and NotifyHeaderTip functions no
longer tied to individual Chainstate objects. It makes them work with the
ChainstateManager object instead so code is simpler and it is no longer
possible to call them incorrectly with an inactive Chainstate.
This change also makes m_cached_finished_ibd caching easier to reason about,
because now there is only one cached value instead of two (for background and
snapshot chainstates) so the cached IBD state now no longer gets reset when a
snapshot is loaded.
There should be no change in behavior because these functions were always
called on the active ChainState objects.
These changes were discussed previously
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905 and
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792 as
possible followups for that PR.
fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 mempool_entry: improve struct packing (Anthony Towns)
1a118062fbc4ec8f645f4ec4298d869a869c3344 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns)
6fa49937e488d0924044786c76b42324b659f351 test: Check tx from disconnected block is immediately requestable (glozow)
e4ffabbffacc4b890d393aafcc8286916ef887d8 net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns)
6ec1809d33bfc42b80cb6f35625dccd56be8d507 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns)
a70beafdb22564043dc24fc98133fdadbaf77d8a validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns)
1e9684f39fba909b3501e9402d5b61f4bf744ff2 mempool_entry: add mempool entry sequence number (Anthony Towns)
Pull request description:
This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).
The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic.
ACKs for top commit:
naumenkogs:
ACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
amitiuttarwar:
review ACK fb02ba3c5f5
glozow:
reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4
d8f1222ac50f089a0af29eaf8ce0555bad8366ef refactor: Correct dbwrapper key naming (TheCharlatan)
be8f159ac59b9e700cbd3314ed71ebf39bd5b67a build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan)
c95b37d641b1eed4a62d55ca5342a6ed8c7a1ce7 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan)
c534a615e93452a5f509aaf5f68c600391a98d6a refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan)
586448888b72f7c87db4dcd30fc4e4044afae13b refactor: Move HandleError to dbwrapper implementation (TheCharlatan)
dede0eef7adb7413f62f5abd68cac8e01635ba4a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan)
a5c2eb57484314b04ec94523d14e0ef0c6c46d4f refactor: Fix logging.h includes (TheCharlatan)
84058e0eed9c05bc30984b39131e88ad1425628f refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan)
e4af2408f2ac59788567b6fc8cb3a68fc43da9fe refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan)
ef941ff1281e76308c3e746e592375bec023e9e4 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan)
b7a1ab5cb4e60230f62c94efb3a10d07c9af4883 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan)
d7437908cdf242626263ba9d5541addcddadc594 refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan)
ea8135de7e617259cda3fc7b1c8e7569d454fd57 refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan)
b9870c920dc475ec759eaf7339ea42aecba92138 refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan)
532ee812a499e13b123af6b8415d8de1f3804f0f refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan)
afc534df9adbf5599b286b5dc3531a4b9ac2d056 refactor: Wrap DestroyDB in dbwrapper helper (TheCharlatan)
Pull request description:
Leveldb headers are currently included in the `dbwrapper.h` file and thus available to many of Bitcoin Core's source files. However, leveldb-specific functionality should be abstracted by the `dbwrapper` and does not need to be available to the rest of the code. Having leveldb included in a widely-used header such as `dbwrapper.h` bloats the entire project's header tree.
The `dbwrapper` is a key component of the libbitcoinkernel library. Future users of this library would not want to contend with having the leveldb headers exposed and potentially polluting their project's namespace.
For these reasons, the leveldb headers are removed from the `dbwrapper` by moving leveldb-specific code to the implementation file and creating a [pimpl](https://en.cppreference.com/w/cpp/language/pimpl) where leveldb member variables are indispensable. As a final step, the leveldb include flags are removed from the `BITCOIN_INCLUDES` and moved to places where the dbwrapper is compiled.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), and more specifically its stage 1 step 3 "Decouple most non-consensus headers from libbitcoinkernel".
ACKs for top commit:
stickies-v:
re-ACK d8f1222ac5
MarcoFalke:
ACK d8f1222ac50f089a0af29eaf8ce0555bad8366ef 🔠
Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
1c976c691cc4b20f43071aabf36c7afed1571057 tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake)
7de23cceb8ac13fcc709453ef0fa14fb93c460b0 refactor: fix unterminated LogPrintf()s (fanquake)
0a1029aa2946787bfe91a84de882c2dd0de076e9 lint: remove /* Continued */ markers from codebase (fanquake)
910007995d8603ffc466878856227153a638caff lint: remove lint-logs.py (fanquake)
d86a83d6b8587b0971e66c6910af23dd8c042969 lint: drop DIR_IWYU global (fanquake)
Pull request description:
Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job.
The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter.
ACKs for top commit:
TheCharlatan:
ACK 1c976c691cc4b20f43071aabf36c7afed1571057
theuni:
ACK 1c976c691cc4b20f43071aabf36c7afed1571057
MarcoFalke:
re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠
Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
fa69e3a95c452c2ba3221b17c19fba5993b5d073 Remove unused MessageStartChars parameters from BlockManager methods (MarcoFalke)
Pull request description:
Seems odd to expose these for mocking, when it is not needed.
Fix this by removing the the unused parameters and use the already existing member field instead.
ACKs for top commit:
Empact:
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
dergoegge:
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
Tree-SHA512: 7814e9560abba8d9c0926bcffc70f92e502d22f543af43671248f6fcd1433f35238553c0f05123fde6d8e0f80261af0ab0500927548115153bd68d57fe2da746
1b52d16d07be3b5d968157913f04d9cd1e2d3678 p2p: network-specific management of outbound connections (Martin Zumsande)
65cff00ceea48ac8a887ffea79aedb4251aa097f test: Add test for outbound protection by network (Martin Zumsande)
034f61f83b9348664d868933dbbfd8f9f8882168 p2p: Protect extra full outbound peers by network (Martin Zumsande)
654d9bc27647fb3797001472e2464dededb45d3f p2p: Introduce data struct to track connection counts by network (Amiti Uttarwar)
Pull request description:
This is joint work with mzumsande.
This is a proposal to diversify outbound connections with respect to reachable networks. The existing logic evaluates peers for connection based purely on the frequency of available addresses in `AddrMan`. This PR adds logic to automatically connect to alternate reachable networks and adds eviction logic that protects one existing connection to each network.
For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're capable of doing so. For smaller networks like CJDNS, this is even more of an issue and often requires adding manual peers to ensure regularly being connected to the network.
Connecting to multiple networks improves resistance to eclipse attacks for individual nodes. It also benefits the entire p2p network by increasing partition resistance and privacy in general.
The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect.
Once an additional peer is added and we have one more outbound connection than we want, outbound eviction ensures that peers are protected if they are the only ones for their network.
Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts to ensure we also have an automatic connection to it (although this may of course happen by random selection).
ACKs for top commit:
naumenkogs:
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678
vasild:
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678
Tree-SHA512: 5616c038a5fbb868d4c46c5963cfd53e4599feee25db04b0e18da426d77d22e0994dc4e1da0b810f5b457f424ebbed3db1704f371aa6cad002b3565b20170ec0
If a peer is the only one of its network, protect it from eviction.
This improves the diversity of outbound connections with respect to
reachable networks.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Rather than using a bloom filter to track announced invs, simply allow
a peer to request any tx that entered the mempool prior to the last INV
message we sent them. This also obsoletes the UNCONDITIONAL_RELAY_DELAY.
When processing `CMPCTBLOCK` message, at some moments
we can need to process cmpct block txns, since all messages
are handled by ProcessMessage, we call ProcessMessage
all over again. For this reason, it creates a function called
`ProcessCompactBlockTxns` to process it.
DataStream Serialize method has surprising behavior because it just serializes
raw bytes without a length prefix. When you serialize a string or vector, a
length prefix is serialized before the raw object contents so the object can be
unambiguously deserialized later. But DataStreams don't support deserializing
at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see
https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
PR just drops the DataStream::Serialize method.
fb02a3cd1a105bdf60ca39e1858e77685be88976 p2p: Log addresses of stalling peers (Martin Zumsande)
Pull request description:
This was suggested in #27705 by ArmchairCryptologist.
It allows node operators that have the `-logips` option enabled to better identify potentially misbehaving peers and maybe ban them.
This is especially helpful in case of inbound peers for which (dis)connections aren't logged per default, so it's impossible to use the debug log to connect their `nodeId` to an address unless the very noisy `net` debugging is enabled.
In case of outbound peers for which the address is potentially logged when establishing the connection, this just adds some convenience.
ACKs for top commit:
stratospher:
tACK fb02a3c.
jamesob:
github ACK fb02a3cd1a
0xB10C:
Untested ACK fb02a3cd1a105bdf60ca39e1858e77685be88976
instagibbs:
utACK fb02a3cd1a105bdf60ca39e1858e77685be88976
Tree-SHA512: 2080f794c715bd36143405828b4b0e1be859095caf8f8a0c20dd2a4b64d192d78fee0fa350a2bb7c39848718332c4dd4d8edb2cc8d22095b65afe710591f7ccb
A single outbound slot is required, so if the first two slots
are taken by inbound in-flights, the node will reject additional
unless they are coming from outbound.
This means in the case where a fast sybil peer is attempting to
stall out a node, a single high bandwidth outbound peer can
mitigate the attack.
This is a change in behavior so that if for some reason we request a block from a peer, we don't allow an unsolicited CMPCT_BLOCK announcement for that same block to cause a request for a full block from the uninvited peer (as some type of request is already outstanding from the original peer)
5b3406094f2679dfb3763de4414257268565b943 net_processing: Boost inv trickle rate (Anthony Towns)
228e9201efb5574b1b96bb924de1d2e8dd1317f3 txmempool: have CompareDepthAndScore sort missing txs first (Anthony Towns)
Pull request description:
Couple of performance improvements when draining the inventory-to-send queue:
* drop txs that have already been evicted from the mempool (or included in a block) immediately, rather than at the end of processing
* marginally increase outgoing trickle rate during spikes in tx volume
ACKs for top commit:
willcl-ark:
ACK 5b34060
instagibbs:
ACK 5b3406094f
darosior:
utACK 5b3406094f2679dfb3763de4414257268565b943
glozow:
code review ACK 5b3406094f2679dfb3763de4414257268565b943
dergoegge:
utACK 5b3406094f2679dfb3763de4414257268565b943
Tree-SHA512: 155cd3b5d150ba3417c1cd126f2be734497742e85358a19c9d365f4f97c555ff9e846405bbeada13c3575b3713c3a7eb2f780879a828cbbf032ad9a6e5416b30
This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
If transactions are being added to the mempool at a rate faster than 7tx/s
(INVENTORY_BROADCAST_PER_SECOND) then peers' inventory_to_send queue can
become relatively large. If this happens, increase the number of txids
we include in an INV message (normally capped at 35) by 5 for each 1000
txids in the queue.
This will tend to clear a temporary excess out reasonably quickly; an
excess of 4000 invs to send will be cleared down to 1000 in about 30
minutes, while an excess of 20000 invs would be cleared down to 1000 in
about 60 minutes.
Under which circumstances we process received 'mempool' P2P messages
caused confusion in #27426. Rather than bikeshedding the formulation
of the IF-statement, this adds a comment clarifing when we process
the message. Also, correcting the comment of `m_send_mempool`.
Co-authored-by: willcl-ark <will8clark@gmail.com>
be55f545d53d44fdcf2d8ae802e9eae551d120c6 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲
ryanofsky:
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
hebasto:
ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
0076bed45eb2b42111fa3f4c95181393c685a42e logging: log ASN when using `-asmap` (brunoerg)
9836c76ae048698e4f7dab21e3be37313e8392ae net: add `GetMappedAS` in `CConnman` (brunoerg)
Pull request description:
When using `-asmap`, you can check the ASN assigned to the peers only with the RPC command `getpeerinfo` (check `mapped_as` field), however, it's not possible to check it in logs (e.g. see in logs the ASN of the peers when a new outbound peer has been connected). This PR includes the peers' ASN in debug output when using `-asmap`.
Obs: Open this primarily to chase some Concept ACK, I've been using this on my node to facilitate to track the peers' ASN especially when reading the logs.
ACKs for top commit:
Sjors:
tACK 0076bed45eb2b42111fa3f4c95181393c685a42e
jamesob:
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))
achow101:
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e
Tree-SHA512: c19cd11e8ab49962021f390459aadf6d33d221ae9a2c3df331a25d6865a8df470e2c8828f6e5219b8a887d6ab5b3450d34be9e26c00cca4d223b4ca64d51111b
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
55c4795c5794c5c2f8a69b394b847413c9cfbe36 [net processing] Use TxRelay::m_relay_txs over CNode::m_relays_txs (dergoegge)
Pull request description:
`CNode::m_relays_txs` is meant to only be used for the eviction logic in `net`. `TxRelay::m_relay_txs` will hold the same value and is meant to be used on the application layer to determine if we will/should relay transactions to a peer.
(Shameless plug: we should really better specify the interface for updating eviction data to avoid refactors like this in the future -> #25572)
ACKs for top commit:
MarcoFalke:
lgtm ACK 55c4795c5794c5c2f8a69b394b847413c9cfbe36
Tree-SHA512: 59cfd23e32568fd96cda5570790e518242a6c76d4edf5b7d1a2a7f9724d590d2a38395504e05be0af4e98dd5c0056fc0be6568eab2818934692483a186e5181d
