Skip to content

Ditch achieved work, reduce MaxReorgDepth#599

Merged
illuzen merged 3 commits into
mainfrom
illuzen/reorgs
Jun 19, 2026
Merged

Ditch achieved work, reduce MaxReorgDepth#599
illuzen merged 3 commits into
mainfrom
illuzen/reorgs

Conversation

@illuzen

@illuzen illuzen commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

This achieved work thing was causing these lucky (or malicious?) blocks to do deep temporary reorgs:

Screenshot 2026-06-18 at 12 55 42 PM

Neither Bitcoin nor Ethereum had this achieved difficulty thing. We just use target difficulty now.


Note

High Risk
Changes consensus fork-choice math, aux key namespace, and max reorg depth—operators must coordinate upgrades and may see different best-chain behavior until aux work is rebuilt.

Overview
Fork choice no longer credits blocks with work from U512::MAX / nonce_hash, which let unusually low hashes swing cumulative totals and drive deep temporary reorgs. Import now calls verify_and_get_block_work and adds each block’s network target difficulty to the parent’s cumulative work (invalid seals get zero work).

Client aux storage moves from QPow:AchievedWork: to QPow:ChainWork: with renamed helpers (store_cumulative_work, initialize_genesis_work, etc.). Old values are not comparable; upgrading nodes should resync or purge legacy aux work entries.

MaxReorgDepth in runtime config drops 180 → 100, tightening how far behind the tip finalization runs. Runtime API, pallet, tests, and RUNTIME_SURFACE.md are updated for the new API name and semantics.

Reviewed by Cursor Bugbot for commit deb046c. Configure here.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but still having AI review it...

@n13

n13 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Review — Ditch achieved work, reduce MaxReorgDepth

Verdict: Approve in principle — the consensus fix is correct — but please address the in-place upgrade hazard before merge (and a couple of nits).

The core change is right

Crediting each block with the target difficulty it had to beat (get_difficulty() at the parent's state) instead of the achieved difficulty (U512::MAX / nonce_hash) is the correct fix. Since difficulty here is defined as U512::MAX / target, summing per-block difficulty is exactly Bitcoin's cumulative-work metric. Every block at a given difficulty now contributes identical, deterministic work, so a single lucky low hash can no longer inflate cumulative work and drive deep temporary reorgs. It's also self-consistent: the credited work is the same get_difficulty() that is_valid_nonce uses to validate the seal, so a block is always credited exactly the target it satisfied.

Main concern — nodes that upgrade without resyncing

The aux namespace moves QPow:AchievedWork: -> QPow:ChainWork: and nothing backfills it. On an in-place upgrade (no purge/resync), every pre-upgrade block reads back as get_chain_work() == 0 under the new prefix, while initialize_genesis_work only seeds genesis.

That breaks fork choice in the unfinalized window until new work accumulates. A single freshly-imported block on an old parent gets new_work = difficulty > 0, and is_heavier compares it against the current best whose work also reads 0:

candidate_work > current_work ||
    (candidate_work == current_work && candidate_number > current_number)

So a competing block from, say, 50 blocks back (parent work 0) beats the current tip (work 0, higher number) purely on difficulty > 0 — a backward reorg. Exposure is bounded by finalization (now best-99) and a full resync avoids it, which the description recommends. But per this repo's "fail early, never fail silently" rule I'd add an explicit guard rather than rely on operators reading release notes: on startup, if best height > 0 but its ChainWork is zero, log an error and refuse to proceed (or force resync). That turns a silent consensus hazard into a loud, actionable failure.

MaxReorgDepth 180 -> 100

Reasonable given the fork-choice fix removes the deep-reorg driver, and it slightly shrinks the transition window above. Worth confirming this is an intentional finalization-margin change that every node must run identically — it's a compiled runtime constant, and divergent values across nodes can finalize conflicting blocks.

Minor / nits

  • node/src/service.rs (~609-612): the call was renamed to initialize_genesis_work, but the surrounding comment and the warn! log still say "achieved work." Stale wording.
  • qpow_math::achieved_difficulty_from_hash is now unused by production code (only its own unit/proptests). It's pub so no dead-code warning, but consider removing it (plus the now-orphaned test_calculate_achieved_difficulty in pallets/qpow/src/tests.rs) to keep things minimal.
  • The runtime API method was renamed in place with no #[api_version] bump on QPoWApi. Fine as long as native + Wasm runtime always ship together (they do here), but worth noting since a client/runtime version skew would fail method dispatch.

Nice cleanup overall — switching to target difficulty is the standard, correct metric.

@n13

n13 commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

I don't think the reorg description is right but what is right is that we should make sure we don't throw the chain into chaos when we upgrade the runtime.. we could build off of previous achieved number, or else ... accept the chaos we're in testnet anyway

@illuzen
illuzen merged commit 597ad08 into main Jun 19, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants