Enterprise hardening: work-based consensus, CI, threat model, fuzz + property + multi-node tests - #2
Merged
Merged
Conversation
The repository was never formatted with rustfmt, so a `cargo fmt --all -- --check` gate would have failed on every file. Apply the default rustfmt style once, in its own commit, so the CI format job added next has a clean baseline and future diffs stay free of formatting noise. No behaviour changes: all 63 unit tests and 7 doc-tests still pass and clippy is clean.
Nothing enforced the quality bar the recent hardening work established, so the next change could quietly undo it. Add a CI workflow that runs on every push and pull request with five jobs: rustfmt in check mode, clippy with warnings denied, the test suite, a release build, and a cargo-audit advisory scan. Build jobs run --locked so a stale Cargo.lock fails loudly, and share a cache; action versions are pinned. Alongside it, the hygiene the repository was missing: - CHANGELOG.md in Keep a Changelog format, with an Unreleased section recording the consensus, replay, framing and panic fixes from the audit plus this pipeline. - LICENSE, so the MIT badge the README already carried points at real text instead of a 404. - rust-version = "1.90" in Cargo.toml, with the README badge and the prerequisites section moved off the stale 1.70 claim. - CI and edition badges at the top of the README. Verified: cargo fmt --all -- --check, cargo clippy --all-targets --all-features --locked -- -D warnings, cargo test --all-features --locked (63 unit + 7 doc tests passing) and cargo build --release --locked all green on rustc 1.94.0. The audit job is not runnable locally - cargo-audit is not installed here - so it is wired to rustsec/audit-check rather than a local install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A node parses bytes it did not write: peer messages, a chain file on disk, a wallet file. Anything on those paths that can abort the process is a remote kill switch, so the crate now denies `clippy::unwrap_used`, `expect_used`, `panic` and `unwrap_in_result` at the root of both the library and the binary, with a `cfg(test)` allow so tests may still unwrap. Two library sites had to change to satisfy it: - `Blockchain::latest_block` panicked on an empty chain. `chain` is a public field, so a chainless `Blockchain` is a value a caller can construct; it now returns `Option<&Block>` and the callers that need a tip report the new `BlockchainError::EmptyChain`. A peer sending `GetLatestBlock` to a node with no chain gets no answer rather than killing it. - `is_valid` reached the genesis block by index behind a separate emptiness check; it now takes the two together with `first()`. The one remaining `expect` is in `Blockchain::new`, indexing the compile-time genesis block, and carries a scoped allow plus a SAFETY note. It is pinned by `genesis_indexes_without_error`, so a future change to the genesis block fails a test instead of aborting a node at startup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
Difficulty was a hardcoded const, so the chain's cost per block never responded to how fast blocks actually arrived and every node was free to pick its own number. Make it a property of the chain instead. Every RETARGET_INTERVAL (10) blocks the wall-clock span of the window that just closed is compared with TARGET_BLOCK_TIME_SECS (60) per block interval: more than 2x too fast raises the difficulty one step, more than 2x too slow lowers it one step, and in between a block inherits its parent's. One step is one leading hex zero, a factor of 16 in work, so the quantisation is itself the per-retarget clamp, stricter than Bitcoin's 4x limit. The result is held within MIN_DIFFICULTY (1) and MAX_DIFFICULTY (32). Genesis is excluded from every window because its timestamp is a fixed determinism constant, not a mining time. Blocks now carry the difficulty they were mined at, and it is part of the hash preimage, so it cannot be relabelled after the fact. Block acceptance and whole-chain validation both re-derive the required difficulty from the chain prefix and reject any block whose claim does not match, the claim is never trusted. Mining uses the same derivation rather than the node's setting. `Blockchain::difficulty` is kept as the chain's *starting* difficulty, so `with_difficulty` and the `--difficulty` flag keep working; chains shorter than one retarget interval behave exactly as before. Fork choice is still longest-chain. With variable difficulty length is only a proxy for accumulated work, which is noted in the README as the natural follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
The unit tests pin down the bugs review already found. These two additions check the same rules against inputs nobody chose. Property tests (`tests/properties.rs`, proptest) assert the invariants the chain rests on over randomized sequences: a chain built from validly mined blocks stays valid and creates coins only through the coinbase; `replace_chain` adopts a candidate exactly when it is strictly longer *and* passes whole-chain validation, and leaves the node untouched otherwise; a sender can never get more than it holds past the mempool, nor replay a confirmed transaction, nor smuggle either past block acceptance; every Merkle proof verifies against its own root while an outsider's does not, and repeating the last leaf always moves the root; a signature verifies only while its payload is untouched and only for a key that owns the sender address. Each property was checked by breaking the production line it guards, the Merkle sentinel padding, the key-to-address binding, the length test in `replace_chain`, the overdraft check and the repeated-id check in `apply_block` , and confirming the matching property, and only that one, failed. The fuzz crate covers the other half: bytes the node takes from somewhere it does not control. Five targets decode untrusted input, `Transaction`, `Block`, `Message`, the length-prefixed framing in `read_message`, and the on-disk chain via `Blockchain::from_json`, with seed corpora checked in, since an unseeded run spends its whole budget failing to produce well-formed JSON. CI builds every target and runs each for 60 seconds. `Block::total_value` fell out of that immediately: a block is deserialized before anything says its amounts are affordable, so two large amounts aborted the process. It and `Blockchain::total_supply` now saturate, with a regression test next to the code. 93 tests pass (77 unit, 9 property, 7 doc); clippy clean.
The README carried a performance table nobody had run, "Apple M1", round numbers, no benchmark behind it. This adds the benchmarks and replaces the table with what they actually report. `benches/core.rs` covers the four costs that decide how a node behaves: block header hashing, ed25519 signing and verification, Merkle tree construction from 1 to 10,000 leaves, and proof-of-work. Mining is benchmarked over a pool of distinct headers rather than one block repeated. A single block has exactly one nonce search, and its length is one draw from a geometric distribution: timing it repeatedly reports that draw, not the expected work. With the pool the measured times track 16^difficulty attempts (difficulty 4 lands at ~66,000 attempts against 65,536 expected), so the hash-attempt rate extrapolates to difficulties too expensive to run. A CI job compiles the suite; timings from a shared runner would be noise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
Every preimage this chain hashed was ambiguous. The transaction hash and
the signing payload joined their fields with `|`, so the payment
("a|b" -> "c") and the payment ("a" -> "b|c") produced identical bytes:
one hash, and one signature, covering two different transfers. The block
header concatenated its fields with no separator at all, so the header
(difficulty 1, nonce 23) and the header (difficulty 12, nonce 3) hashed
identically -- one proof-of-work standing for two different claims about
how hard the block was. Merkle internal nodes concatenated their children,
unambiguous only because of what the caller happened to pass.
core::hashing::CanonicalEncoding replaces all of it: a domain tag first,
then every field as an 8-byte big-endian length followed by exactly that
many bytes. The tag keeps a preimage built in one context from ever being
valid in another. Timestamps encode as (seconds, subsecond nanoseconds)
rather than through timestamp_nanos_opt, which is None outside 1677..=2262
and would collapse every out-of-range instant onto one value -- the same
ambiguity one layer down.
This changes every hash on the chain. Pre-1.0 with no live network, so no
migration path: an existing blockchain.json will no longer validate.
Also in this pass:
- Signature verification uses verify_strict, which refuses small-order
keys and non-canonical R and A encodings. Those are the signatures two
ed25519 implementations may legitimately disagree about, and a rule two
nodes can disagree about is a chain split.
- docs/THREAT-MODEL.md enumerates the attack surface -- double-spend,
majority hashpower, eclipse and Sybil, DoS, timestamp manipulation,
replay, malleability, inflation, key storage -- and says plainly which
ones the code does not resist: length-based fork choice, an unbounded
and unauthenticated peer table, an unpriced mempool, plaintext keys.
SECURITY.md carries the disclosure policy.
- The balance map is called what it is, an account ledger, not "UTXO
simplified"; this chain has no unspent outputs.
- #![warn(missing_docs)] at the crate root, plus the docs it asked for.
- Performance table re-measured on the same machine: the canonical header
hash costs 23% (436 -> 538 ns), verify_strict 10% (21.9 -> 24.2 us),
Merkle construction is unchanged, and the transaction hash got 4%
faster.
103 tests pass (86 unit, 10 property, 7 doc); clippy clean with -D warnings.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
…odes The node could already push blocks and, when told to, pull a whole chain. What it could not do was close a fork on its own: a broadcast block that would not attach was logged and dropped, and a handshake that announced a longer chain was logged and ignored. Two nodes that mined competing forks stayed forked forever unless an operator ran `sync` by hand. Three pieces of glue fix that, all of them routed through the existing `replace_chain`, which stays the only thing that decides: - `Node::reconverge` asks every known peer for its chain and adopts the longest valid one. Gossip is push-only, and a block names a parent without carrying it, so a node forked more than one block deep can never catch up on relayed blocks alone. - A `NewBlock` at or beyond our own tip that will not attach now triggers that pull instead of being dropped: not attaching is precisely the signal that the missing history is ours to fetch. - A `Version` announcing a greater height triggers it on the listening side, and the dialling side syncs when the reply says the peer is ahead, so a node joining a running network converges on contact. `bind` is split out of `start` so a node can be given port 0 and still learn, and advertise, the port it was actually assigned, it used to keep announcing `:0`, an address no peer can dial back. tests/reconvergence.rs runs the result as a network: two or three real nodes on OS-assigned loopback ports, the real listener, the real length-prefixed framing, no mocks and no fixed ports. Three forked nodes reconverge on one tip hash at one height; a shorter chain loses from either direction; an unattachable block pulls the history behind it; a mined block relays to a node the miner has never heard of. Waits poll real chain state under a deadline rather than sleeping. Verified the tests are not vacuous by disabling the three glue points: three of the four fail. 108 tests pass, clippy clean.
Three places called the fork-choice rule "heaviest" while `replace_chain` compares `new_chain.len()` and nothing sums per-block work. The README said both things about itself two sections apart: the reconvergence tests "the heaviest chain wins", the security section "length-based fork choice" as one of the four open gaps. Since the retarget landed, length and work are no longer the same ordering, so the loose word is a false claim rather than a synonym. Renamed to "longest" in the reconvergence tests, the `Node::reconverge` doc comment and the README. `docs/THREAT-MODEL.md` §2 keeps "heaviest" where it names the fix that is *not* implemented. Also: the README clone URL still pointed at `yourusername`, and the changelog was missing its last two tranches, difficulty retargeting and peer-driven reconvergence, both of which change behaviour a reader needs told. 108 tests pass, fmt/clippy/release build clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
Close the consensus holes found in review: - Fork choice is now heaviest-work, not longest-chain. replace_chain validates the candidate in full, then adopts it only if total_work (the saturating sum of 16^difficulty over its blocks) strictly exceeds our own. With the difficulty retarget in play, length and work come apart, so a long run of cheap blocks (a time-warp reorg) could beat the honest chain on block count while costing far less work; it is now rejected. A regression test builds exactly that chain, longer and internally valid but lighter, and confirms it loses. - Block timestamps gain a lower bound: a block whose timestamp is at or below the median of the previous up to 11 blocks (median-time-past) is refused, so one miner can no longer backdate blocks to steer the retarget, which reads nothing but timestamps. Honest mining nudges a new block past its parent so clock resolution alone never fails the rule. - The genesis/base difficulty is no longer a trusted deserialized field. It is serde-skipped and anchored to DEFAULT_DIFFICULTY on load, so a crafted chain file can no longer declare its first retarget window mined at difficulty 1. A node running a lower-difficulty local network still sets it in memory through with_difficulty. - replace_chain reads the genesis through first() instead of indexing, so a chainless Blockchain reports EmptyChain rather than panicking. Also corrects the retarget doc comment: one step is a 16x change per window, a coarser control than Bitcoin's 4x limit, not a stricter one. The multi-node reconvergence test is renamed to say heaviest, and the property suite gains a middle-splice corruption in place of the vacuous truncate arm. Persistence coverage at the network difficulty moves to a focused test since the fast property mines below it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
Network and quality hardening: - Fork choice at the network layer routes through replace_chain, which now decides on work. reconverge and sync_with_peer drop their length pre-checks and let replace_chain accept only a strictly heavier chain, so a longer-but-lighter offer is refused from either direction. - connect_to_peer wraps both peer reads in REQUEST_TIMEOUT, mapping a timeout to a wire error, so a peer that accepts the connection and goes silent cannot pin the task. - Peer addresses are validated as socket addresses and the table is capped at MAX_PEERS, and addresses a peer merely advertises through GetPeers are no longer auto-dialled: discovery is limited to explicitly configured peers and the peers we directly handshake with, which contains the SSRF/reflection surface. - Wallet implements Debug by hand and renders the private key as <redacted>, so a log line or panic message can never print it; Serialize (explicit export) is unchanged. - The empty Merkle tree gets its own MERKLE_EMPTY_DOMAIN tag instead of reusing the leaf domain, so the empty root can equal no leaf. - clap derives author and version from Cargo.toml instead of a placeholder author and a hardcoded version string. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
- README and CHANGELOG now describe heaviest-work fork choice instead of the longest-chain rule, and correct the retarget wording: one 16x step per window is a coarser control than Bitcoin's 4x limit, not a stricter one. - THREAT-MODEL: the reorganisation section (§2) records the time-warp reorg as closed by the heaviest-work rule, median-time-past and the anchored genesis difficulty, with the ordinary more-work reorg left as the honest-majority assumption; §3, §4 (peer bound and SSRF containment, with the residual eclipse risk stated), §6 (median-time- past enforced), §8 (empty Merkle domain), §10 (Debug redaction) and the summary table are updated to match. - SECURITY: the supported branch is `master`, not `main`. - CHANGELOG: the field-injection note counts the preimages correctly (seven, now that the empty-tree root has its own domain). - README fuzz recipe includes the `mkdir -p fuzz/corpus/<target>` its own fuzz README requires. - CI gains an MSRV job pinned to 1.90 running `cargo check --all-targets --locked`, so the declared minimum is exercised rather than only badged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0148m7vK62CprRtui8x6F6Ak
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
1 participant
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.
Promotes the verified hardening branch to master: heaviest-work fork choice with median-time-past (closes the time-warp reorg), panic-free library with a clippy deny gate, PoW difficulty retargeting, canonical length-prefixed hashing, criterion benches, cargo-fuzz + property tests, a multi-node TCP reconvergence test, CI, and a threat model. Full gate green: fmt, clippy -D warnings, 115 tests, release build.