BIP157/158: compact block filters - #10
Open
xanimo wants to merge 184 commits into
Open
Conversation
This adds the listening address on which incoming connections were received to the CNode and CNodeStats structures. The address is reported in `getpeerinfo`. This can be useful for distinguishing connections received on different listening ports (e.g. when using a different listening port for Tor hidden service connections) or different networks. Cherry-picked from: a7e3c28
We don't normally use ReadVarInt from untrusted inputs, but we might see this in the case of corruption. This is exposed in test_bitcoin_fuzzy. Cherry-picked from: 45f0961
Cherry-picked from: 4cac0d1
Before this commit:
for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {
}
After this commit:
for (auto& x : y) {
}
Cherry-picked from: 680bc2c
Cherry-picked from: 2761bca
Cherry-picked from: f5e9a01
This makes ConnectTip responsible for the ConnectTrace instead of splitting the logic between ActivateBestChainStep and ConnectTip Cherry-picked from: 822000c
Cherry-picked from: 29e6e23
Cherry-picked from: d3167ba
Cherry-picked from: a147687
This makes a later change to move it all into one per-block callback simpler. Cherry-picked from: f404334
xanimo
force-pushed
the
backport/bip157-158-onmaster3
branch
7 times, most recently
from
August 2, 2026 04:30
4d2e94c to
1467cc0
Compare
This simplifies fixing the wallet-returns-stale-info issue as we can now hold cs_wallet across an entire block instead of only per-tx (though we only actually do so in the next commit). This change also removes the NOT_IN_BLOCK constant in favor of only passing the CBlockIndex* parameter to SyncTransactions when a new block is being connected, instead of also when a block is being disconnected. This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard. Further in this change-set, CValidationInterface starts listening to mempool directly, placing it in the middle and giving it a bit of logic to know how to route notifications from block-validation, mempool, etc (though not listening for conflicted-removals yet). Cherry-picked from: 461e49f [dogecoin: the boost::bind placeholders here are qualified as boost::placeholders::_N for Boost >= 1.73, matching this tree's convention; see dogecoin dogecoin#1655.]
This simplifies fixing the wallet-returns-stale-info issue as we now hold cs_wallet across an entire block instead of only per-tx. Cherry-picked from: e6d5e6c
Cherry-picked from: acad82f
Cherry-picked from: 91f1e6c [dogecoin: the boost::bind placeholders here are qualified as boost::placeholders::_N for Boost >= 1.73, matching this tree's convention; see dogecoin dogecoin#1655.]
Cherry-picked from: b1a6d4c
Cherry-picked from: 1c95e2f
Try to hide CDB/bitdb behinde CWalletDB. Prepare for full wallet database abstraction. Cherry-picked from: 7184e25 [dogecoin: use the fs:: wrapper instead of boost::filesystem, matching the fs migration (a2caaf1, 37909dd). basename()/extension() become path::stem()/extension(), which were removed from Boost in 1.85.]
Cherry-picked from: 188f89c
This function has been unused ever since the RPC tests no longer use `bitcoin-cli`. Cherry-picked from: 99fecf8
Cherry-picked from: 6665977
Cherry-picked from: 73296f5
Cherry-picked from: 735d9b5
Fixes that belong with the blockfilterindex change itself rather than two branches later: - Use CDiskBlockPos rather than Bitcoin's FlatFilePos in the ReadFilterFromDisk/WriteFilterToDisk signatures and definitions. - Include <util.h>; this tree has no util/system.h. - Guard the global filter index map with a critical section. - Port blockfilter_index_tests.cpp to this tree's APIs: pass consensus params by height, take CDiskBlockPos + hash in UndoReadFromDisk, add the fMineWitnessTx argument to CreateNewBlock, and construct BlockFilter explicitly.
…wn call - Use CDiskBlockPos, not Bitcoin's FlatFilePos, in the on-disk read/write paths; include <util.h> (this tree has no util/system.h). - init.cpp: remove an interrupt_all()/join_all() pair wrongly added to Shutdown(); this tree interrupts threads via Interrupt() and never calls join_all() there (matches the baseline and green integration). Relocated from the later catch-all backport-fixup commit to the branch that introduces blockfilterindex.
getblockfilter constructed its placeholder BlockFilter from an empty byte vector before LookupFilter() overwrites it. Decoding an empty GCS filter throws "VectorReader::read(): end of data", failing the RPC. Use a single 0x00 byte (a valid zero-element GCS filter) as the placeholder. Relocated from the later catch-all backport-fixup commit to the branch that introduces the getblockfilter RPC.
GetFilterType() returned BASIC unconditionally: the ternary tested for
BASIC and returned BASIC either way. That silently disabled both guards
built on it.
WriteBlock()'s
if (GetFilterType() != BlockFilterType::BASIC) return error(...)
became unreachable, and WriteFilterToDisk()'s
assert(filter.GetFilterType() == GetFilterType())
became vacuous. A filter of an unsupported type would have been written
into the basic index rather than rejected -- the opposite of what both
checks were added for.
This is not upstream behaviour; it was introduced in this series.
…own order With BlockFilter default-constructible again, LookupFilterRange() and the getblockfilter RPC no longer need to fabricate a one-byte filter just to have an object to read into. The placeholder was load-bearing only in that it had to survive GCSFilter's constructor, which made it look meaningful when it was not. Also documents the shutdown ordering around DestroyAllBlockFilterIndexes(). GetBlockFilterIndex() returns a raw pointer after dropping g_cs_block_filter_indexes, so the lock protects the map but not the lifetime of the returned index. That is safe today only because g_connman->Stop() joins threadMessageHandler before the indexes are destroyed. Nothing recorded that dependency, so a later reordering of AppShutdown could reintroduce a use-after-free with no obvious cause.
files.md gained a row for indexes/txindex/ but not for the block filter index this series also creates, so a single change left the file internally inconsistent. The index writes two kinds of file under indexes/blockfilter/<type>/: a LevelDB database in db/, and the filters themselves in fltr?????.dat flat files managed by FlatFileSeq. Both are listed, since operators sizing a data directory need to know the flat files exist -- they hold the bulk of the data, not the LevelDB index.
Cherry-picked from: f9e00bb
Cherry-picked from: f6b58c1
Cherry-picked from: 11106a4
If -peerblockfilters is configured, signal the NODE_COMPACT_FILTERS service bit to indicate that we are able to serve compact block filters, headers and checkpoints. Cherry-picked from: 132b30d
Cache block filter headers at heights of multiples of 1000 in memory. Block filter headers at height 1000x are checkpointed, and will be the most frequently requested. Cache them in memory to avoid costly disk reads. Cherry-picked from: 0187d4c
Cherry-picked from: 667bc7a
Port upstream test/functional/rpc_misc.py getindexinfo coverage to qa/rpc-tests/getindexinfo.py for the legacy rpc-test harness. Cherry-picked from: c447b09
The compact block filter headers cache added here declares its lock as Mutex, which this tree does not have. Use CCriticalSection and include <sync.h> for it and the GUARDED_BY annotation. Also align qa/rpc-tests/getindexinfo.py with the RPC as implemented on this branch.
- Remove duplicate net-processing constant definitions re-added by the backport on top of the ones this tree already defines. - PrepareBlockFilterRequest: gate the stop block on chainActive.Contains() rather than BlockRequestAllowed(). - protocol.cpp: drop the NODE_NETWORK_LIMITED service-flag case for a flag this tree does not define. - init.cpp: signal NODE_COMPACT_FILTERS via std::find over the enabled filter types (this tree's vector has no count()). Relocated from the later catch-all backport-fixup commit to the BIP157 net-layer branch.
The getindexinfo command table entry references &getindexinfo, but the function is defined further down in the file. Add a forward declaration so misc.cpp compiles. Relocated from the later catch-all backport-fixup commit to the branch that introduces the getindexinfo RPC.
The 2000-entry cap and its comment came from upstream, where a 1000-block checkpoint interval and a 10-minute target put 2,000,000 blocks somewhere around 2047. Dogecoin's 1-minute target is already past 6,300,000, so the cap is roughly a third of what the chain needs. The cache only inserts while size() < CF_HEADERS_CACHE_MAX_SZ and never evicts, so the practical effect is worse than a cache that is merely too small: it fills with the oldest 2000 checkpoints and then permanently refuses the ~4,300 near the tip, which are the ones peers actually ask for. Every getcfcheckpt at the tip then misses and walks LevelDB for thousands of headers with m_cs_headers_cache held, serializing every other peer behind it. At 64 bytes per entry, 16000 entries is about 1 MiB.
-blockfilterindex has a help entry but -peerblockfilters had none, so the flag that actually exposes filters to the network was invisible in dogecoind -help and discoverable only by reading init.cpp. Also replaces the bare false at the GetBoolArg call site with DEFAULT_PEERBLOCKFILTERS, so the documented default and the applied default cannot drift apart, matching how DEFAULT_BLOCKFILTERINDEX and DEFAULT_TXINDEX are handled.
Records why PrepareBlockFilterRequest() diverges from upstream. Requiring the stop block to be on the active chain, rather than using BlockRequestAllowed(), is what makes keying the cfcheckpt cache on stop_hash alone sound. It is a peer-facing policy change, not just an API adaptation: a client asking for a tip that was reorged out a moment earlier is disconnected rather than served, which is the race upstream's ~1 month window exists to absorb.
serviceFlagToStr() deliberately omits a default arm so the compiler warns when a service flag has no case -- upstream's comment says as much. It arrived here with cases for upstream's flags only, and Dogecoin also defines NODE_XTHIN (1 << 4), so the switch warned under -Wswitch from the moment it landed. Adding the case silences the warning the intended way rather than by adding a default arm, which would disable the check for every flag added later.
Exercise the BIP 157 wire protocol (getcfilters, getcfheaders, getcfcheckpt) over the P2P framework. Covers NODE_COMPACT_FILTERS advertisement, correct cfilter / cfheaders / cfcheckpt responses and basic checkpoint-interval validation.
Test that the node disconnects peers sending malformed or oversized getcfilters / getcfheaders / getcfcheckpt requests: - unsupported filter type (all three message types) - unknown / off-chain stop hash - start_height > stop_height - range >= MAX_GETCFILTERS_SIZE (1000) - range >= MAX_GETCFHEADERS_SIZE (2000)
p2p-blockfilters.py and getblockfilter.py contained no references to invalidateblock, reconsiderblock, or reorgs, which is why the header-walk bug in ProcessGetCFCheckPt went unnoticed. Adds four cases: - cfcheckpt spanning two checkpoint intervals. The existing case stops at height 1001 and yields a single header, so it cannot distinguish a correct walk from one that only ever reads the first entry. - a reorg beneath the second checkpoint, with a transaction mined onto the replacement chain so the checkpoint header is guaranteed to move rather than differing only by an incidentally distinct coinbase. The test asserts the first checkpoint is unchanged and the second has moved, so that a reorg which failed to alter anything fails loudly instead of passing vacuously. - a stop hash reorged off the active chain, which must be rejected rather than served from the response cache or a stale index. - cfcheckpt and getcfilters after the reorg, checked against the RPC values for the current chain. Note on what this does not cover: the mixed-chain case in the pre-fix header walk -- where cs_main was released between loop iterations and the vector could be assembled from two chains -- is a race and cannot be provoked deterministically from a functional test. With the walk anchored to stop_index it is impossible by construction, which is the reason to fix it there rather than to test around it. Requires the wallet for sendtoaddress; if these tests need to run against a --disable-wallet build, the replacement chain will need another way to guarantee a distinguishing filter.
Cache the computed checkpoint header vector in net_processing keyed by (filter_type, stop_hash), so repeated getcfcheckpt requests for the same stop block are served without re-reading filter headers from the index. At current chain height each miss costs roughly 6,200 index reads. The cache needs no reorg invalidation: PrepareBlockFilterRequest() checks chainActive.Contains(stop_index) before the cache is consulted, so a hit implies the stop block is still on the active chain, which pins every one of its ancestors. The header walk itself deliberately keeps GetAncestor() rather than chainActive[height]. It is anchored to the requested stop block instead of the moving tip, so it needs no lock and cannot observe a reorg partway through the loop. This is Dogecoin-specific; there is no upstream equivalent. The index-level header cache from Bitcoin Core is a separate commit in this stack.
Reorder artifact: PR #9725 (37060fb038) is stacked before PR #8574 in this linear-faithful arrangement. Upstream applies #8574 (which adds the include) before #9725 (which removes it), netting to removed. With #9725 first, the removal is a no-op and #8574 re-adds the now-unused include. This cleanup restores the byte-identical final tree.
msg_cfilter.deserialize() read block_hash before filter_type, mirroring the same transposition the C++ serializer had. Because both sides were wrong in the same way, the test round-tripped against our own node and passed, which is why the wire format stayed broken through every run of this test. With the serializer corrected, this parser is now an independent check: reverting it makes the test fail on 'assert message.filter_type == 0', which is exactly the failure a conforming BIP 157 client would hit against a node with the old framing.
The cache keeps one entry per filter type, so two peers alternating between different stop hashes evict each other and every request pays the full ancestor walk. That is a deliberate tradeoff rather than an oversight, but nothing said so. Keying on stop_hash alone is sound because PrepareBlockFilterRequest requires the stop block to be on the active chain, which pins the ancestor set. The same restriction is what makes the thrash case rare in practice: peers converge on the tip and end up sharing the entry.
The series adds a user-visible feature (compact block filters) and a downgrade-breaking chainstate change, and neither was documented anywhere under doc/. A grep for blockfilterindex, peerblockfilters, getblockfilter, getindexinfo, hash_serialized_2 or per-txout across doc/ hit only files.md. Every statement here is checked against this branch rather than carried over from upstream's notes: the option names and their defaults, the NODE_COMPACT_FILTERS bit value, the six P2P message names, the prune and -blockfilterindex init guards, the on-disk index layout, the automatic CCoinsViewDB::Upgrade() call, and the gettxoutsetinfo field rename. Also covers the user-visible changes that ride along with the series: getpeerinfo's new addrbind field; the wallet RPCs' call to BlockUntilSyncedToCurrentChain(); and the three fields the per-txout chainstate removes -- gettxout's version, gettxoutsetinfo's bytes_serialized, and the REST getutxos txvers field. Two of those deserve spelling out for operators rather than only for developers. The wallet change alters RPC latency and consistency semantics, so a wallet call can now block on the validation queue draining; that surfaces as a wallet RPC intermittently taking longer just after a block arrives, which is hard to attribute without knowing it was deliberate. And the REST binary format keeps the version field's width while always writing zero, so parsing still succeeds and a consumer simply reads 0 where it read a real version -- a missing JSON key gets noticed, a plausible zero does not.
xanimo
force-pushed
the
backport/bip157-158-onmaster3
branch
from
August 2, 2026 06:50
eda3229 to
1ac165a
Compare
Ports the assertions this test was missing relative to upstream's p2p_blockfilters.py. The upstream commits themselves (2308385, 9e36067, 5308c97, f5c003d) target test/functional/ and cannot be cherry-picked onto this tree's qa/rpc-tests harness, so the checks are carried over rather than the files. Two gaps mattered. The cfheaders response was only compared against filter hashes derived from our own getblockfilter output, which checks self-consistency: if the node computed a filter header wrongly it would report it wrongly in both places and the test would still pass. compute_last_header() applies BIP 157's chaining rule directly -- header_n = SHA256d(filter_hash_n || header_n-1) -- and checks that chaining the returned hashes onto prev_header reproduces the header for the stop block. Verified non-vacuous: reversing the hash order fails the assertion. Nothing exercised a node running -blockfilterindex without -peerblockfilters. That node must not advertise NODE_COMPACT_FILTERS, over the wire or in getnetworkinfo, and must disconnect peers that send filter requests anyway. A second node covers that, including all three request types, since each is gated separately in PrepareBlockFilterRequest().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BIP157/158: compact block filters
Backports client-side block filtering (BIP158 filters + BIP157 P2P transport)
onto
master. This lets light clients sync privately by downloading compactper-block filters and fetching only the blocks that match their wallet, instead
of leaking addresses to a server (BIP37 bloom) or trusting a third party.
This is the complete backport as a single PR. Because 164 commits is a lot to
review at once, the series is organized into twelve per-topic branches
(
bip157-pr01→bip157-pr12, linked below) as a suggested review roadmap: readthem in order — each is small, self-contained, and a strict superset of the one
before. This PR
(
backport/bip157-158-onmaster3)is their sum, and is what CI and the end-to-end testing below run against.
What's included
getcfilters/cfilter,getcfheaders/cfheaders,getcfcheckpt/cfcheckpt, gated behind theNODE_COMPACT_FILTERSservice bitgetblockfilter,getindexinfo-blockfilterindex(build the index),-peerblockfilters(serve it to peers)BaseIndex,txindexon that base,FlatFileSeq,LookupBlockIndex, the validation-interface rework, wallet-db abstraction, and per-txout chainstateReview roadmap (12 per-topic branches)
bip157-pr01-serializationbip157-pr02-per-txoutbip157-pr03-bip158bip157-pr04-validationinterfacebip157-pr05-walletbip157-pr06-lookupblockindexLookupBlockIndexbip157-pr07-txindexbip157-pr08-baseindexBaseIndexbip157-pr09-flatfileseqFlatFileSeqbip157-pr10-blockfilterindexbip157-pr11-bip157bip157-pr12-dogecoinEach branch is a strict superset of the one below it; 164 commits total.
Upstream fidelity & provenance
bitcoin/bitcoin. Every cherry-picked commit carries aCherry-picked from: <sha>trailer that resolves to a canonical upstream commit(142 in total). Dogecoin-native commits carry no trailer.
Commit/Rewind, null-locator guard);no non-upstream index behavior is introduced.
in a
[dogecoin: …]note rather than left silent.Dogecoin-specific adaptations (each documented in-commit):
fs::wrapper — 2016-era wallet picks are rewired fromboost::filesystemto this tree's
fs::alias (Dogecoin migrated the db layer ina2caaf1f).boost::bindplaceholders are qualified asboost::placeholders::_Nand<boost/bind/bind.hpp>is included, matchingDogecoin's own fix (
41406bfa, PR Build on recent Linux dogecoin/dogecoin#1655). Without this the backport regressesthe build on modern system Boost.
ReadBlockFromDiskwhen serving peers, are preserved throughout.
New surface
-blockfilterindex,-peerblockfiltersgetblockfilter,getindexinfogetcfilters/cfilter,getcfheaders/cfheaders,getcfcheckpt/cfcheckptNODE_COMPACT_FILTERSServing filters to peers is off by default;
-peerblockfiltersrequires-blockfilterindex.Testing
test_dogecoinsuite passes (incl. the BIP158 filter vectors).qa/pull-testersuite — 71/71 pass, 0 fail, includinggetblockfilter.py,getindexinfo.py, andp2p-blockfilters.py(extended withcross-reorg cfcheckpt coverage).
a full mainnet datadir and synced to the chain tip (height 6,307,111);
sampled filters are valid across the whole range (genesis → tip). The full-range
build completes with no crash, assert, or corruption.
system Boost (1.74).
On the cfcheckpt reorg test
The reorg coverage in
p2p-blockfilters.pypasses, but note what it proves: sincePrepareBlockFilterRequest()gates onchainActive.Contains(stop_index), a servedrequest always has its stop block on the active chain, so the checkpoint walk is
correct by construction (it uses
GetAncestor(), anchored to the stop block, lock-free, and cannot observe a mid-loop reorg). The test's value is its anti-vacuity
guard — it asserts the reorg genuinely moved a checkpoint — so the cache-staleness
path is exercised against a real chain reorganization rather than a no-op.
Notes
pr01 ⊂ pr02 ⊂ … ⊂ pr12is a single consistent, GPG-signed lineage; thecombined branch equals
pr12.depends.dogecoincore.org's TLS certificateis expired; this clears once the cert is renewed. Environmental, not code.