This rpc can be used when we want a node to send a message, but
cannot use a python P2P object, for example for testing of low-level
net transport behavior.
fadf671fa5 Refactor: Remove confusing static_cast (MarcoFalke)
faeea1ab58 refactor: Add missing includes (MarcoFalke)
Pull request description:
It seems confusing to use `static_cast<uint160>(bla)` to call the constructor of `uint160`. The normal and common way to call a constructor is by simply calling it. (`uint160{bla}`).
Do this, and also drop the constructor completely where the existing `const&` reference is enough.
Also, add missing includes while touching the file.
ACKs for top commit:
vincenzopalazzo:
ACK fadf671fa5
TheCharlatan:
ACK fadf671fa5
Tree-SHA512: 8fb9a72203a6461b1f4b38bb90943ca25a92b218fc87da2022b90802e7747350e3668a13db3189201ad30e2e39a51d6658fed4aad176fd52cecc1c7f972c3134
fa6286891f Remove unused includes from wallet.cpp (MarcoFalke)
fa8fdbe229 Remove unused includes from blockfilter.h (MarcoFalke)
fad8c36aa9 move-only: Create src/kernel/mempool_removal_reason.h (MarcoFalke)
fa57608800 Remove unused includes from txmempool.h (MarcoFalke)
Pull request description:
This makes compilation of wallet.cpp use a few % less memory and time, locally.
Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem.
ACKs for top commit:
hebasto:
ACK fa6286891f, I have reviewed the code and it looks OK.
Tree-SHA512: 06f1120af2a8ef3368dbd9ae747acda88ace2507bd261bcc10341d476a0b3d71c8485377ea6c108b47df3e4c13b7f75a15f486bafa6a8466303168dde16ebbc8
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.
fa60fa3b0c bitcoin-tidy: Apply bitcoin-unterminated-logprintf to spkm as well (MarcoFalke)
faa11434fe refactor: Enable all clang-tidy plugin bitcoin tests (MarcoFalke)
fa6dc57760 refactor: Enforce C-str fmt strings in WalletLogPrintf() (MarcoFalke)
fa244f3321 doc: Fix bitcoin-unterminated-logprintf tidy comments (MarcoFalke)
Pull request description:
All fmt functions only accept a raw C-string as argument.
There should never be a need to pass a format string that is not a compile-time string literal, so disallow it in `WalletLogPrintf()` to avoid accidentally introducing it.
Apart from consistency, this also fixes the clang-tidy plugin bug https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141.
ACKs for top commit:
theuni:
ACK fa60fa3b0c
Tree-SHA512: fa6f4984c50f9b34e850bdfee7236706af586e512d866cc869cf0cdfaf9aa707029c210ca72d91f85e75fcbd8efe0d77084701de8c3d2004abfd7e46b6fa9072
57cc136282 crypto: make ChaCha20::SetKey wipe buffer (Pieter Wuille)
da0ec62e34 tests: miscellaneous hex / std::byte improvements (Pieter Wuille)
bdcbc8594c fuzz: support std::byte in Consume{Fixed,Variable}LengthByteVector (Pieter Wuille)
7d1cd93234 crypto: require key on ChaCha20 initialization (Pieter Wuille)
44c11769a8 random: simplify FastRandomContext::randbytes using fillrand (Pieter Wuille)
3da636e08b crypto: refactor ChaCha20 classes to use Span<std::byte> interface (Pieter Wuille)
Pull request description:
This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be `Span<std::byte>` based, and other improvements.
* Modifies all functions and constructors of `ChaCha20` and `ChaCha20Aligned` to be `Span<std::byte>` based (aligning them with `FSChaCha20`, `AEADChaCha20Poly1305`, and `FSChaCha20Poly1305`)
* Remove default constructors, to make sure all call sites provide a key (suggested in https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1129313162)
* Wipe key material on rekey for security (suggested in https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267164605)
* Use `HexStr` on byte vectors in tests (suggested in https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1262023316)
* Support `std::byte` vectors in `ConsumeRandomLengthByteVector` and `ConsumeFixedLengthByteVector`, and use it (suggested in https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1265337111)
* And a few more.
While related, I don't see this as a necessary for BIP324.
ACKs for top commit:
stratospher:
ACK 57cc136.
theStack:
re-ACK 57cc136282
Tree-SHA512: 361da4ff003c8465a32eeac0983a8a6f047dbbf5b400168b409c8e3234e79d577fc854e0764389446585da3e12b964c94dd67fc0c9c1d1d092cec296121e05d4
Right now when we get the help for -torcontrol it says that there is a
default ip and port we dont specify if there is a specified ip that we
would also use port 9051 as default
This removes unused includes, primitives/block found manually, and the
others by iwyu:
blockfilter.h should remove these lines:
- #include <serialize.h> // lines 16-16
- #include <undo.h> // lines 18-18
... and move them to where they are really needed.
This was found by IWYU:
txmempool.h should remove these lines:
- #include <random.h> // lines 29-29
- class CBlockIndex; // lines 43-43
- class Chainstate; // lines 45-45
Also, move the stdlib section to the right place. Can be reviewed with:
--color-moved=dimmed-zebra
5e3e83b005 RPC/Mining: Document template_request better for getblocktemplate (Luke Dashjr)
de319c6175 RPC/rpcdoccheck: Error if a oneline_description has a quote for a non-string (Luke Dashjr)
7c61e9df90 Bugfix: RPC: Remove quotes from non-string oneline descriptions (Luke Dashjr)
Pull request description:
Various JSON Object parameters had a `oneline_description` with quote characters. Fix those, and extend `rpcdoccheck` to detect them.
Also, slightly improve GBT's oneline description for template_request.
ACKs for top commit:
MarcoFalke:
review ACK 5e3e83b005
Tree-SHA512: 363d1669a661d0acfc19fddb57e777d781c7246f330cf62160e77dde10a6adcb0249db748127067da1afe1b7d17c71cf611d9fdc3664d6bf5b3f30105637769a
3388e523a1 Rework receive buffer pushback (Pieter Wuille)
Pull request description:
See https://github.com/ElementsProject/elements/issues/1233. There, it has been observed that if both sides of a P2P connection have a significant amount of data to send, a stall can occur, where both try to drain their own send queue before trying to receive. The same issue seems to apply to the current Bitcoin Core codebase, though I don't know whether it's a frequent issue for us.
The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even select() for receiving; if it then turns out that sending is not possible either, no progress is made at all. To address this, the solution used in this PR is to still select() for both sending and receiving when an optimistic send fails, but skip receiving if sending succeeded, and (still) doesn't fully drain the send queue.
This is a significant reduction in how aggressive the "receive pushback" mechanism is, because now it will only mildly push back while sending progress is made; if the other side stops receiving entirely, the pushback disappears. I don't think that's a serious problem though:
* We still have a pushback mechanism at the application buffer level (when the application receive buffer overflows, receiving is paused until messages in the buffer get processed; waiting on our own net_processing thread, not on the remote party).
* There are cases where the existing mechanism is too aggressive; e.g. when the send queue is non-empty, but tiny, and can be sent with a single send() call. In that case, I think we'd prefer to still receive within the same processing loop of the network thread.
ACKs for top commit:
ajtowns:
ACK 3388e523a1
naumenkogs:
ACK 3388e523a1
mzumsande:
Tested ACK 3388e523a1
Tree-SHA512: 28960feb3cd2ff3dfb39622510da62472612f88165ea98fc9fb844bfcb8fa3ed3633f83e7bd72bdbbbd37993ef10181b2e1b34836ebb8f0d83fd1c558921ec17
91d924ede1 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow)
bacdb2e208 Clean up script/standard.{h/cpp} includes (Andrew Chow)
f3c9078b4c Clean up things that include script/standard.h (Andrew Chow)
8bbe257bac MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow)
7a172c76d2 Move CTxDestination to its own file (Andrew Chow)
145f36ec81 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow)
86ea8bed54 Move CScriptID to script.{h/cpp} (Andrew Chow)
b81ebff0d9 Remove ScriptHash from CScriptID constructor (Andrew Chow)
cba69dda3d Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h (Anthony Towns)
Pull request description:
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to `SigningProvider` as these are used only during signing.
* `CScriptID` is moved to `script/script.h` to be next to `CScript`.
* `MANDATORY_SCRIPT_VERIFY_FLAGS` is moved to `interpreter.h`
* The parameters `DEFAULT_ACCEPT_DATACARRIER` and `MAX_OP_RETURN_RELAY` are moved to `policy.h`
* `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above moves
ACKs for top commit:
Sjors:
ACK 91d924ede1
ajtowns:
ACK 91d924ede1
MarcoFalke:
ACK 91d924ede1😇
murchandamus:
ACK 91d924ede1
darosior:
Code review ACK 91d924ede1.
theStack:
Code-review ACK 91d924ede1
Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
fb02ba3c5f mempool_entry: improve struct packing (Anthony Towns)
1a118062fb net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns)
6fa49937e4 test: Check tx from disconnected block is immediately requestable (glozow)
e4ffabbffa net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns)
6ec1809d33 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns)
a70beafdb2 validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns)
1e9684f39f 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 fb02ba3c5f
amitiuttarwar:
review ACK fb02ba3c5f
glozow:
reACK fb02ba3c5f
Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4
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
CTxDestination is really our internal representation of an address and
doesn't really have anything to do with standard script types, so move
them to their own file.
Replaces the constructor in CScriptID that converts a ScriptHash with a
function ToScriptID that does the same. This prepares for a move of
CScriptID to avoid a circular dependency.
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
The legacy wallet allowed to import any raw script, without checking if
it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means IsMineInner() returning IsMineResult::INVALID
for them.
Closes#21070
v21.0 introduced a behaviour changed noted in #21070 where using a config value
`rpcallowip=::0` no longer also permitted ipv4 ip addresses.
The rpc_bind.py functional test covers this new behaviour already by checking
that the list of bind addresses exactly matches what is expected so this
commit only updates the documentation.
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>
faafc35a77 doc: Clarify that -datacarriersize applies to the full raw scriptPubKey, not the data push (MarcoFalke)
55550e7fe7 test: Add -datacarriersize=2 tests (MarcoFalke)
Pull request description:
Clarify with a test that `-datacarriersize` applies to the serialized size of the scriptPubKey, not the size of the pushed data. So for example,
* `-datacarriersize=2` will reject a `raw(6a01aa)`, even though only one byte is pushed
* `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a)`, even though no byte is pushed
* `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a00)`, even though zero bytes are pushed
ACKs for top commit:
ajtowns:
ACK faafc35a77
instagibbs:
ACK faafc35a77
Tree-SHA512: f01ace02798f596ac2a02461e9f2a6ef91b3b37c976ea0b3bc860e2d3efb0ace0fd8b779dd18249cee7f84ebbe5fd21d8506afd3a15edadc00b843ff3b4aacc7
025fda0a76 fuzz: addrman, avoid `ConsumeDeserializable` when possible (brunoerg)
Pull request description:
Using specific functions like `ConsumeService`, `ConsumeAddress` and `ConsumeNetAddr` may be more effective than using `ConsumeDeserializable`. They always return some value while `ConsumeDeserializable` may return `std::nullopt`.
E.g.: In this part of the code, if `op_net_addr` is `std::nullopt`, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them:
```cpp
std::vector<CAddress> addresses;
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider);
if (!opt_address) {
break;
}
addresses.push_back(*opt_address);
}
const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider);
if (opt_net_addr) {
addr_man.Add(addresses, *opt_net_addr, std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)});
}
```
Also, if we are not calling `Add` effectively, it would also be affect other functions that may "depend" on it.
ACKs for top commit:
dergoegge:
Code review ACK 025fda0a76
Tree-SHA512: 02450bec0b084c15ba0cd1cbdfbac067c8fea4ccf27be0c86d54e020f029a6c749a16d8e0558f9d6d35a7ca9db8916f180c872f09474702b5591129e9be0d192
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.
Instead of recursively calling `UnserializeMany` and peeling off one
argument at a time, use a fold expression. 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.
Instead of recursively calling `SerializeMany` and peeling off one
argument at a time, use a fold expression. 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.
Wrap leveldb::DestroyDB in a helper function without exposing
leveldb-specifics.
Also, add missing optional include.
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.
fa633aa690 streams: Teach AutoFile how to XOR (MarcoFalke)
000019e158 Add AutoFile::detail_fread member function (MarcoFalke)
fa7724bc9d refactor: Modernize AutoFile (MarcoFalke)
fa8d227d58 doc: Remove comments that just repeat what the code does (MarcoFalke)
fafe2ca0ce refactor: Remove redundant file check from AutoFile shift operators (MarcoFalke)
9999a49b32 Extract util::Xor, Add key_offset option, Add bench (MarcoFalke)
Pull request description:
This allows `AutoFile` to roll an XOR pattern while reading or writing to the underlying file.
This is needed for https://github.com/bitcoin/bitcoin/pull/28052, but can also be used in any other place.
Also, there are tests, so I've split this up from the larger pull to make review easier, hopefully.
ACKs for top commit:
Crypt-iQ:
crACK fa633aa
willcl-ark:
Lightly tested ACK fa633aa690
jamesob:
reACK fa633aa690 ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tree-SHA512: 6d66cad0a089a096d3f95e4f2b28bce80b349d4b76f53d09dc9a0bea4fc1b7c0652724469c37971ba27728c7d46398a4c1d289c252af4c0f83bb2fcbc6f8e90b
The block index (CBlockTreeDB) is required to write and read blocks, so
move it to blockstorage. This allows to drop the txdb.h include from
`node/blockstorage.h`.
Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
bee2d57a65 script: update flake8 to 6.1.0 (Jon Atack)
38c3fd846b test: python E721 updates (Jon Atack)
Pull request description:
Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green.
```
$ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py
test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:34:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:64:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
src/test/fuzz/descriptor_parse.cpp:88: occurences ==> occurrences
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
ACKs for top commit:
MarcoFalke:
lgtm ACK bee2d57a65
Tree-SHA512: f3788a543ca98e44eeeba1d06c32f1b11eec95d4aef068aa1b6b5c401261adfa3fb6c6d6c769f3fe6839d78e74a310d5c926867e7c367d6513a53d580fd376f3
a733dd79e2 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar)
d4a11abb19 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar)
3556b85022 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar)
0ce805b632 Documentation improvements for assumeutxo (Ryan Ofsky)
768690b7ce Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar)
d43a1f1a2f Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar)
d0d40ea9a6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar)
3cfc75366e test: Clear block index flags when testing snapshots (Suhas Daftuar)
272fbc370c Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar)
10c05710ce Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar)
471da5f6e7 Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar)
1cfc887d00 Remove CChain dependency in node/blockstorage (Suhas Daftuar)
fe86a7cd48 Explicitly track maximum block height stored in undo files (Suhas Daftuar)
Pull request description:
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates.
Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well.
ACKs for top commit:
achow101:
ACK a733dd79e2
jamesob:
reACK a733dd79e2 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))
Sjors:
Code review ACK a733dd79e2.
ryanofsky:
Code review ACK a733dd79e2. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.
Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
ecfe507e07 fuzz: use `ConnmanTestMsg` in `connman` (brunoerg)
Pull request description:
Fixes#27980
Using `ConnmanTestMsg` we can add nodes and be
more effective fuzzing functions like `DisconnectNode`,
`FindNode`, `GetNodeStats` and other ones.
ACKs for top commit:
MarcoFalke:
review ACK ecfe507e07
dergoegge:
utACK ecfe507e07
Tree-SHA512: 97c363b422809f2e9755c082d1102237347abfab72c7baca417bd8975f8a595ddf3a085f8353dbdb9f17fb98fbfe830792bfc0b83451168458018faf6c239efa
fabef121b0 refactor: Use EnsureAnyNodeContext (MarcoFalke)
fa1640617e test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC (MarcoFalke)
Pull request description:
There should be no risk or downside in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example,
* If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass.
* It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
ACKs for top commit:
TheCharlatan:
ACK fabef121b0
furszy:
Code review ACK fabef121. Convinced by checking all current tests usages.
Tree-SHA512: c9e9a536a8721d1b3f267a66b40578b34948892301affdcad121ef8e02bf17037305d0dd53aa94b1b064753e66f9cfb31823b916b707a9d812627f502b818003
06199a995f refactor: Revert addition of univalue sighash string check (TheCharlatan)
0b47c16215 doc: Correct release-notes for sighashtype exceptions (TheCharlatan)
Pull request description:
This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557).
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555).
ACKs for top commit:
MarcoFalke:
lgtm ACK 06199a995f
jonatack:
Tested ACK 06199a995f
stickies-v:
ACK 06199a995f
Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
fa940f41ea Remove unused raw-pointer read helper from univalue (MarcoFalke)
Pull request description:
The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`.
Fix both issues by removing them.
Also, simplify the tests code by removing a `std::string` constructor where possible.
ACKs for top commit:
stickies-v:
utACK fa940f41ea
TheCharlatan:
tACK fa940f41ea
Tree-SHA512: 60c154c1046f01551335af79bf820a6104844f63e89977271b4336b3cd06ac3bab1379e18b7bc61d12bef7446029e91c16541ddecf9e88bc8bc897fc1f6ee2c8
131314b62e fuzz: increase coverage of the descriptor targets (Antoine Poinsot)
90a24741e7 fuzz: add a new, more efficient, descriptor parsing target (Antoine Poinsot)
d60229ede5 fuzz: make the parsed descriptor testing into a function (Antoine Poinsot)
Pull request description:
The current descriptor parsing fuzz target requires valid public or private keys to be provided. This is unnecessary as we are only interested in fuzzing the descriptor parsing logic here (other targets are focused on fuzzing keys serializations). And it's pretty inefficient, especially for formats that need a checksum (`xpub`, `xprv`, WIF).
This introduces a new target that mocks the keys as an index in a list of precomputed keys. Keys are represented as 2 hex characters in the descriptor. The key type (private, public, extended, ..) is deterministically based on this one-byte value. Keys are deterministically generated at target initialization. This is much more efficient and also largely reduces the size of the seeds.
TL;DR: for instance instead of requiring the fuzzer to generate a `pk(xpub6DdBu7pBoyf7RjnUVhg8y6LFCfca2QAGJ39FcsgXM52Pg7eejUHLBJn4gNMey5dacyt4AjvKzdTQiuLfRdK8rSzyqZPJmNAcYZ9kVVEz4kj)` to parse a valid descriptor, it just needs to generate a `pk(03)`.
Note we only mock the keys themselves, not the entire descriptor key expression. As we want to fuzz the real code that parses the rest of the key expression (origin, derivation paths, ..).
This is a target i used for reviewing #17190 and #27255, and figured it was worth PR'ing on its own since the added complexity for mocking the keys is minimal and it could help prevent introducing bugs to the descriptor parsing logic much more efficiently.
ACKs for top commit:
MarcoFalke:
re-ACK 131314b62e🐓
achow101:
ACK 131314b62e
Tree-SHA512: 485a8d6a0f31a3a132df94dc57f97bdd81583d63507510debaac6a41dbbb42fa83c704ff3f2bd0b78c8673c583157c9a3efd79410e5e79511859e1470e629118
This check is already done by the rpc parser. Re-doing it is adding dead
code. Instead, throwing an exception when the assumption does not hold
is the already correct behavior.
To make the fuzz test more accurate and not swallow all runtime errors,
add a check that the passed in UniValue sighash argument is either a
string or null.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
This adds the FSChaCha20Poly1305 AEAD as specified in BIP324, a wrapper
around the ChaCha20Poly1305 AEAD (as specified in RFC8439 section 2.8) which
automatically rekeys every N messages, and automatically increments the nonce
every message.
This adds the FSChaCha20 stream cipher as specified in BIP324, a
wrapper around the ChaCha20 stream cipher (specified in RFC8439
section 2.4) which automatically rekeys every N messages, and
manages the nonces used for encryption.
Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
This adds an implementation of the ChaCha20Poly1305 AEAD exactly matching
the version specified in RFC8439 section 2.8, including tests and official
test vectors.
Remove the variant of ChaCha20Poly1305 AEAD that was previously added in
anticipation of BIP324 using it. BIP324 was updated to instead use rekeying
wrappers around otherwise unmodified versions of the ChaCha20 stream cipher
and the ChaCha20Poly1305 AEAD as specified in RFC8439.
fa9108f85a refactor: Use reinterpret_cast where appropriate (MarcoFalke)
3333f950d4 refactor: Avoid casting away constness (MarcoFalke)
fa6394dd10 refactor: Remove unused C-style casts (MarcoFalke)
Pull request description:
Using a C-style cast to convert pointer types to a byte-like pointer type has many issues:
* It may accidentally and silently throw away `const`.
* It forces reviewers to check that it doesn't accidentally throw away `const`.
For example, on current master a `const char*` is cast to `unsigned char*` (without `const`), see d23fda0584/src/span.h (L273) . This can lead to UB, and the only reason why it didn't lead to UB is because the return type added back the `const`. (Obviously this would break if the return type was deduced via `auto`)
Fix all issues by adding back the `const` and using `reinterpret_cast` where appropriate.
ACKs for top commit:
darosior:
re-utACK fa9108f85a
hebasto:
re-ACK fa9108f85a.
john-moffett:
ACK fa9108f85a
Tree-SHA512: 87f6e4b574f9bd96d4e0f2a0631fd0a9dc6096e5d4f1b95042fe9f197afc2fe9a24e333aeb34fed11feefcdb184a238fe1ea5aff10d580bb18d76bfe48b76a10
faca9a3d5a test: Avoid intermittent issues due to async events in validationinterface_tests (MarcoFalke)
Pull request description:
Currently the tests have many issues:
* They setup the genesis block, even though it is not needed
* They queue an async `UpdatedBlockTip` even, which causes intermittent issues: https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650064645
Fix all issues by trimming down the setup to just `ChainTestingSetup`.
ACKs for top commit:
Crypt-iQ:
tACK faca9a3d5a
Tree-SHA512: 4449040330f89bbaf5ce5b2052417c160b451c373987fdf1069596c07834ed81f0aea1506d53c7d2cd21062b27332d30679285dae194b272fd0cb9ce5ded32cf
07c59eda00 Don't derive secure_allocator from std::allocator (Casey Carter)
Pull request description:
Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`.
(Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.)
ACKs for top commit:
jonatack:
re-ACK 07c59eda00 no change since my previous ACK apart from squashing the commits
achow101:
ACK 07c59eda00
john-moffett:
ACK 07c59eda00 Reviewed and tested. Performance appears unaffected in my environment.
Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
6960c81cbf kernel: Remove Univalue from kernel library (TheCharlatan)
10eb3a9faa kernel: Split ParseSighashString (TheCharlatan)
Pull request description:
Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files.
UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.
ACKs for top commit:
achow101:
ACK 6960c81cbf
theuni:
Re-ACK 6960c81cbf
stickies-v:
re-ACK 6960c81cbf
Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
Affects both secure_allocator and zero_after_free_allocator.
Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`.
Drive-by: Aggressively remove facilities unnecessary since C++11 from both allocators to keep things simple.
This is to (a) avoid repeated lookups into the block index for an entry that
should never change and (b) emphasize that the snapshot base should always
exist when set and not change during the runtime of the program.
Thanks to Russ Yanofsky for suggesting this approach.
Also rewrite CheckBlockIndex() to perform tests on all chainstates.
This increases sanity-check coverage, as any place in our code where we were
invoke CheckBlockIndex() on a single chainstate will now invoke the sanity
checks on all chainstates.
This change also tightens up the checks on setBlockIndexCandidates and
mapBlocksUnlinked, to more precisely match what we aim for even in the presence
of assumed-valid blocks.
When using assumeutxo and multiple chainstates are active, the background
chainstate should consider all HAVE_DATA blocks that are ancestors of the
snapshotted block and that have more work than the tip as potential candidates.
When using assumeutxo, we only need the background chainstate to consider
blocks that are on the chain leading to the snapshotted block.
Note that this introduces the new invariant that we can only have an assumeutxo
snapshot where the snapshotted blockhash is in our block index. Unknown block
hashes that are somehow passed in will cause assertion failures when processing
new blocks.
Includes test fixes and improvements by Andrew Chow and Fabian Jahr.
Once a descriptor is successfully parsed, execute more of its methods.
There is probably still room for improvements by checking for some
invariants, but this is a low hanging fruit that significantly increases
the code coverage of these targets.
This new target focuses on fuzzing the actual descriptor parsing logic
by not requiring the fuzzer to produce valid keys (nor a valid checksum
for that matter).
This should make it much more efficient to find bugs we could introduce
moving forward.
Using a character as a marker (here '%') to be able to search and
replace in the string without having to mock the actual descriptor
parsing logic was an insight from Pieter Wuille.
Separate the notion of which blocks are stored on disk, and what data is in our
block index, from what tip a chainstate might be able to get to. We can use
chainstate-agnostic data to determine when to store a block on disk (primarily,
an anti-DoS set of criteria) and let the chainstates figure out for themselves
when a block is of interest for being a candidate tip.
Note: some of the invariants in CheckBlockIndex are modified, but more work is
needed (ie to move CheckBlockIndex to ChainstateManager, as most of what
CheckBlockIndex is doing is checking the consistency of the block index, which
is outside of Chainstate).
node_context is never null, but if it was, it would lead to a nullptr
dereference in node_context->scheduler. Just use EnsureAnyNodeContext
everywhere for more robust, consistent, and correct code.
dd9633b516 test: wallet, add coverage for watch-only raw sh script migration (furszy)
cc781a2180 descriptor: InferScript, do not return top-level only func as sub descriptor (furszy)
286e0c7d5e wallet: loading, log descriptor parsing error details (furszy)
Pull request description:
Linked to #28057.
Currently, the `InferScript` function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.
This behavior occurs because the inference process bypasses the `pkh` subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a `sh(addr(ADDR))` descriptor. Then, the failure arises because the `addr()` function is restricted to being used only at the top level.
For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).
ACKs for top commit:
achow101:
ACK dd9633b516
darosior:
utACK dd9633b516
Tree-SHA512: 61e763206c604c372019d2c36e31684f3dddf81f8b154eb9aba5cd66d8d61bda457ed4e591613eb6ce6c76cf7c3f11764abc6cd727a7c2b6414f1065783be032
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
fa6245da60 fuzz: Generate process_message targets individually (MarcoFalke)
fa1471e575 refactor: Remove duplicate allNetMessageTypesVec (MarcoFalke)
Pull request description:
Now that `LIMIT_TO_MESSAGE_TYPE` is a runtime setting after commit 927b001502, it shouldn't hurt to also generate each message type individually. Something similar was done for the `rpc` target in commit cf4da5ec29.
ACKs for top commit:
stickies-v:
re-crACK fa6245da60
brunoerg:
reACK fa6245da60
Tree-SHA512: 8f3ec71bab89781f10820a0e027fcde8949f3333eb19a30315aaad6f90f5167028113cea255b2d60b700da817c7eaac20b7b4c92f931052d7f5c2f148d33aa5a
e8c31f135c tests: Test for bumping single output transaction (Andrew Chow)
4f4d4407e3 test: Test bumpfee reduce_output (Andrew Chow)
7d83502d3d bumpfee: Allow original change position to be specified (Andrew Chow)
Pull request description:
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify the index of the change output.
Fixes#11233Fixes#20795
ACKs for top commit:
ismaelsadeeq:
re ACK e8c31f135c
pinheadmz:
re-ACK e8c31f135c
furszy:
Code review ACK e8c31f13
Tree-SHA512: 3a230655934af17f7c1a5953fafb5ef0d687c21355cf284d5e98fece411f589cd69ea505f06d6bdcf82836b08d268c366ad2dd30ae3d71541c9cdf94d1f698ee
f1d807e383 Add more tests for the BIP21 implementation (Kiminuo)
Pull request description:
This PR is an attempt to make it clear how the current BIP21 implementation behaves in Bitcoin Core. Especially, I'm interested whether one can specify multiple `amount` (`message`, etc.) parameters.
My primary end goal is to answer [this question of mine](https://bitcoin.stackexchange.com/questions/118654/how-to-interpret-bip21-uri-with-amount-specified-twice/) but I figured that maybe it's worth a PR. If not, I'll close the PR.
ACKs for top commit:
MarcoFalke:
lgtm ACK f1d807e383
kevkevinpal:
ACK [f1d807e](f1d807e383)
Tree-SHA512: d287809d47c5cfc667f850927bfd969bd345a996d3d53a4c26ef0ffd29eb75ef53358692a15f9a0493ec9e1c101123b6584572e25f87bcb98ff67f6b6c166de4
7d92b1430a refactor: use Span for SipHash::Write (Sebastian Falbesoner)
Pull request description:
This simple refactoring PR changes the interface for the `SipHash` arbitrary-data `Write` method to take a `Span<unsigned char>` instead of having to pass data and length. (`Span<std::byte>` seems to be more modern, but vectors of `unsigned char` are still used prety much everywhere where SipHash is called, and I didn't find it very appealing having to clutter the code with `Make(Writable)ByteSpan` helpers).
ACKs for top commit:
sipa:
utACK 7d92b1430a
MarcoFalke:
lgtm ACK 7d92b1430a
achow101:
ACK 7d92b1430a
Tree-SHA512: f17a27013c942aead4b09f5a64e0c3ff8dbc7e83fe63eb9a2e3ace8be9921c9cbba3ec67e3e83fbe3332ca941c42370efd059e702c060f9b508307e9657c66f2
f6a26196cf Added `longpollid` and `data` params to `template_request` #27998 (Rhythm Garg)
Pull request description:
This PR will add the optional parameters `longpollid` and `data` to `template_request` as they were missing when calling `help getblocktemplate` in RPCHelpMan.
I request the maintainers to review this and let me know about any mistakes in the descriptions of the parameters.
This PR refers to the issue #27998
ACKs for top commit:
ItIsOHM:
> tACK [f6a2619](f6a26196cf)
russeree:
tACK f6a26196cf
stickies-v:
tACK f6a26196cf
Tree-SHA512: 6c592db59cb11b2d031ce5265c547fa296266278f6c25f96afe18a420e0d547f4d483e0f66de75d52c0c319ac1585f3558b9f70c12ef208c96ec96a51f786c6a
5080c9c25f build: adapt Windows builds for libsecp256k1 build changes (fanquake)
ff061fde18 Squashed 'src/secp256k1/' changes from 705ce7ed8c..c545fdc374 (fanquake)
Pull request description:
Includes https://github.com/bitcoin-core/secp256k1/pull/1378. Which fixes#28079.
Adapts Windows build for https://github.com/bitcoin-core/secp256k1/pull/1367.
ACKs for top commit:
hebasto:
ACK 5080c9c25f, I've made the `src/secp256k1` subtree update locally and got zero diff with this PR branch.
jonasnick:
ACK 5080c9c25f
Tree-SHA512: 37915d420ebacefc6bc82c2511bff3d6884e01d5c92795f19cd61862f96b30aa1fe768aeabec128c9d25c1d8bc62b46b46969626067266074b39566ad9e2f5ba
1cd45d4e08 test: move random.h include header from setup_common.h to cpp (Jon Atack)
1b246fdd14 test: move remaining random test util code from setup_common to random (jonatack)
Pull request description:
and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!)
ACKs for top commit:
MarcoFalke:
lgtm ACK 1cd45d4e08🌂
Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93
c545fdc374 Merge bitcoin-core/secp256k1#1298: Remove randomness tests
b40e2d30b7 Merge bitcoin-core/secp256k1#1378: ellswift: fix probabilistic test failure when swapping sides
c424e2fb43 ellswift: fix probabilistic test failure when swapping sides
907a67212e Merge bitcoin-core/secp256k1#1313: ci: Test on development snapshots of GCC and Clang
0f7657d59c Merge bitcoin-core/secp256k1#1366: field: Use `restrict` consistently in fe_sqrt
cc55757552 Merge bitcoin-core/secp256k1#1340: clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
600c5adcd5 clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
981e5be38c ci: Fix typo in comment
e9e9648219 ci: Reduce number of macOS tasks from 28 to 8
609093b387 ci: Add x86_64 Linux tasks for gcc and clang snapshots
1deecaaf3b ci: Install development snapshots of gcc and clang
b79ba8aa4c field: Use `restrict` consistently in fe_sqrt
c9ebca95f9 Merge bitcoin-core/secp256k1#1363: doc: minor ellswift.md updates
afd7eb4a55 Merge bitcoin-core/secp256k1#1371: Add exhaustive tests for ellswift (with create+decode roundtrip)
2792119278 Add exhaustive test for ellswift (create+decode roundtrip)
c7d900ffd1 doc: minor ellswift.md updates
332af315fc Merge bitcoin-core/secp256k1#1344: group: save normalize_weak calls in `secp256k1_ge_is_valid_var`/`secp256k1_gej_eq_x_var`
9e6d1b0e9b Merge bitcoin-core/secp256k1#1367: build: Improvements to symbol visibility logic on Windows (attempt 3)
0aacf64352 Merge bitcoin-core/secp256k1#1370: Corrected some typos
b6b9834e8d small fixes
07c0e8b82e group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var`
3fc1de5c55 Merge bitcoin-core/secp256k1#1364: Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
fb758fe8d6 Merge bitcoin-core/secp256k1#1323: tweak_add: fix API doc for tweak=0
c6cd2b15a0 ci: Add task for static library on Windows + CMake
020bf69a44 build: Add extensive docs on visibility issues
0196e8ade1 build: Introduce `SECP256k1_DLL_EXPORT` macro
9f1b1904a3 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API`
ae9db95cea build: Introduce `SECP256K1_STATIC` macro for Windows users
7966aee31d Merge bitcoin-core/secp256k1#1369: ci: Print commit in Windows container
a7bec34231 ci: Print commit in Windows container
249c81eaa3 Merge bitcoin-core/secp256k1#1368: ci: Drop manual checkout of merge commit
98579e297b ci: Drop manual checkout of merge commit
5b9f37f136 ci: Add `CFLAGS: -O1` to task matrix
a6ca76cdf2 Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
0fa84f869d Merge bitcoin-core/secp256k1#1358: tests: introduce helper for non-zero `random_fe_test()` results
5a95a268b9 tests: introduce helper for non-zero `random_fe_test` results
304421d57b tests: refactor: remove duplicate function `random_field_element_test`
3aef6ab8e1 Merge bitcoin-core/secp256k1#1345: field: Static-assert that int args affecting magnitude are constant
4494a369b6 Merge bitcoin-core/secp256k1#1357: tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
799f4eec27 Merge bitcoin-core/secp256k1#1356: ci: Adjust Docker image to Debian 12 "bookworm"
c862a9fb49 ci: Adjust Docker image to Debian 12 "bookworm"
a1782098a9 ci: Force DWARF v4 for Clang when Valgrind tests are expected
7d8d5c86df tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
8a7273465b Help the compiler prove that a loop is entered
fd491ea1bb Merge bitcoin-core/secp256k1#1355: Fix a typo in the error message
ac43613d25 Merge bitcoin-core/secp256k1#1354: Add ellswift to CHANGELOG
67887ae65c Fix a typo in the error message
926dd3e962 Merge bitcoin-core/secp256k1#1295: abi: Use dllexport for mingw builds
10836832e7 Merge bitcoin-core/secp256k1#1336: Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
7c7467ab7f Refer to ellswift.md in API docs
c32ffd8d8c Add ellswift to CHANGELOG
3c1a0fd37f Merge bitcoin-core/secp256k1#1347: field: Document return value of fe_sqrt()
5779137457 field: Document return value of fe_sqrt()
be8ff3a02a field: Static-assert that int args affecting magnitude are constant
efa76c4bf7 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var`
5b7bf2e9d4 Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
05873bb6b1 tweak_add: fix API doc for tweak=0
6ec3731e8c Simplify test PRNG implementation
fb5bfa4eed Add static test vector for Xoshiro256++
723e8ca8f7 Remove randomness tests
bc7c8db179 abi: Use dllexport for mingw builds
git-subtree-dir: src/secp256k1
git-subtree-split: c545fdc374964424683d9dac31a828adedabe860
c7db88af71 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c97 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)
Pull request description:
`IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.
This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).
ACKs for top commit:
sipa:
utACK c7db88af71
achow101:
ACK c7db88af71
Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
4e5c933f6a Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d1ae tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b9fc crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b391f crypto: switch poly1305 to incremental implementation (Pieter Wuille)
Pull request description:
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the ciphertext, together another 16 bytes.
Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361).
This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.
ACKs for top commit:
achow101:
ACK 4e5c933f6a
theStack:
ACK 4e5c933f6a
stratospher:
tested ACK 4e5c933.
Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
fa6dfaaf45 scripted-diff: Use new FUZZ_TARGET macro everywhere (MarcoFalke)
fa36ad8b09 fuzz: Accept options in FUZZ_TARGET macro (MarcoFalke)
Pull request description:
The `FUZZ_TARGET` macros have many issues:
* The developer will have to pick the right macro to pass the wanted option.
* Adding a new option requires doubling the number of existing macros in the worst case.
Fix all issues by using only a single macro.
This refactor does not change behavior.
ACKs for top commit:
dergoegge:
ACK fa6dfaaf45
Tree-SHA512: 49a34553867a1734ce89e616b2d7c29b784a67cd8990db6573f0c7b18957636ef0c81d3d0d444a04c12cdc98bc4c4aa7a2ec94e6232dc363620a746e28416444
4da243ba02 qt: show own outputs on PSBT signing window (Hernan Marino)
Pull request description:
This fixes https://github.com/bitcoin-core/gui/issues/732 .
It allows you to identify your own addresses in the outputs of a transaction in the PSBT signing window. This enables easy identification of change outputs, and prevents certain attacks where someone (co-signers of a multisig, or others ) might trick you into signing a transaction while they are stealing the change, since prior to this modification there was no easy way of knowing this.
The identification of the output is similar to the way this is done in the transaction details window.
A sample output is :
ACKs for top commit:
achow101:
ACK 4da243ba02
jarolrod:
ACK 4da243ba02
Tree-SHA512: fa9901d2acc84472c11afcd0a59a859db598cdf5cea755b492178d3e7434b70d9bd8f554928938a2ff9920c8f397fef814ce14b416556c30fba0c3c1f62cd722
When simulating a snapshot, remove the HAVE_DATA status for blocks below the
snapshot height, to simulate never having downloaded them at all. This makes
tests more realistic (and more closely match what will happen when using
assumeutxo).
Block arrival information (and the preciousblock RPC, a related concept) are
both chainstate-agnostic, so these are moved to ChainstateManager. This should
just be a refactor, without any observable behavior changes.
