Instead of using `cost_of_change` for `min_viable_change`
and `change_cost`, and 0 for `change_fee`, use values from
`coin_params`. The previous values don't generate any effects
that is relevant for that context.
faaba770e1 Sort includes in compat.h (MarcoFalke)
fa91a23d63 remove unused limits.h include in compat.h (MarcoFalke)
fa32af22b3 Replace LocaleIndependentAtoi with ToIntegral (MarcoFalke)
faab76c1c0 iwyu on torcontrol (MarcoFalke)
fa0a60dd93 Remove unused boost signals2 from torcontrol (MarcoFalke)
Pull request description:
Remove unused boost, and other includes, and other legacy functions from torcontrol.
ACKs for top commit:
TheCharlatan:
Re-ACK faaba770e1
achow101:
ACK faaba770e1
dergoegge:
utACK faaba770e1
Tree-SHA512: 440f8d3ae9c3cf4dcc368e35b29459b5fcec8c6d233e8f9be3a854e7624b8633d6ccdde10cb0c6f74f86278e06557c4e9e24de30c3c692826237939265c6160a
fa776e61cd Add importmempool RPC (MarcoFalke)
fa20d734a2 refactor: Add and use kernel::ImportMempoolOptions (MarcoFalke)
fa8866990d doc: Clarify the getmempoolinfo.loaded RPC field documentation (MarcoFalke)
6888886cec Remove Chainstate::LoadMempool (MarcoFalke)
Pull request description:
Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
* Users aren't expected to fiddle with the datadir, possibly corrupting it
* An existing mempool file in the datadir may be overwritten
* The node needs to be restarted
* Importing an untrusted file this way is dangerous, because it can corrupt the mempool
Fix all issues by adding a new RPC.
ACKs for top commit:
ajtowns:
utACK fa776e61cd
achow101:
ACK fa776e61cd
glozow:
reACK fa776e61cd
Tree-SHA512: fcb1a92d6460839283c546c47a2d930c363ac1013c4c50dc5215ddf9fe5e51921d23fe0abfae0a5a7631983cfc7e2fff3788b70f95937d0a989a203be4d67546
93cb8f0380 refactor: add missing headers for BIP324 ciphersuite (stratospher)
d22d5d925c crypto: BIP324 ciphersuite follow-up (stratospher)
Pull request description:
follow-up to #28008.
* move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests outside of the loop to be reused every time
* use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
* comment for initiator in `BIP324Cipher::Initialize()`
* systematically damage ciphertext with bit positions in bip324_tests
* use 4095 max bytes for `aad` in bip324 fuzz test
ACKs for top commit:
fanquake:
ACK 93cb8f0380 - thanks for following up here.
Tree-SHA512: 361f3e226d3168fdef69a2eebe6092cfc04ba14ce009420222e762698001eaf8be69a1138dab0be237964509c2b96a41a0b4db5c1df43ef75062f143c5aa741a
e417c988f6 fuzz: coins_view: remove an incorrect assertion (Antoine Poinsot)
c5f6b1db56 fuzz: coins_view: correct an incorrect assertion (Antoine Poinsot)
Pull request description:
The `coins_view` fuzz target would assert in two places that the cache is consistent with the backend. But it's never the case (that's the whole point of using a cache).
The only reason this didn't result in a crash was that we would never actually hit these assertions. I ran into this while introducing a new target with an in-memory `CCoinsViewDB` as the backend view (see https://github.com/bitcoin/bitcoin/pull/28216) which made the code paths with those assertions actually reachable.
ACKs for top commit:
dergoegge:
Code review ACK e417c988f6
Tree-SHA512: 5847bb2744a2f2831dace62d32b79cc491bf54e2af4ce425411d245d566622d9aff816d9be5ec8e830d10851c13f2500bf4f0c004d88b4d7cca1d483ef8960a6
follow-up to #28008.
* move `dummy_tag` variable in FSChaCha20Poly1305 crypto_tests
outside of the loop to be reused every time
* use easy to read `cipher.last()` in `AEADChaCha20Poly1305::Decrypt()`
* comment for initiator in `BIP324Cipher::Initialize()`
* systematically damage ciphertext with bit positions in bip324_tests
* use 4095 max bytes for aad in bip324 fuzz test
547fa52443 net processing: clamp -blockreconstructionextratxn to uint32_t bounds (stickies-v)
e451d1e3c6 net processing: clamp -maxorphantx to uint32_t bounds (stickies-v)
aa89e04e07 doc: document PeerManager::Options members (stickies-v)
Pull request description:
Avoid out-of-bounds user input for `PeerManager::Options` by safely clamping `-maxorphantx` and `-blockreconstructionextratxn`, and avoid platform-specific behaviour by changing `PeerManager::Options::max_extra_txs` from `size_t` to a `uint32_t`. Addresses https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1544114932.
Also documents all `PeerManager::Options` members, addressing https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1272302469.
ACKs for top commit:
dergoegge:
Code review ACK 547fa52443
glozow:
reACK 547fa52443
Tree-SHA512: 042d47b35bb8a7b29ef3dadd4c0c5d26f13a8f174f33687855d603c19f8de0fcbbda94418453331e149885412d4edd5f402d640d938f6d94b4dcf54e2fdbbcc9
d8f1222ac5 refactor: Correct dbwrapper key naming (TheCharlatan)
be8f159ac5 build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan)
c95b37d641 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan)
c534a615e9 refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan)
586448888b refactor: Move HandleError to dbwrapper implementation (TheCharlatan)
dede0eef7a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan)
a5c2eb5748 refactor: Fix logging.h includes (TheCharlatan)
84058e0eed refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan)
e4af2408f2 refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan)
ef941ff128 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan)
b7a1ab5cb4 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan)
d7437908cd refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan)
ea8135de7e refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan)
b9870c920d refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan)
532ee812a4 refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan)
afc534df9a 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 d8f1222ac5🔠
Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
1c976c691c tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake)
7de23cceb8 refactor: fix unterminated LogPrintf()s (fanquake)
0a1029aa29 lint: remove /* Continued */ markers from codebase (fanquake)
910007995d lint: remove lint-logs.py (fanquake)
d86a83d6b8 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 1c976c691c
theuni:
ACK 1c976c691c
MarcoFalke:
re-ACK 1c976c691c👠
Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
fa69e3a95c 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 fa69e3a95c
dergoegge:
utACK fa69e3a95c
Tree-SHA512: 7814e9560abba8d9c0926bcffc70f92e502d22f543af43671248f6fcd1433f35238553c0f05123fde6d8e0f80261af0ab0500927548115153bd68d57fe2da746
1b52d16d07 p2p: network-specific management of outbound connections (Martin Zumsande)
65cff00cee test: Add test for outbound protection by network (Martin Zumsande)
034f61f83b p2p: Protect extra full outbound peers by network (Martin Zumsande)
654d9bc276 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 1b52d16d07
vasild:
ACK 1b52d16d07
Tree-SHA512: 5616c038a5fbb868d4c46c5963cfd53e4599feee25db04b0e18da426d77d22e0994dc4e1da0b810f5b457f424ebbed3db1704f371aa6cad002b3565b20170ec0
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
Since CharCast is no longer needed in the header, move it to the
implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Make it a static function in dbwrapper.cpp, since it is not used
elsewhere and when left in the header, would expose a leveldb type.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Hide the leveldb::Iterator member variable with a pimpl in order not to
expose it directly in the header.
Also, move CDBWrapper::NewIterator to the dbwrapper implementation to
use the pimpl for CDBIterator initialziation.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Hide the leveldb::WriteBatch member variable with a pimpl in order not
to expose it directly in the header.
Also move CDBBatch::Clear to the dbwrapper implementation to use the new
impl_batch.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
f054bd072a refactor: use "if constexpr" in std::vector's Unserialize() (Martin Leitner-Ankerl)
088caa68fb refactor: use "if constexpr" in std::vector's Serialize() (Martin Leitner-Ankerl)
0fafaca4d3 refactor: use "if constexpr" in prevector's Unserialize() (Martin Leitner-Ankerl)
c8839ec5cd refactor: use "if constexpr" in prevector's Serialize() (Martin Leitner-Ankerl)
1403d181c1 refactor: use fold expressions instead of recursive calls in UnserializeMany() (Martin Leitner-Ankerl)
bd08a008b4 refactor: use fold expressions instead of recursive calls in SerializeMany() (Martin Leitner-Ankerl)
Pull request description:
This simplifies the serialization code a bit and should also make it a bit faster.
* use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
* use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization.
ACKs for top commit:
MarcoFalke:
only change is to add a missing `&`. lgtm, re-ACK f054bd072a📦
jonatack:
ACK f054bd072a
sipa:
utACK f054bd072a
john-moffett:
ACK f054bd072a
Tree-SHA512: 0417bf2d6be486c581732297945449211fc3481bac82964e27628b38ef55a47dfa58d730148aeaf1b19fa8eb1076489cc646ceebb178162a9afa59034601501d
It is incorrect to assert that `cache.HaveCoin()` will always be `true`
if `backend.HaveCoin()` is. The coin could well have been marked as
spent in the cache but not yet flushed, in which case `cache.HaveCoin()`
would return `false`.
Note this was never hit because `exists_using_have_coin_in_backend` is
currently never `true` (it's the default implementation of `CCoinsView`.
However this might change if we were to add a target where the backend
is a `CCoinsViewDB`.
Diversify outbound connections with respect to
networks: Every ~5 minutes, try to add an extra connection
to a reachable network which we currently don't have a connection to.
This is done defensively - only try management with respect to networks
after all existing outbound slots are filled.
The resulting situation with an extra outbound peer will be handled
by the extra outbound eviction logic, which protects peers from
eviction if they are the only ones for their network.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
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>
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and
MANUAL connections. Unused until next commit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
