chainparams: sync the public dogecoin_chainparams with the internal one - #400
Open
xanimo wants to merge 1 commit into
Open
chainparams: sync the public dogecoin_chainparams with the internal one#400xanimo wants to merge 1 commit into
xanimo wants to merge 1 commit into
Conversation
edtubbs
approved these changes
Aug 5, 2026
edtubbs
left a comment
Member
There was a problem hiding this comment.
ACK, good catch on the layout drift and great regression coverage as well.
dogecoin_chainparams is declared twice by design: once in the public
libdogecoin.h so the installed header is self-contained, once in the
internal chainparams.h. The objects are defined once, in chainparams.c,
against the internal declaration and read through the public one, so the
two must describe the same layout.
They had drifted. The public copy was missing genesisblockchainwork,
strict_id, auxpow_id, pow_limit and minimumchainwork. Only the first of
those matters for layout, and it matters a great deal: it sits in the
middle rather than at the end, so this was not a harmless truncation.
Every field after it landed 32 bytes early. A consumer reading
dogecoin_chainparams_main.default_port
through the public header read offset 84 while the object had it at 116,
returning the low four bytes of the chainwork instead of 22556. dnsseeds
shifted with it, so the seed list read into the middle of a hostname.
sizeof reported 2136 against an object of 2236, so stack-allocating or
copying a dogecoin_chainparams was short by 100 bytes. None of it warns.
Adds the missing fields in the order chainparams.c defines them, and a
comment on both declarations saying they move together.
Adds test/chainparams_abi_tests.c, which includes only libdogecoin.h and
reads the exported objects the way a consumer does. Including both
headers to compare them is not possible -- C forbids redefining a struct
tag even identically -- so the test checks values instead: they agree
only if the declaration a consumer sees matches the one the objects were
built with. Verified it fails on both drift modes. Removing a field
fails at compile time; inserting one the internal copy lacks compiles
clean and fails at runtime, with default_port reading 1684366707.
The test asserts on minimumchainwork rather than pow_limit deliberately,
so that it fails on layout drift only and not on the pow_limit byte
order change under separate review.
Registered in both build systems. 79/79 under CMake and autotools.
xanimo
force-pushed
the
0.1.5-dev-chainparams-abi
branch
from
August 5, 2026 21:03
1005620 to
121d84e
Compare
xanimo
added a commit
to xanimo/libdogecoin
that referenced
this pull request
Aug 7, 2026
The checkpoint arrays were declared with hardcoded bounds that did not
match what chainparams.c defines:
chainparams.h dogecoin_mainnet_checkpoint_array[87]
libdogecoin.h dogecoin_mainnet_checkpoint_array[33]
chainparams.c 89 entries
Every caller counted them with sizeof(array)/sizeof(array[0]). On an
extern array that yields whatever bound the header states, not the real
length, and it does so silently. So the parallel header downloader built
its segments from the first 87 checkpoints and a consumer including
libdogecoin.h would have seen 33.
This was not theoretical: the two checkpoints added in the previous
commit had no effect at all. The downloader still built 86 segments. It
compiled, the suite passed, and the data was simply ignored. With counts
exported from the same translation unit that defines the arrays, it
builds 88.
Both headers now declare the arrays unsized and export
dogecoin_mainnet_checkpoint_count / dogecoin_testnet_checkpoint_count
alongside them. Removing the bogus bounds turned every remaining misuse
into a compile error rather than leaving it silently wrong -- four more
sites in src/cli/spvnode.c, eight expressions in total, which is a fair
argument for the unsized declaration on its own.
Same shape as the chainparams struct in dogecoinfoundation#400: one object, two
declarations, drift nobody could see. sizeof on an extern array with a
declared bound cannot be trusted, and there is now no such expression
left in the tree.
78/78, and the downloader reports 88 segments.
xanimo
added a commit
to xanimo/libdogecoin
that referenced
this pull request
Aug 7, 2026
The checkpoint arrays were declared with hardcoded bounds that did not
match what chainparams.c defines:
chainparams.h dogecoin_mainnet_checkpoint_array[87]
libdogecoin.h dogecoin_mainnet_checkpoint_array[33]
chainparams.c 89 entries
Every caller counted them with sizeof(array)/sizeof(array[0]). On an
extern array that yields whatever bound the header states, not the real
length, and it does so silently. So the parallel header downloader built
its segments from the first 87 checkpoints and a consumer including
libdogecoin.h would have seen 33.
This was not theoretical: the two checkpoints added in the previous
commit had no effect at all. The downloader still built 86 segments. It
compiled, the suite passed, and the data was simply ignored. With counts
exported from the same translation unit that defines the arrays, it
builds 88.
Both headers now declare the arrays unsized and export
dogecoin_mainnet_checkpoint_count / dogecoin_testnet_checkpoint_count
alongside them. Removing the bogus bounds turned every remaining misuse
into a compile error rather than leaving it silently wrong -- four more
sites in src/cli/spvnode.c, eight expressions in total, which is a fair
argument for the unsized declaration on its own.
Same shape as the chainparams struct in dogecoinfoundation#400: one object, two
declarations, drift nobody could see. sizeof on an extern array with a
declared bound cannot be trusted, and there is now no such expression
left in the tree.
78/78, and the downloader reports 88 segments.
xanimo
added a commit
to xanimo/libdogecoin
that referenced
this pull request
Aug 7, 2026
The checkpoint arrays were declared with hardcoded bounds that did not
match what chainparams.c defines:
chainparams.h dogecoin_mainnet_checkpoint_array[87]
libdogecoin.h dogecoin_mainnet_checkpoint_array[33]
chainparams.c 90 entries
Every caller counted them with sizeof(array)/sizeof(array[0]). On an
extern array that yields whatever bound the header states, not the real
length, and it does so silently. So the parallel header downloader built
its segments from the first 87 checkpoints, and a consumer including
libdogecoin.h would have seen 33.
This was not theoretical: the checkpoints added in the previous commit
had no effect at all. The downloader still built 86 segments. It
compiled, the suite passed, and the data was simply ignored. With counts
exported from the translation unit that defines the arrays, it builds 89.
Both headers now declare the arrays unsized and export
dogecoin_mainnet_checkpoint_count / dogecoin_testnet_checkpoint_count
alongside them. Removing the bogus bounds turned every remaining misuse
into a compile error rather than leaving it silently wrong -- four more
sites in src/cli/spvnode.c, eight expressions in total, which is a fair
argument for the unsized declaration on its own.
Same shape as the chainparams struct in dogecoinfoundation#400: one object, two
declarations, drift nobody could see. sizeof on an extern array with a
declared bound cannot be trusted, and there is no such expression left in
the tree.
78/78, and the downloader reports 89 segments.
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
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.
dogecoin_chainparamsis declared twice by design — once in the publiclibdogecoin.hso the installed header stays self-contained, once in the internalchainparams.h. The objects are defined once, inchainparams.c, against the internal declaration and read through the public one. That arrangement works only if the two describe the same layout.They had drifted. The public copy was missing
genesisblockchainwork,strict_id,auxpow_id,pow_limitandminimumchainwork.Only the first matters for layout, and it matters a lot: it sits in the middle, not at the end, so this was not a harmless truncation. Every field after it landed 32 bytes early.
offsetof(default_port)offsetof(dnsseeds)sizeofgenesisblockchainworkitself is at offset 84 — exactly where the public header putdefault_port. So a consumer writing the obvious thing:read the first four bytes of the chainwork instead.
dnsseedsshifted with it, so the seed list read into the middle of a hostname, and anything that stack-allocated or copied adogecoin_chainparamswas short by 100 bytes. None of it warns — the declaration is self-consistent, just not the one the object was built with.Blast radius
In-tree code was not miscompiled. I checked every
src/andtest/file that includeslibdogecoin.hwithout also includingchainparams.h:src/context.cdoes see the short declaration, but only stores, compares and returnsconst dogecoin_chainparams*— no field is read through ittest/context_tests.creadschainname, which is at offset 0, before the divergenceSo this is an installed-header bug, and the people it hit are downstream consumers, not us. Ed's #396 happens to remove both of those in-tree exposures by switching them to include
chainparams.hdirectly — complementary, but it does not touch this.Change
Adds the missing fields in the order
chainparams.cdefines them, plus a comment on the declaration saying the two move together. No behaviour change inside the library, per the above. It is an ABI-visible change to the installed header, in the sense that it makes the header agree with an object that has been 2236 bytes all along — consumers need a recompile, and after it they get the right answers.Test
test/chainparams_abi_tests.cincludes onlylibdogecoin.hand reads the exported objects the way a consumer does. Including both headers to compare them directly is not possible — C forbids redefining a struct tag even identically — so the test checks values instead. They agree only if the declaration a consumer sees matches the one the objects were built with.Verified against both drift modes rather than assumed:
default_portreading1684366707It asserts on
minimumchainworkrather thanpow_limitdeliberately, so it fails on layout drift only and not on thepow_limitbyte-order change proposed in #398.Registered in both build systems. 79/79 under CMake and autotools.
Noted, not fixed here
auxpow_idis typeddogecoin_bool(auint8_t) but holds the AuxPoW chain ID —0x62on all three networks, not a boolean. Dogecoin's fits in a byte so nothing is broken today, but a chain ID above 255 would truncate silently. Retyping it is a separate change; the test asserts the value it actually holds rather thantrue.