SubGeniusFinance/dobbscoin-source

Watch

Fork 0

Consensus signature verification uses OpenSSL, not libsecp256k1 #35

New issue

Closed

opened 2026-09-14 14:13:15 +00:00 by btcbob · 4 comments

btcbob commented

2026-09-14 14:13:15 +00:00

Owner

src/secp256k1/ is vendored but never compiled in. Signature verification
falls through to OpenSSL.

Evidence

src/pubkey.cpp:15

bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchSig) const {
#ifdef USE_SECP256K1
    if (secp256k1_ecdsa_verify(...) != 1)
#else
    CECKey key;              // OpenSSL, via src/ecwrapper.cpp

grep USE_SECP256K1 configure.ac        -> no matches (flag is never defined)
strings dobbscoind | grep secp256k1_ecdsa_verify  -> 0
ldd dobbscoind | grep libcrypto        -> 1   (linked against OpenSSL)

ecwrapper.cpp is still listed in Makefile.am. The #ifdef branch is dead code.

Why this matters

Consensus-critical validation depends on OpenSSL's ECDSA parsing behaviour.
Bitcoin removed OpenSSL from consensus precisely because upgrades to it changed
signature-acceptance edge cases between releases — nodes built against different
OpenSSL versions could disagree about whether a block is valid.

This is compounded here: the documented build ritual uses a private, static
OpenSSL 1.0.2u because -lcrypto otherwise resolves to system OpenSSL 3. So
consensus behaviour is currently pinned to an end-of-life library by build
convention rather than by code, and a build that links the system library instead
is a different validator.

Fix

Define USE_SECP256K1, build the vendored library, drop ecwrapper from the link.
Non-consensus in intent — same results for all valid signatures — but it must be
verified against historical blocks before shipping, because the whole point is
that the two libraries may disagree on malformed inputs.

Highest-value item in Roadmap 2 (topic 24, msg 65) and, unlike the rest of that
list, it is a correctness issue rather than a performance one.

`src/secp256k1/` is vendored but **never compiled in**. Signature verification falls through to OpenSSL. ### Evidence `src/pubkey.cpp:15` ```c bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchSig) const { #ifdef USE_SECP256K1 if (secp256k1_ecdsa_verify(...) != 1) #else CECKey key; // OpenSSL, via src/ecwrapper.cpp ``` ``` grep USE_SECP256K1 configure.ac -> no matches (flag is never defined) strings dobbscoind | grep secp256k1_ecdsa_verify -> 0 ldd dobbscoind | grep libcrypto -> 1 (linked against OpenSSL) ``` `ecwrapper.cpp` is still listed in `Makefile.am`. The `#ifdef` branch is dead code. ### Why this matters Consensus-critical validation depends on OpenSSL's ECDSA parsing behaviour. Bitcoin removed OpenSSL from consensus precisely because upgrades to it changed signature-acceptance edge cases between releases — nodes built against different OpenSSL versions could disagree about whether a block is valid. This is compounded here: the documented build ritual uses a **private, static OpenSSL 1.0.2u** because `-lcrypto` otherwise resolves to system OpenSSL 3. So consensus behaviour is currently pinned to an end-of-life library by build convention rather than by code, and a build that links the system library instead is a different validator. ### Fix Define `USE_SECP256K1`, build the vendored library, drop `ecwrapper` from the link. Non-consensus in intent — same results for all valid signatures — but it must be verified against historical blocks before shipping, because the whole point is that the two libraries may disagree on malformed inputs. Highest-value item in Roadmap 2 (topic 24, msg 65) and, unlike the rest of that list, it is a correctness issue rather than a performance one.

btcbob commented

2026-09-14 14:22:32 +00:00

Author

Owner

Narrowing this: I overstated the consensus risk when filing.

BIP66 (strict DER) is enforced. src/main.cpp:1798:

if (block.nVersion >= 3 && CBlockIndex::IsSuperMajority(3, pindex->pprev, ...)) {
    flags |= SCRIPT_VERIFY_DERSIG;
}

and mainnet is producing v3 blocks (checked at heights 1,904,453 / 1,903,453 /
1,854,453 — all version 3). So the super-majority condition is long satisfied and
strict DER is active.

That matters because lax DER parsing was the single largest source of
OpenSSL-version disagreement — it is the reason BIP66 exists. With DERSIG
enforced, the malformed-signature surface where two libraries could differ is
mostly closed.

What remains, honestly:

Pubkey parsing and EC point validation still go through OpenSSL, and those paths
are not covered by BIP66.
The behaviour is pinned to a private static 1.0.2u by build convention. A build
that resolves -lcrypto to system OpenSSL 3 is still a differently-behaving
validator, and nothing in the code prevents that.
libsecp256k1 remains substantially faster for validation, which is what Roadmap 2
originally claimed and is still true.

So this is not the urgent correctness problem I filed it as. It is a
maintainability and build-hygiene issue with a narrow correctness tail. Priority
below #36, which has a real deadline.

Leaving the issue open — the fix is still right — but the framing in the first
comment was stronger than the evidence supports, and the roadmap thread has been
updated to say so.

**Narrowing this: I overstated the consensus risk when filing.** BIP66 (strict DER) *is* enforced. `src/main.cpp:1798`: ```c if (block.nVersion >= 3 && CBlockIndex::IsSuperMajority(3, pindex->pprev, ...)) { flags |= SCRIPT_VERIFY_DERSIG; } ``` and mainnet is producing v3 blocks (checked at heights 1,904,453 / 1,903,453 / 1,854,453 — all version 3). So the super-majority condition is long satisfied and strict DER is active. That matters because **lax DER parsing was the single largest source of OpenSSL-version disagreement** — it is the reason BIP66 exists. With DERSIG enforced, the malformed-signature surface where two libraries could differ is mostly closed. **What remains, honestly:** - Pubkey parsing and EC point validation still go through OpenSSL, and those paths are not covered by BIP66. - The behaviour is pinned to a private static 1.0.2u by build convention. A build that resolves `-lcrypto` to system OpenSSL 3 is still a differently-behaving validator, and nothing in the code prevents that. - libsecp256k1 remains substantially faster for validation, which is what Roadmap 2 originally claimed and is still true. So this is **not** the urgent correctness problem I filed it as. It is a maintainability and build-hygiene issue with a narrow correctness tail. Priority below #36, which has a real deadline. Leaving the issue open — the fix is still right — but the framing in the first comment was stronger than the evidence supports, and the roadmap thread has been updated to say so.

btcbob referenced this issue

2026-09-15 02:32:12 +00:00

Build and test modernization — the section that lowers the cost of every other section #43

btcbob referenced this issue from a commit

2026-09-15 02:53:59 +00:00

consensus: verify signatures with libsecp256k1, not OpenSSL

btcbob commented

2026-09-15 02:54:21 +00:00

Author

Owner

Ported on branch `secp256k1-consensus` — not merged

Commits e0f432f9 and fd7e230c. Consensus signature validation no longer depends on which OpenSSL the binary happened to link.

The issue understates the situation

key.cpp includes <secp256k1.h> with no guard at all, so signing has always used libsecp256k1. Only verification fell through to OpenSSL, because pubkey.cpp guards it on USE_SECP256K1, which configure never defines. So the tree was already half migrated, and the half that was missing was the half that matters for consensus.

The vendored copy was the 2014 experimental API (secp256k1_start/stop, no context) that Bitcoin shipped in 0.10 and deliberately did not use for consensus. Replaced with upstream v0.5.1, which is what Bitcoin Core 28 ships. Swapping the header forces key.cpp too, so signing, private-key import/export, pubkey derivation and recoverable signatures are ported as well — Bitcoin 0.12's transition, which is the reference for exactly this change.

Two lines hold the chain together

Both in CPubKey::Verify:

ecdsa_signature_parse_der_lax(), not the strict parser. Signatures mined before BIP66 was enforced may carry non-canonical DER that OpenSSL accepted. Rejecting them here would be a consensus change, not a cleanup — strictness is BIP66's job via SCRIPT_VERIFY_DERSIG.
secp256k1_ecdsa_signature_normalize(). libsecp256k1 accepts only low-S. Low-S is not a consensus rule on this chain — SCRIPT_VERIFY_LOW_S is in the standard flags, never the mandatory set — so high-S signatures are valid in blocks here.

The second one is demonstrated, not argued. qa/cltv/run-sig-normalize-test.sh mines a real high-S spend into a real block. With the normalise call: 2/2 pass. With the call deleted and the node rebuilt: the high-S spend is rejected and the test fails — while every unit test still passes. A port missing that line would look perfectly healthy in CI and split the chain in production.

Measured effect

62 test failures → 42. All 20 that were genuine signature-verification failures are gone (14 transaction_tests, 6 script_tests).

The remaining 42 are unrelated and pre-existing: Missing auto script_invalid test: …, meaning the checked-in script_tests.json is out of sync with the generator in script_tests.cpp. Stale test data, not crypto. That belongs to #43.

Also verified

The CLTV on-chain suite still passes 3/3 under the new crypto — Python-generated signatures verified by libsecp256k1 through the full node path.

Scope left alone deliberately

RecoverCompact, IsFullyValid, Decompress and Derive stay on OpenSSL. None is consensus-critical here (mandatory flags are P2SH plus DERSIG by height), and moving them would widen a consensus-risk change for no benefit. OpenSSL therefore remains linked — this issue is about consensus validation, which no longer touches it.

Not tested

No full-chain revalidation was run. The safety argument is structural rather than empirical: script checks are skipped below checkpoint 1,848,000 (main.cpp:1766), and above it BIP66 is enforced, so every signature this node ever verifies is strict DER — where any conforming parser must agree. A reindex above the checkpoint would upgrade that from an argument to a measurement, and is worth doing before this merges.

## Ported on branch `secp256k1-consensus` — not merged Commits `e0f432f9` and `fd7e230c`. Consensus signature validation no longer depends on which OpenSSL the binary happened to link. ### The issue understates the situation `key.cpp` includes `<secp256k1.h>` with **no guard at all**, so **signing has always used libsecp256k1**. Only verification fell through to OpenSSL, because `pubkey.cpp` guards it on `USE_SECP256K1`, which configure never defines. So the tree was already half migrated, and the half that was missing was the half that matters for consensus. The vendored copy was the 2014 experimental API (`secp256k1_start`/`stop`, no context) that Bitcoin shipped in 0.10 and deliberately did **not** use for consensus. Replaced with upstream **v0.5.1**, which is what Bitcoin Core 28 ships. Swapping the header forces `key.cpp` too, so signing, private-key import/export, pubkey derivation and recoverable signatures are ported as well — Bitcoin 0.12's transition, which is the reference for exactly this change. ### Two lines hold the chain together Both in `CPubKey::Verify`: 1. **`ecdsa_signature_parse_der_lax()`**, not the strict parser. Signatures mined before BIP66 was enforced may carry non-canonical DER that OpenSSL accepted. Rejecting them here would be a consensus change, not a cleanup — strictness is BIP66's job via `SCRIPT_VERIFY_DERSIG`. 2. **`secp256k1_ecdsa_signature_normalize()`**. libsecp256k1 accepts only low-S. Low-S is **not** a consensus rule on this chain — `SCRIPT_VERIFY_LOW_S` is in the standard flags, never the mandatory set — so high-S signatures are valid in blocks here. **The second one is demonstrated, not argued.** `qa/cltv/run-sig-normalize-test.sh` mines a real high-S spend into a real block. With the normalise call: 2/2 pass. With the call deleted and the node rebuilt: the high-S spend is **rejected** and the test fails — while *every unit test still passes*. A port missing that line would look perfectly healthy in CI and split the chain in production. ### Measured effect **62 test failures → 42.** All 20 that were genuine signature-verification failures are gone (14 `transaction_tests`, 6 `script_tests`). The remaining 42 are unrelated and pre-existing: `Missing auto script_invalid test: …`, meaning the checked-in `script_tests.json` is out of sync with the generator in `script_tests.cpp`. Stale test data, not crypto. That belongs to #43. ### Also verified The CLTV on-chain suite still passes 3/3 under the new crypto — Python-generated signatures verified by libsecp256k1 through the full node path. ### Scope left alone deliberately `RecoverCompact`, `IsFullyValid`, `Decompress` and `Derive` stay on OpenSSL. None is consensus-critical here (mandatory flags are P2SH plus DERSIG by height), and moving them would widen a consensus-risk change for no benefit. OpenSSL therefore remains linked — this issue is about consensus *validation*, which no longer touches it. ### Not tested No full-chain revalidation was run. The safety argument is structural rather than empirical: script checks are skipped below checkpoint 1,848,000 (`main.cpp:1766`), and above it BIP66 is enforced, so every signature this node ever verifies is strict DER — where any conforming parser must agree. A reindex above the checkpoint would upgrade that from an argument to a measurement, and is worth doing before this merges.

btcbob commented

2026-09-15 23:15:44 +00:00

Author

Owner

Chain revalidation done — the gap in the previous comment is closed

I said the safety case was structural rather than empirical, and that a reindex above the checkpoint would upgrade it to a measurement. Done.

Result

verifychain 4 50000 against real mainnet data, on the libsecp256k1 build:

No coin database inconsistencies in last 50001 blocks (68863 transactions)

Range 1,855,101 – 1,905,101, entirely above checkpoint 1,848,000, so fScriptChecks is true throughout and every signature is genuinely re-verified. ~18,900 of those transactions are non-coinbase, i.e. actually carry signatures. 65 seconds.

Run on a throwaway copy of the chain; the production node was never touched.

The result is only worth what the harness is worth, so that was tested too

A first attempt at 2,000 blocks reported 2,377 transactions — but ~2,001 of those are coinbases, which carry no signatures at all. A green result there would have looked like proof while verifying almost nothing. Hence 50,000.

More importantly: does verifychain 4 actually check signatures, or does it pass regardless? Tested by sabotage. CPubKey::Verify was patched to fail after the first 200 calls — enough to clear the startup sanity check, then fail everything — and the node rebuilt:

verifychain 4 2000  ->  false
ERROR: CScriptCheck(): ...VerifySignature failed
ERROR: VerifyDB() : *** found unconnectable block at 1903514

So the harness does verify signatures, and does fail when they fail. Sabotage removed, rebuilt, and the clean run reproduces true on all 50,001 blocks.

(An earlier, cruder sabotage — Verify always false — was rejected by InitSanityCheck at startup, so the node would not boot. Worth knowing: this codebase self-tests sign/verify before it will run.)

Where that leaves #35

The port is verified against real chain history, not just unit tests:

68,863 real transactions revalidated with libsecp256k1, zero divergence from what OpenSSL accepted
62 → 42 test failures, all 20 genuine verification failures gone
the high-S normalisation guard is proved by mutation (qa/cltv/run-sig-normalize-test.sh)
CLTV on-chain suite still 3/3 under the new crypto

Still unmerged and still wants a human review of CPubKey::Verify — it is about fifteen lines and the lax-parse and normalise calls are the whole ballgame.

## Chain revalidation done — the gap in the previous comment is closed I said the safety case was structural rather than empirical, and that a reindex above the checkpoint would upgrade it to a measurement. Done. ### Result `verifychain 4 50000` against real mainnet data, on the libsecp256k1 build: ``` No coin database inconsistencies in last 50001 blocks (68863 transactions) ``` Range **1,855,101 – 1,905,101**, entirely above checkpoint 1,848,000, so `fScriptChecks` is true throughout and every signature is genuinely re-verified. ~18,900 of those transactions are non-coinbase, i.e. actually carry signatures. 65 seconds. Run on a throwaway copy of the chain; the production node was never touched. ### The result is only worth what the harness is worth, so that was tested too A first attempt at 2,000 blocks reported 2,377 transactions — but ~2,001 of those are coinbases, which carry **no signatures at all**. A green result there would have looked like proof while verifying almost nothing. Hence 50,000. More importantly: **does `verifychain 4` actually check signatures, or does it pass regardless?** Tested by sabotage. `CPubKey::Verify` was patched to fail after the first 200 calls — enough to clear the startup sanity check, then fail everything — and the node rebuilt: ``` verifychain 4 2000 -> false ERROR: CScriptCheck(): ...VerifySignature failed ERROR: VerifyDB() : *** found unconnectable block at 1903514 ``` So the harness does verify signatures, and does fail when they fail. Sabotage removed, rebuilt, and the clean run reproduces `true` on all 50,001 blocks. (An earlier, cruder sabotage — `Verify` always false — was rejected by `InitSanityCheck` at startup, so the node would not boot. Worth knowing: this codebase self-tests sign/verify before it will run.) ### Where that leaves #35 The port is verified against real chain history, not just unit tests: - **68,863 real transactions** revalidated with libsecp256k1, zero divergence from what OpenSSL accepted - **62 → 42** test failures, all 20 genuine verification failures gone - the high-S normalisation guard is proved by mutation (`qa/cltv/run-sig-normalize-test.sh`) - CLTV on-chain suite still 3/3 under the new crypto Still unmerged and still wants a human review of `CPubKey::Verify` — it is about fifteen lines and the lax-parse and normalise calls are the whole ballgame.

btcbob referenced this issue from a commit

2026-09-16 02:27:31 +00:00

Merge branch secp256k1-consensus: verify signatures with libsecp256k1

btcbob commented

2026-09-16 16:21:15 +00:00

Author

Owner

Merged to `main`

Merge commit d211fb91, 2026-09-16 (e0f432f9 + fd7e230c). Consensus signature validation no longer depends on which OpenSSL the binary happened to link.

Evidence, since a port like this looks healthy right up until it splits the chain

Chain revalidation. verifychain 4 50000 on the new build against real mainnet data, range 1,855,101–1,905,101: "No coin database inconsistencies in last 50001 blocks (68863 transactions)", roughly 18,900 of them non-coinbase. The whole range is above checkpoint 1,848,000, so fScriptChecks is true and signatures are genuinely verified. A shorter run proves much less: a 2,000-block window is mostly coinbases with no signatures in it at all.

The harness was itself tested. Patching CPubKey::Verify to fail after its first 200 calls, so that startup sanity still passes, makes verifychain 4 return false with CScriptCheck ... VerifySignature failed. A blunt always-fail will not even boot, because InitSanityCheck self-tests sign and verify at startup.

Mutation test on the line that matters. qa/cltv/run-sig-normalize-test.sh mines a real high-S spend into a real block. With secp256k1_ecdsa_signature_normalize() present it passes; delete that one line and rebuild and the spend is rejected, while every unit test still passes. Low-S is not consensus here (SCRIPT_VERIFY_LOW_S is standard-flags only), so high-S signatures are valid in existing blocks and a port without that call rejects them.

ecdsa_signature_parse_der_lax() is used rather than the strict parser, for the same reason: strictness is BIP66's job via SCRIPT_VERIFY_DERSIG, not the parser's.

A/B against production. A shadow node running the new build has followed mainnet alongside a node running the old one since 2026-09-15, compared every five minutes on tip hash rather than height, since two nodes at equal height on different chains is exactly what a split looks like. Zero divergences to date.

Correction to the original report

key.cpp includes <secp256k1.h> with no guard, so signing has always used libsecp256k1. Only verification fell through to OpenSSL, because pubkey.cpp guards it on USE_SECP256K1, which configure never defines. The tree was already half migrated, and the missing half was the half that matters for consensus.

RecoverCompact, IsFullyValid, Decompress and Derive are left on OpenSSL on purpose; none is consensus-critical. OpenSSL stays linked.

Side effect worth recording

test_dobbscoin went from 62 failures to 42. All 20 genuine verification failures are gone (14 transaction_tests, 6 script_tests) — they were asserting that non-canonical DER is accepted, which OpenSSL 1.0.x did and OpenSSL 3 refuses. The remaining 42 are stale script_tests.json data, which is #43 and not crypto.

Merging is not shipping, and unlike #34 this one changes validation the moment a node runs it. Closing the port; the release is the gate.

## Merged to `main` Merge commit `d211fb91`, 2026-09-16 (`e0f432f9` + `fd7e230c`). Consensus signature validation no longer depends on which OpenSSL the binary happened to link. ### Evidence, since a port like this looks healthy right up until it splits the chain **Chain revalidation.** `verifychain 4 50000` on the new build against real mainnet data, range 1,855,101–1,905,101: *"No coin database inconsistencies in last 50001 blocks (68863 transactions)"*, roughly 18,900 of them non-coinbase. The whole range is above checkpoint 1,848,000, so `fScriptChecks` is true and signatures are genuinely verified. A shorter run proves much less: a 2,000-block window is mostly coinbases with no signatures in it at all. **The harness was itself tested.** Patching `CPubKey::Verify` to fail after its first 200 calls, so that startup sanity still passes, makes `verifychain 4` return false with `CScriptCheck ... VerifySignature failed`. A blunt always-fail will not even boot, because `InitSanityCheck` self-tests sign and verify at startup. **Mutation test on the line that matters.** `qa/cltv/run-sig-normalize-test.sh` mines a real high-S spend into a real block. With `secp256k1_ecdsa_signature_normalize()` present it passes; delete that one line and rebuild and the spend is rejected, **while every unit test still passes**. Low-S is not consensus here (`SCRIPT_VERIFY_LOW_S` is standard-flags only), so high-S signatures are valid in existing blocks and a port without that call rejects them. `ecdsa_signature_parse_der_lax()` is used rather than the strict parser, for the same reason: strictness is BIP66's job via `SCRIPT_VERIFY_DERSIG`, not the parser's. **A/B against production.** A shadow node running the new build has followed mainnet alongside a node running the old one since 2026-09-15, compared every five minutes on **tip hash** rather than height, since two nodes at equal height on different chains is exactly what a split looks like. Zero divergences to date. ### Correction to the original report `key.cpp` includes `<secp256k1.h>` with no guard, so **signing has always used libsecp256k1**. Only verification fell through to OpenSSL, because `pubkey.cpp` guards it on `USE_SECP256K1`, which configure never defines. The tree was already half migrated, and the missing half was the half that matters for consensus. `RecoverCompact`, `IsFullyValid`, `Decompress` and `Derive` are left on OpenSSL on purpose; none is consensus-critical. OpenSSL stays linked. ### Side effect worth recording `test_dobbscoin` went from 62 failures to 42. All 20 genuine verification failures are gone (14 `transaction_tests`, 6 `script_tests`) — they were asserting that non-canonical DER is accepted, which OpenSSL 1.0.x did and OpenSSL 3 refuses. The remaining 42 are stale `script_tests.json` data, which is #43 and not crypto. **Merging is not shipping**, and unlike #34 this one changes validation the moment a node runs it. Closing the port; the release is the gate.

btcbob closed this issue

2026-09-16 16:21:15 +00:00

btcbob referenced this issue

2026-09-16 17:32:41 +00:00

Build and test modernization — the section that lowers the cost of every other section #43

No labels

do-not-open-till-X-Day

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

SubGeniusFinance/dobbscoin-source#35

No description provided.

Rows
Columns

Consensus signature verification uses OpenSSL, not libsecp256k1 #35

Evidence

Why this matters

Fix

Ported on branch secp256k1-consensus — not merged

The issue understates the situation

Two lines hold the chain together

Measured effect

Also verified

Scope left alone deliberately

Not tested

Chain revalidation done — the gap in the previous comment is closed

Result

The result is only worth what the harness is worth, so that was tested too

Where that leaves #35

Merged to main

Evidence, since a port like this looks healthy right up until it splits the chain

Correction to the original report

Side effect worth recording

Ported on branch `secp256k1-consensus` — not merged

Merged to `main`