The fuzzer goes through a sequence of operations that get applied to both a
real stack of CCoinsViewCache objects, and to simulation data, comparing
the two at the end.
80f39c99ef addrman, refactor: combine two size functions (Amiti Uttarwar)
4885d6f197 addrman, refactor: move count increment into Create() (Martin Zumsande)
c77c877a8e net: Load fixed seeds from reachable networks for which we don't have addresses (Martin Zumsande)
d35595a78a addrman: add function to return size by network and table (Martin Zumsande)
Pull request description:
AddrMan currently doesn't track the number of its entries by network, it only knows the total number of addresses. This PR makes AddrMan keep track of these numbers, which would be helpful for multiple things:
1. Allow to specifically add fixed seeds to AddrMan of networks where we don't have any addresses yet - even if AddrMan as a whole is not empty (partly fixing #26035). This is in particular helpful if the user abruptly changes `-onlynet` settings (such that addrs that used to be reachable are no longer and vice versa), in which case they currently could get stuck and not find any outbound peers. The second commit of this PR implements this.
1. (Future work): Add logic for automatic connection management with respect to networks - such as making attempts to have at least one connection to each reachable network as suggested [here](https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1249420209). This would involve requesting an address from a particular network from AddrMan, and expanding its corresponding function `AddrMan::Select()` to do this requires internal knowledge of the current number of addresses for each network and table to avoid getting stuck in endless loops.
1. (Future work): Perhaps display the totals to users. At least I would find this helpful to debug, the existing option (`./bitcoin-cli -addrinfo`) is rather indirect by doing the aggregation itself in each call, doesn't distinguish between new and tried, and being based on `AddrMan::GetAddr()` it's also subject to a quality filter which we probably don't want in this spot.
ACKs for top commit:
naumenkogs:
utACK 80f39c9
stratospher:
ACK 80f39c9
achow101:
ACK 80f39c99ef
vasild:
ACK 80f39c99ef
Tree-SHA512: 6359f2e3f4db7c120c0789d92d74cb7d87a2ceedb7d6a34b5eff20c7f55c5c81092d10ed94efe29afc1c66947820a0d9c14876ee0c8d1f8e068a6df4e1131927
The functionality of the old size() is covered by the new Size()
when no arguments are specified, so this does not change behavior.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
fa035fe2d6 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cda Use DataStream where possible (MarcoFalke)
fa9becfe1c streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](fa035fe2d6)
aureleoules:
diff re-ACK fa035fe2d6fa0e6640ba..fa035fe2d6
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
c58c249a5b net_processing: indicate more work to do when orphans are ready to reconsider (Anthony Towns)
ecb0a3e425 net_processing: Don't process tx after processing orphans (Anthony Towns)
c583775706 net_processing: only process orphans before messages (Anthony Towns)
be2304676b txorphange: Drop redundant originator arg from GetTxToReconsider (Anthony Towns)
a4fe09973a txorphanage: index workset by originating peer (Anthony Towns)
Pull request description:
We currently process orphans by assigning them to the peer that provided a missing parent; instead assign them to the peer that provided the orphan in the first place. This prevents a peer from being able to marginally delay another peer's transactions and also simplifies the internal API slightly. Because we're now associating orphan processing with the peer that provided the orphan originally, we no longer process orphans immediately after receiving the parent, but defer until a future call to `ProcessMessage`.
Based on #26295
ACKs for top commit:
naumenkogs:
utACK c58c249a5b
glozow:
ACK c58c249a5b
mzumsande:
Code Review ACK c58c249a5b
Tree-SHA512: 3186c346f21e60440266a2a80a9d23d7b96071414e14b2b3bfe50457c04c18b1eab109c3d8c2a7726a6b10a2eda1f0512510a52c102da112820a26f5d96f12de
If we made progress on orphans, consider that enough work for this peer
for this round of ProcessMessages. This also allows cleaning up the api
for TxOrphange:GetTxToReconsider().
9ab62d71fb [fuzz] Actually use mocked mempool in tx_pool target (dergoegge)
Pull request description:
The current tx_pool target uses the default mempool, making the target non-deterministic. This PR replaces the active chainstate's mempool (i.e. the node's default mempool) with the already present mocked mempool in the target.
ACKs for top commit:
fanquake:
ACK 9ab62d71fb
Tree-SHA512: fe9af3dbdd13cb569fdc2ddbb4290b5ce94206ae83d94267c6365ed0ee9bbe072fcfe7fd632a1a8522dce44608e89aba2f398c1e20bd250484bbadb78143320c
a1c36275b5 [fuzz] Assert that omitting missing transactions always fails block reconstruction (dergoegge)
a8ac61ab5e [fuzz] Add PartiallyDownloadedBlock target (dergoegge)
42bd4c7468 [block encodings] Avoid fuzz blocking asserts in PartiallyDownloadedBlock (dergoegge)
1429f83770 [block encodings] Make CheckBlock mockable for PartiallyDownloadedBlock (dergoegge)
Pull request description:
This PR adds a fuzz target for `PartiallyDownloadedBlock`, which we currently do not have any coverage for.
ACKs for top commit:
mzumsande:
Code Review ACK a1c36275b5
MarcoFalke:
re-ACK a1c36275b5🎼
Tree-SHA512: 01ae452fe457da0c8f2b28c72091d40807c56a9e5d0f80b55f166b67be50baf80a02f53d4cbe9736bb22424cca1758b87e4e471b8a24e756c22563a2640e9a5f
58c2bbdb55 [fuzz] Enable erlay in process_message(s) targets (dergoegge)
Pull request description:
The process_message(s) targets can't exercise the Erlay logic at the moment as the config setting is off by default and not switched on in the fuzz targets.
This PR enables the `-txreconciliation` setting in both targets.
ACKs for top commit:
fanquake:
ACK 58c2bbdb55
Tree-SHA512: a2754fd04549bdcac94d8225244c5c83fe4c26114c0c2fdf316257480625e05e4e6b1b791974e1f1021451d3f81cb59a109261fb73178ad03911f0a3db963077
2022917223 Add secp256k1_selftest call (Pieter Wuille)
3bfca788b0 Remove explicit enabling of default modules (Pieter Wuille)
4462cb0498 Adapt to libsecp256k1 API changes (Pieter Wuille)
9d47e7b71b Squashed 'src/secp256k1/' changes from 44c2452fd3..21ffe4b22a (Pieter Wuille)
Pull request description:
Now that libsecp256k1 has a release (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-December/021271.html), update the subtree to match it.
The changes themselves are not very impactful for Bitcoin Core, but include:
* It's no longer needed to specify whether contexts are for signing or verification or both (all contexts support everything), so make use of that in this PR.
* Verification operations can use the static context now, removing the need for some infrastructure in pubkey.cpp to make sure a context exists.
* Most modules are now enabled by default, so we can drop explicit enabling for them.
* CI improvements (in particular, MSVC and more recent MacOS)
* Introduction of an internal int128 type, which has no effect for GCC/Clang builds, but enables 128-bit multiplication in MSVC, giving a ~20% speedup there (but still slower than GCC/Clang).
* Release process changes (process documentation, changelog, ...).
ACKs for top commit:
Sjors:
ACK 2022917223, but 4462cb0498 could use more eyes on it.
achow101:
ACK 2022917223
jonasnick:
utACK 2022917223
Tree-SHA512: 8a9fe28852abe74abd6f96fef16a94d5a427b1d99bff4caab1699014d24698aab9b966a5364a46ed1001c07a7c1d825154ed4e6557c7decce952b77330a8616b
36c201feb7 remove CBlockIndex copy construction (James O'Beirne)
Pull request description:
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer members (e.g. pprev).
(See also https://github.com/bitcoin/bitcoin/pull/24008#discussion_r891949166)
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
ACKs for top commit:
ajtowns:
ACK 36c201feb7 - code review only
MarcoFalke:
re-ACK 36c201feb7 🏻
Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer references (e.g. pprev).
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
Delete move constructors and declare the destructor to satisfy the
"rule of 5."
* Use SECP256K1_CONTEXT_NONE when creating signing context, as
SECP256K1_CONTEXT_SIGN is deprecated and unnecessary.
* Use secp256k1_static_context where applicable.
7082ce3e88 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79c Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864 txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba6 net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b39 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d196 Remove unnecessary includes of txorphange.h (Anthony Towns)
Pull request description:
Moves extra transactions to be under the `m_msgproc_mutex` lock rather than `g_cs_orphans` and refactors orphan handling so that the lock can be internal to the `TxOrphange` class.
ACKs for top commit:
dergoegge:
Code review ACK 7082ce3e88
glozow:
ACK 7082ce3e88 via code review and some [basic testing](https://github.com/glozow/bitcoin/blob/review-26295/src/test/orphanage_tests.cpp#L150). I think putting txorphanage in charge of handling peer work sets is the right direction.
Tree-SHA512: 1ec454c3a69ebd45ff652770d6a55c6b183db71aba4d12639ed70f525f0035e069a81d06e9b65b66e87929c607080a1c5e5dcd2ca91eaa2cf202dc6c02aa6818
b89530483d util: move threadinterrupt into util (fanquake)
Pull request description:
Alongside thread and threadnames. It's part of libbitcoin_util.
ACKs for top commit:
ryanofsky:
Code review ACK b89530483d. No changes since last review other than rebase
theuni:
ACK b89530483d.
Tree-SHA512: 0421f4d1881ec295272446804b27d16bf63e6b62b272f8bb52bfecde9ae6605e8109ed16294690d3e3ce4b15cc5e7c4046f99442df73adb10bdf069d3fb165aa
0eeb9b0442 [fuzz] Move ConsumeNetAddr to fuzz/util/net.h (dergoegge)
291c8697d4 [fuzz] Make ConsumeNetAddr produce valid onion addresses (dergoegge)
c9ba3f836e [netaddress] Make OnionToString public (dergoegge)
Pull request description:
The chance that the fuzzer is able to guess a valid onion address is probably slim, as they are Base32 encoded and include a checksum. Right now, any target using `ConsumeNetAddr` would have a hard time uncovering bugs that require valid onion addresses as input.
This PR makes `ConsumeNetAddr` produce valid onion addresses by using the 32 bytes given by the fuzzer as the pubkey for the onion address and forming a valid address according to the torv3 spec.
ACKs for top commit:
vasild:
ACK 0eeb9b0442
brunoerg:
ACK 0eeb9b0442
Tree-SHA512: 7c687a4d12f9659559be8f0c3cd4265167d1261d419cfd3d503fd7c7f207cc0db745220f02fb1737e4a5700ea7429311cfc0b42e6c15968ce6a85f8813c7e1d8
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
626b7c8493 fuzz: add scanblocks as safe for fuzzing (James O'Beirne)
94fe5453c7 test: rpc: add scanblocks functional test (Jonas Schnelli)
6ef2566b68 rpc: add scanblocks - scan for relevant blocks with descriptors (Jonas Schnelli)
a4258f6e81 rpc: move-only: consolidate blockchain scan args (James O'Beirne)
Pull request description:
Revives #20664. All feedback from the previous PR has either been responded to inline or incorporated here.
---
Major changes from Jonas' PR:
- consolidated arguments for scantxoutset/scanblocks
- substantial cleanup of the functional test
Here's the range-diff (`git range-diff master jonasschnelli/2020/12/filterblocks_rpc jamesob/2021-11-scanblocks`): https://gist.github.com/jamesob/aa4a975344209f0316444b8de2ec1d18
### Original PR description
> The `scanblocks` RPC call allows one to get relevant blockhashes from a set of descriptors by scanning all blockfilters in a given range.
>
> **Example:**
>
> `scanblocks start '["addr(<bitcoin_address>)"]' 661000` (returns relevant blockhashes for `<bitcoin_address>` from blockrange 661000->tip)
>
> ## Why is this useful?
> **Fast wallet rescans**: get the relevant blocks and only rescan those via `rescanblockchain getblockheader(<hash>)[height] getblockheader(<hash>)[height])`. A future PR may add an option to allow to provide an array of blockhashes to `rescanblockchain`.
>
> **prune wallet rescans**: (_needs additional changes_): together with a call to fetch blocks from the p2p network if they have been pruned, it would allow to rescan wallets back to the genesis block in pruned mode (relevant #15946).
>
> **SPV mode** (_needs additional changes_): it would be possible to build the blockfilterindex from the p2p network (rather then deriving them from the blocks) and thus allow some sort of hybrid-SPV mode with moderate bandwidth consumption (related #9483)
ACKs for top commit:
furszy:
diff re-ACK 626b7c8
Tree-SHA512: f84e4dcb851b122b39e9700c58fbc31e899cdcf9b587df9505eaf1f45578cc4253e89ce2a45d1ff21bd213e31ddeedbbcad2c80810f46755b30acc17b07e2873
b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
