spv: parallel cfheaders and cfilter download (--cf_workers, stacked on #387) - #389
Open
xanimo wants to merge 12 commits into
Open
spv: parallel cfheaders and cfilter download (--cf_workers, stacked on #387)#389xanimo wants to merge 12 commits into
xanimo wants to merge 12 commits into
Conversation
xanimo
force-pushed
the
0.1.5-dev-cf-parallel
branch
from
August 3, 2026 03:01
a903c81 to
6a9207b
Compare
Golomb-Coded Set construction and matching, the encoding BIP158 compact block filters are built from: Golomb-Rice encode/decode over a bitstream, the SipHash-2-4 based mapping of an element into the [0, N*M) range, and set construction and membership queries over a sorted delta list. This is the arithmetic layer only. It has no dependency on any P2P message type, on filter headers, or on the network at all -- it takes byte strings in and answers membership questions -- so it builds and tests standalone, and does so with WITH_NET=OFF. Placed in the unconditional source lists of both build systems rather than under WITH_NET, which is where filter code would otherwise land. Guarding it on net would have made the two build systems disagree (Makefile.am lists core sources unconditionally), and that disagreement is not theoretical: a source present in one build system and absent from the other is invisible until the other platform links. Verified src/*.c membership matches between Makefile.am and CMakeLists.txt after the change. 79/79 with test_golomb registered and running; CMake build clean with WITH_NET=OFF.
One filter header per CFCHECKPT_INTERVAL (1000) blocks, used to validate a peer's cfcheckpt response before any filter header it serves is trusted. Mainnet carries 6239 entries; testnet and regtest are empty for now. Kept out of chainparams.c on purpose. It is 6200 lines of generated data, which would bury the chain parameter definitions it would otherwise sit between, and it means adding block checkpoints and adding filter checkpoints touch different files -- so work on header sync and work on compact filters stop colliding in the same region of the same file. The regtest array carries a single zero entry with a count of 0: regtest chains are generated per test run, so there is no stable header to pin, and the entry exists only because a zero-length array is not valid C.
The BIP157 wire layer on top of the GCS encoding: getcfilters, getcfheaders and getcfcheckpt requests, the cfilter, cfheaders and cfcheckpt responses, filter header chaining, and validation of a served filter against the header committed to by that chain. Two things worth a reviewer's attention. The cfilter message follows BIP157 field order -- filter_type first, then block_hash, then the filter bytes. This is worth stating because it has been the other way round here before, and because Dogecoin Core's own compact filter work has been observed serialising block_hash first, which is not the specified order. A peer pairing this code with an implementation that swaps those two fields will parse filter_type out of block_hash[0], take the next 32 bytes as the hash, and then hit a bogus varint -- deserialisation failure at best, a silent header mismatch and a ban at worst. That failure mode does not show up in a round-trip test, because a swapped serialiser and a swapped deserialiser agree with each other perfectly. It needs a test against a fixed BIP157 payload with known bytes. Note also that the on-disk cfilters format is framed separately from the wire format, so a client reading filters from a bootstrap or cache exercises none of this. The counts in cfheaders and cfcheckpt are bounded before they size an allocation. Both are compact_size values straight off the wire, and vector_new() rounds its reservation up to a power of two, so 0xFFFFFFFF became a ~34 GB allocation before a single hash was read -- a memory exhaustion DoS from a ten byte message, the same class as the getheaders locator count. Each entry is a 32 byte hash, so any count the remaining buffer cannot hold is rejected, and cfheaders additionally takes BIP157's 2000 hash cap. Tests cover the wire format, header chaining, checkpoint validation, both unbounded-count regressions, and payload tampering: every single-bit flip of a filter payload, plus truncation and extension, must fail validation against the header computed from the original. That last one is the attack the validation exists to stop, since a peer able to strip a script from a filter makes the client skip the block that pays it. 80/80 with test_compact_filter registered and running.
The Windows build failed to link every executable with six unresolved
externals -- the three CF checkpoint arrays and their three counts:
spvnode.vcxproj : error LNK2019: unresolved external symbol
dogecoin_mainnet_cf_checkpoint_array
dogecoin.dll : fatal error LNK1120: 6 unresolved externals
The file was committed, listed in both build systems, and compiled
cleanly. The tell was in the build log: chainparams.c was compiled nine
times and cf_checkpoints.c once.
chainparams.c is declared TARGET_SOURCES(... PUBLIC ...), so CMake
compiles it into every consumer target as well as the library.
cf_checkpoints.c was PRIVATE, so only the library got it. That is
invisible on Linux, where a shared object resolves the data symbol for
the executables anyway, and fatal on a Windows DLL build, where data
symbols do not cross the boundary without dllimport -- and
LIBDOGECOIN_API expands to nothing for consumers.
So the existing PUBLIC on chainparams is deliberate rather than
incidental: it is what lets the CLI tools reference chain data arrays at
all on Windows. Filter checkpoints are the same kind of thing and need
the same treatment.
Verified by reproducing the Windows configuration locally
(-DBUILD_SHARED_LIBS=1): cf_checkpoints.c now compiles into 9 targets,
matching chainparams.c exactly, and the shared build links clean.
… does The two build systems disagreed. Makefile.am lists src/compact_filter.c in the unconditional sources, CMake had it inside IF(WITH_NET). Since test/compact_filter_tests.c is registered unconditionally in both, a CMake build configured with -DWITH_NET=OFF compiled the tests but not the code under test and failed to link with 55 undefined references. compact_filter.c has no net dependency to justify the guard: it includes only cf_checkpoints, chainparams, hash, mem, serialize and utils, and references no node, nodegroup or event_base symbol. It is message serialization and filter header arithmetic, in the same category as golomb.c, which is already unconditional. Aligning CMake with Makefile.am rather than gating the test, so the two build systems describe the same library. WITH_NET=OFF: 74/74, was failing to link. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 80/80, unchanged.
compact_filter.h referenced nothing from either: no dogecoin_headers_db, no dogecoin_headers_db_interface, no dogecoin_blockindex. headersdb.h in particular includes logdb/logdb.h, so anything including compact_filter.h also needed -I src/logdb/include on its command line, even when it never touched logdb. That is invisible inside the library, whose CPPFLAGS already carry that path, and only shows up when something outside it -- a fuzz harness, a consumer -- includes the header directly and fails with include/dogecoin/headersdb.h:37:10: fatal error: 'logdb/logdb.h' file not found The header now pulls only what it uses. Removing the two lines leaves it self-contained given the include path for dogecoin's own headers. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 80/80. WITH_NET=OFF: 74/74.
Adds the file-backed stores behind the BIP157 filter header chain: cfheaders.dat holding height+filter_header records, and cfilters.dat holding the raw filter payloads. The on-disk layout is segregated per network, <datadir>/filter/basic/<chainname>/. Sharing one directory across chains is not merely untidy: a regtest or testnet client loads the mainnet cache at startup, rescans it, matches mainnet blocks its peer has never heard of, and then blocks forever waiting for them -- while writing its own filters back into the mainnet files. A one-time rename migrates a pre-segregation mainnet cache into the new location, and only mainnet, because only mainnet data is worth keeping. Bound the cfilters record length before allocating. data_len is read straight off disk and was passed to dogecoin_malloc unchecked, so a corrupt or tampered file could request up to 4 GiB (CWE-400). A cfilter cannot legitimately exceed the P2P message that delivered it, so DOGECOIN_MAX_P2P_MSG_SIZE is the ceiling. The short read that follows would have failed the call anyway, but only after the allocation had already been attempted. Tests cover the round trip, the genesis header at offset 8, reset(), in-memory mode, a trailing partial record (dropped rather than misparsed), and the length bound. The bound test asserts on the allocator rather than the return value. An earlier version of it passed with the bound removed: malloc(4 GiB) succeeds under overcommit and the oversized read then fails, so iterate() returns false either way and the return value cannot tell the two apart. It now installs a recording dogecoin_mem_mapper and asserts no allocation exceeded the bound, which was confirmed by removing the bound and watching the test fail. 81/81, and verified against a -DBUILD_SHARED_LIBS=1 configure.
The test hardcoded /tmp/libdogecoin_cfheaders_test.dat. Neither Windows nor Android has /tmp, so x86_64-win-native and aarch64-android failed: cfheadersdb: cannot open /tmp/libdogecoin_cfheaders_test.dat: No such file or directory Use plain relative filenames in the working directory, as the wallet and spv tests already do. Both files are removed at the end of the test. Same portability class as the truncate()/_chsize() call this test already avoids -- caught there, missed here.
Wires the BIP157 message flow into the SPV client: getcfcheckpt to pin
the peer's checkpoints, getcfheaders to build and validate the filter
header chain, getcfilters to fetch filters, GCS matching against watched
scripts, and a getdata for the blocks that matched.
Filter headers are persisted through the cfheaders/cfilters DBs added in
the previous layer, so a restart resumes from the stored tip rather than
re-downloading. Cached filters are rescanned at startup, which covers
scripts registered after the filters were stored.
BIP37 and BIP157 are mutually exclusive here, by design and for privacy:
a bloom filter tells the peer which scripts are being watched, which is
the disclosure compact filters exist to avoid. Compact filters are on by
default and dogecoin_spv_client_filterload fails closed while they are,
so a BIP37 consumer opts out explicitly via
dogecoin_spv_enable_compact_filters(client, false). The test asserts both
halves of that.
headersdb gains a block-hash-by-height lookup with a resume cursor.
cfheaders batches request ascending heights, and rescanning the whole
file per lookup is O(N^2) over a 6.2M-record file; resuming from the last
match keeps it linear.
Deliberately not included, to keep this reviewable and because each
belongs elsewhere:
- parallel cfilter/cfheaders download. The state fields live in the
compact_filter layer; the logic lands in its own PR. Every path here
takes the sequential branch.
- the par_hdr parallel genesis header download, which is dogecoinfoundation#378.
- skip_pow and the stored-hash-on-load shortcut. Both were in the
prototype branch, both weaken what header loading verifies, and
neither is needed by filter sync. They belong with the bulk-load work
where their trust assumptions can be reviewed as a unit. Header
loading here is unchanged from 0.1.5-dev.
Report the height range actually covered on completion. cf_scan_start_height
was only set on one of the two scan-start paths, and the log falls back to 1
when it is unset, so a run that covered the checkpoint tail reported
"scanned heights 1..tip" -- overstating coverage in exactly the way that
hides a gap.
Verified against a local 1.14.99 node serving BIP157 with the corrected
cfilter wire order, run with an isolated HOME so the real filter cache was
untouched: 1442 cfilters parsed off the wire, full
getcfcheckpt/getcfheaders/getcfilters round trip, zero validation errors or
misbehaviour, and the completion line now reports 6314395..6315836 rather
than 1..6315836.
That run does not exercise GCS matching: -a is not wired to the filter
state until the CLI layer, so no scripts were watched. The matching block
is byte-identical to the prototype's sequential arm, which was separately
confirmed to match at a predicted height over the wire, and the unit tests
cover GCS matching directly -- but this layer's own end-to-end run proves
the wire path, not the match path.
WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 81/81. WITH_NET=OFF: 74/74.
Connects the BIP157 client to the command line, which is what makes the
feature reachable by a user rather than only by the library API.
-a/--address registers the address as a watched script with the
filter state, so cfilters are matched against it
-e/--no_cfilters disables compact filters, the documented opt-out
for a consumer that wants BIP37 instead
-o logs filter header checkpoints during sync
--cfheaders_path override cfheaders.dat location
--cfilters_path override cfilters.dat location
wallet gains dogecoin_wallet_add_watchonly_addr so a watched address is
persisted without a private key. It follows the existing add-address path
exactly, including that dogecoin_address_to_pubkey_hash returns a pointer
into a static buffer and is not freed by its existing caller either.
Deliberately excluded, because each is the seam with another feature and
not the CLI's to own:
--cf_from_genesis, --genesis_headers, --filter_hash_db, --cf_workers
--genesis_headers is the clearest case. It calls
dogecoin_spv_client_enable_genesis_headers, which initialises par_hdr
*and* resets cf_start_height and clears the CF databases -- one function
spanning parallel header download and compact filters. Exposing it here
would pull dogecoinfoundation#378 into this PR. The peer-list replication that gave each
parallel worker its own TCP connection goes with them.
End to end against a local 1.14.99 node serving BIP157, run with an
isolated HOME so the real filter cache was untouched:
[bip157] watching address: DJoQKLWJtM251NhzsR7DmbgT88TwqBbPX7
[bip157] MATCH at height 6315000
[bip157] all filters processed: scanned heights 6314418..6315859,
1 matched blocks
[bip157] requesting 1 matched full blocks across 1 peers
[bip157] processing 2 txs from matched block height=6315000
1442 cfilters parsed off the wire, zero validation errors. The address was
taken from block 6315000 via getrawtransaction before the run, so the
matched height was predicted rather than read back afterwards.
This closes the gap left open in the spv layer, where -a was not yet wired
to the filter state and so GCS matching was never exercised end to end.
WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 81/81. WITH_NET=OFF: 74/74.
Actuates the parallel-download members that compact_filter.h already declares. Until now nothing set par_num_workers > 1, so every path took the sequential branch and those fields were inert. Splits the cfheaders and cfilter ranges across N connections to the peer, each worker owning a disjoint height range. cfilter results are buffered per worker and flushed to disk in height order, so the on-disk file stays sequential regardless of completion order. A worker that goes quiet past CF_RESPONSE_TIMEOUT has its slot reclaimed and the range reassigned, preferring a peer other than the one that stalled. --cf_workers N enables it, capped at 8: each worker is a separate TCP connection to the same peer, and past a handful the peer is the bottleneck. The peer list is repeated N times so the nodegroup opens one connection per worker. Fixes a chunk-anchoring bug found by running it rather than by reading it. Every chunk after the first anchors on the cfcheckpt filter header at (start - 1), which requires start - 1 to land exactly on a CFCHECKPT_INTERVAL boundary -- a precondition the code documented but did not enforce. Chunk size is already a multiple of the interval, so the condition holds for every chunk iff it holds for the first, and cfh_start is now snapped down to the nearest k*CFCHECKPT_INTERVAL + 1. Without that, a resume from height 6314590 gave chunk 1 a start of 6315590, and (6315590 - 1) / 1000 = 6315 selected the checkpoint for height 6315000: an anchor 589 blocks off. Every header the chunk derived was wrong, surfacing as [bip157-cfh-par] cfheader at height 6316000 FAILED checkpoint at the next boundary rather than at the point the bad anchor was chosen. A genesis sync starts at height 1 and satisfies the precondition by accident, so this only appears when resuming from a checkpoint -- which is the normal case. Verified against a local 1.14.99 node serving BIP157, isolated HOME: --cf_workers 4, chunks 6314001..6315000, 6315001..6316000, 6316001..6316047 -- every start now 1 mod 1000 all 3 cfheader chunks complete, no checkpoint failures 1437 cfilters off the wire, flushed in height order MATCH at height 6315000, 1 matched block, 2 txs processed zero validation errors, misbehaviour or stalls Sequential mode re-checked on this branch and unchanged: no par activity, same match at 6315000, zero errors. WITH_NET=ON with -DBUILD_SHARED_LIBS=1: 81/81. WITH_NET=OFF: 74/74.
xanimo
force-pushed
the
0.1.5-dev-cf-parallel
branch
from
August 5, 2026 21:04
6a9207b to
fd7f055
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| cstring *old_obj = NULL; | ||
| legacy_filter_path(&old_obj, filename); | ||
| if (stat(old_obj->str, &sb) == 0 && sb.st_size > 0) { | ||
| if (rename(old_obj->str, new_path) == 0) { |
| dogecoin_bool create = (stat(path, &sb) != 0) || (sb.st_size < (long)CF_HEADERS_FILE_HDR_LEN); | ||
|
|
||
| if (!create) { | ||
| db->file = fopen(path, "r+b"); |
| fclose(db->file); | ||
| db->file = NULL; | ||
| } | ||
| remove(path); |
| } | ||
|
|
||
| /* Create fresh v2 file */ | ||
| db->file = fopen(path, "w+b"); |
| dogecoin_spv_client* client = dogecoin_spv_client_new(chain, debug, in_memory_headers, use_checkpoint, full_sync, maxnodes, http_server); | ||
|
|
||
| if (logfile) { | ||
| g_logfile = fopen(logfile, "a"); |
CodeQL cpp/world-writable-file-creation on test/cfheadersdb_tests.c. fopen(path, "wb") leaves the mode to the process umask, so the file can land world-writable and another local user could rewrite it between creation and read. Creates it 0600 via open()+fdopen() on POSIX; Windows keeps fopen, which does not have the same exposure.
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
2 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.
Stacked on #387. Review only the top commit.
Actuates the parallel-download members that
compact_filter.halready declares(see the note on #381). Until now nothing set
par_num_workers > 1, so everypath took the sequential branch and those fields were inert. This PR adds no
new fields — only the logic that drives them.
Splits the cfheaders and cfilter ranges across N connections, each worker owning
a disjoint height range. cfilter results are buffered per worker and flushed to
disk in height order, so the on-disk file stays sequential regardless of
completion order. A worker quiet past
CF_RESPONSE_TIMEOUThas its slotreclaimed and its range reassigned, preferring a peer other than the one that
stalled.
--cf_workers N, capped at 8 — each worker is a separate TCP connection to thesame peer, and past a handful the peer is the bottleneck.
A chunk-anchoring bug, found by running it
Every chunk after the first anchors on the
cfcheckptfilter header atstart - 1, which requiresstart - 1to land exactly on aCFCHECKPT_INTERVALboundary. The code documented that precondition but didnot enforce it:
Resuming from height 6314590 gave chunk 1 a start of 6315590, and
(6315590 - 1) / 1000 = 6315selected the checkpoint for height 6315000 —an anchor 589 blocks off. Every header the chunk derived was then wrong,
surfacing as
at the next boundary rather than at the point the bad anchor was chosen.
Chunk size is already a multiple of the interval, so the condition holds for
every chunk iff it holds for the first.
cfh_startis now snapped down to thenearest
k*CFCHECKPT_INTERVAL + 1.Worth noting why this survived: a genesis sync starts at height 1, which
satisfies the precondition by accident. It only breaks when resuming from a
checkpoint — the normal case. Unit tests would not have caught it; it needed a
live resume against a real peer.
Verified
Against a local
1.14.99node serving BIP157, isolatedHOME:Sequential mode re-checked on this branch: no
paractivity, same match at6315000, zero errors — so enabling the feature did not disturb the default
path.
WITH_NET=ONwith-DBUILD_SHARED_LIBS=1: 81/81.WITH_NET=OFF: 74/74.