Skip to content

Hive broadcast reliability - #22

Merged
mcfarhat merged 27 commits into
developfrom
hive-broadcast-reliability
Aug 24, 2026
Merged

mcfarhat merged 27 commits into
developfrom
hive-broadcast-reliability

Conversation

@perlaimad

Copy link
Copy Markdown
Collaborator

This PR strengthens the existing Hive integration layer without changing HiveLore’s non-custodial signing boundary.
It adds a centralized Hive broadcast reliability layer with:
Bounded exponential backoff with jitter
Transient vs permanent Hive/RPC error classification
Node failover with in-memory health and cooldown
Ambiguous broadcast recovery by confirming the same transaction before retrying
Mainnet/testnet network configuration with validated chain IDs and node lists
Structured confirmed broadcast results
Safer canon transaction confirmation using backend read-back instead of trusting client block metadata
Tests for retry, fail-fast errors, node rotation, ambiguous outcomes, operation mismatch, and env validation
Documentation for supported networks, confirmation semantics, smoke-test procedure, and operational recovery
Verification completed:
API tests: PASS
Typecheck: PASS
Lint: PASS
Build: PASS
Prisma validate: PASS
Known blocker: real testnet broadcast/read-back was not executed because signer/account and testnet confirmation prerequisites were not available. Repo-wide format:check also still fails on unrelated pre-existing files.

perlaimad and others added 7 commits August 10, 2026 10:29
…anagement

- Added auth-rate-limit-store for managing rate limits on API requests.
- Introduced canon-voting-policy for defining voting rules and tallying votes.
- Developed canon-voting module for handling proposal voting logic, including casting votes and finalizing decisions.
- Created API routes for managing proposals, including fetching details, voting, and confirming transactions.
- Implemented client-side API functions for interacting with the new proposal and voting endpoints.
…arity

+ feat: enhance canon voting logic and add tests for voting policy
- Introduced endpoints for listing and creating comments on proposals.
- Added ProposalComment model to the database schema with necessary fields and constraints.
- Implemented logic for handling comment creation, including validation and authorization checks.
- Updated frontend to support displaying and posting comments on proposals.
- Created tests to ensure proper functionality and error handling for comment-related operations.
- Added new environment variables for Hive mainnet and testnet configurations in env.ts.
- Introduced HiveReliableBroadcaster for improved transaction broadcasting with retry logic.
- Implemented network configuration utilities to build and validate Hive network settings.
- Updated canon voting logic to support reliable broadcasting and confirmation of transactions.
- Added tests for broadcasting reliability, including transient and permanent error handling.
- Refactored existing Hive client and constants to accommodate new configurations and broadcasting features.
@github-actions

github-actions Bot commented Aug 10, 2026 •

Copy link
Copy Markdown

Vercel preview is ready:

https://hivelore-qh3v3d9do-kareemn1936s-projects.vercel.app

Commit: 1f2fe43

@mcfarhat

Copy link
Copy Markdown
Contributor

@perlaimad reviewed the delta only — this branch is exactly one commit ahead of m2-worlds-seed-bible-hub, so I scoped to 06fe5d1d (~14 files) rather than re-reviewing #14.

The design here is thoughtful. Bounded retries with a total deadline, correct symmetric jitter, fail-fast on permanent errors without rotating nodes, a node pool that degrades to the cursor node instead of wedging when everything is cooling down, and read-back verification that checks operation content rather than trusting client-supplied block metadata. 8/8 new tests pass locally.

The non-custodial claim in your PR body — I verified it and it holds. broadcastSignedTransaction accepts only a pre-signed transaction; nothing in the delta accepts, derives, stores or logs key material, and there's no logging in the new module at all. A repo-wide grep for key patterns turns up only docs and a dhive test fixture. Good.

That said, the one path actually wired into the application cannot confirm a real transaction today.

Blockers

B1 — operationsEqual uses JSON.stringify, so every real confirmation is permanently rejected

broadcast-reliability.ts:541:

function operationsEqual(left: HiveLoreOperation, right: HiveLoreOperation) {
  return JSON.stringify(left) === JSON.stringify(right);
}

That's key-order sensitive. Your locally-built op emits {id, json, required_auths, required_posting_auths}; hived/HAF returns custom_json_operation in FC-reflection order {required_auths, required_posting_auths, id, json}. I ran the comparison on two semantically identical operations differing only in key order — false.

Line 503 then throws BROADCAST_REJECTED classed permanent, so it isn't "keep looking", it's a hard reject. The real-world sequence: the author signs the decision via Keychain, the op lands on-chain correctly, the app POSTs to confirm, and the API permanently rejects its own valid transaction. The decision can never be anchored.

The repo already has hashCanonicalJson (used at canon-voting.ts:788) — use that, or compare field-wise. Your test at :182-205 hides this because the match case embeds the same in-memory object that's passed as expectedOperation, so key order is trivially identical. Please add a fixture with hived's real field ordering.

B2 — Confirmation aborts on the first unrelated operation in the scanned range

:479-480 calls normalizeHafOperation(row) on every row before filtering by transactionId, and projection.ts:122 throws a bare Error for anything that isn't a comment/custom_json. The transport never passes operationTypes, so /block-search returns all types. A single vote_operation ahead of your target row kills the lookup — and every real block range has plenty. It escapes as an unclassified Error, not a HiveBroadcastError.

Compounding it: :458 widens the scan to head - 1200 .. head (~1 hour of blocks) with no pagination. HafClient.searchBlocks accepts page/pageSize but the transport never sends them, so only the first ~100 rows of 1200 blocks are ever examined. Even with B2 fixed, the target op would usually fall outside page 1.

This is the same root cause as B2 on #14 — worth fixing once in projection.ts and having both paths use it.

The bigger question: this code has no caller

broadcastSignedTransaction is referenced nowhere outside its own module and tests — I checked across apps/api/src and apps/web/src. Only confirmTransactionOperation is wired in (via proposals.ts:102). Broadcasting is client-side through Keychain by design, which is correct for a non-custodial app.

So the retry/backoff/failover/ambiguity engine — the headline of this PR — is currently unreachable infrastructure. That may well be the right call as groundwork, but it should be a stated decision rather than an accident. Either add a comment at the top of the module saying it's prepared for server-side broadcasting that doesn't exist yet, or wire it. Right now a reader can't tell which.

Should-fix

  • Delete the testnet configuration surface. network-config.ts:71 reads isTestnet ? input.testnetHafUrl : input.mainnetHafUrl || DEFAULT_HAF_API_URL — || binds tighter than ?:, so on testnet hafUrl is undefined and HafClient falls back to its mainnet default. Your README says "HiveLore never silently falls back between mainnet and testnet"; it does. But rather than fix it: we don't use the Hive testnet on any project, we run mainnet always. Please drop the testnet branch entirely — it's a config path we'll never exercise, and untested config paths are where this class of bug lives. Chain-ID validation is also format-only, so HIVE_NETWORK=testnet with a mainnet chain id passes startup; that concern goes away with the branch.
  • No HTTP timeout inside an attempt. haf-client.ts:74 calls fetch(url) with no signal and no timeout, and the transport drops the signal parameter. The total deadline is only checked between attempts, so one hung HAF socket hangs the confirm request indefinitely. Also totalDeadlineMs (30s) is shorter than confirmationTimeoutMs (60s), so with defaults the first poll alone exhausts the deadline and maxAttempts: 4 never happens — validateHiveRetryConfig doesn't check that relationship.
  • Error classification is free-text substring matching never validated against real hived strings. Unrecognized errors fall through to unknown and get retried, so genuinely permanent rejections (hived's Assert Exception: ...) retry pointlessly. Your README claims insufficient-RC fails fast, but the matcher looks for insufficient rc / resource credits while hived's rejection reads roughly Account: X has N RC, needs M RC — I don't believe that matches. lower.includes('parse') → permanent will misclassify a malformed response from a flaky node as a permanent transaction rejection. And already known / duplicate transaction classed transient is backwards: a duplicate means it was accepted, so it should short-circuit straight to confirmation.
  • A slow HAF marks a healthy RPC node unhealthy. reportTransientFailure at :233 fires for errors originating in confirmation too, since confirmAfterBroadcast sits inside the same try. With maxConsecutiveNodeFailures: 1, one HAF hiccup cools a perfectly good RPC node for 30s.
  • If confirmation itself throws inside the catch at :237, the exception escapes broadcastSignedTransaction unwrapped, so callers get a raw Error instead of a HiveBroadcastError.
  • Node health is per-process (client.ts:66 builds a fresh pool per route registration), so with multiple API instances each rediscovers bad nodes independently and health is lost on restart. Fine for now — worth a comment saying so.
  • The confirm endpoint isn't rate-limited (proposals.ts:328). Each call can hold a request open 60s and fire ~20 unpaginated 1200-block scans at public HAF.

On the ambiguous-broadcast case

Your logic is sound, but the safety actually comes from somewhere other than where the PR body claims. On retry you re-broadcast the exact same input.transaction (:214) — never re-signed, so the same trx_id, so hived's own duplicate-transaction check dedupes it inside the expiration window. That is what prevents a double broadcast. The confirm-before-retry is a best-effort optimisation on top. Worth restating in the PR body, because the current wording would lead a future reader to think removing the confirm step is unsafe when the real invariant is "never re-sign on retry" — which is the thing that must not be broken.

This matters more than it would elsewhere: we're on mainnet, so a duplicate custom_json is a permanent public artifact, not something a testnet reset clears.

The dangerous case is also the untested one. test:148-180 covers broadcast-ambiguous-then-confirmation-succeeds. Nothing covers broadcast-ambiguous-then-not-yet-visible-then-re-broadcast, which is the path that actually protects you.

Nits

base + jitter can exceed maxDelayMs by the jitter ratio at the cap. Dead config fields never read anywhere: addressPrefix, customJsonId, transactionExpirationMinutes, explorerTransactionUrl, plus the NETWORK_MISMATCH status code. :262-265 maps TRANSIENT_RPC_ERROR → BROADCAST_STATUS_UNKNOWN and everything else → BROADCAST_CONFIRMATION_TIMEOUT, which reads backwards. confirmationSource is hardcoded 'haf' despite the type allowing three values. A fromBlock hint of 0 is ignored by the falsy check.

Definition of done

Fix B1 and B2, then do one real mainnet round-trip through this path and post the transaction id. We don't use the testnet — see the card for how the account is provided. A single real confirmation would have surfaced both blockers immediately, which is exactly the argument for doing it before the review rather than after.

Please also rebase this onto m2-worlds-seed-bible-hub rather than develop, so the diff shows your 14 new files instead of 41.

@mcfarhat

Copy link
Copy Markdown
Contributor

@perlaimad addendum — my earlier review missed a commit, which is my mistake. I scoped it to 06fe5d1d and said so explicitly, but you'd already pushed 5341e03c at 09:23 and I posted at 09:36. So the error-classification work went unreviewed. Covering it now.

5341e03c — one of my findings genuinely fixed

You gave confirmation its own HiveNodePool (confirmationPool) and wrapped the poll in a try/catch that classifies the error, reports success or transient failure against the node actually used, and re-throws only when the class is permanent.

That directly fixes the point I raised about a slow HAF marking a perfectly healthy RPC node unhealthy — confirmation failures now count against the confirmation pool instead. Good fix, and you got there before I'd finished writing it up.

But the two blockers are unchanged, and one of them changed shape

B1 is untouched. broadcast-reliability.ts:565 is still:

function operationsEqual(left, right) {
  return JSON.stringify(left) === JSON.stringify(right);
}

Still key-order sensitive, so a valid on-chain operation returned by hived in FC-reflection order still fails the comparison and gets a permanent BROADCAST_REJECTED. Use hashCanonicalJson.

B2 now fails differently — arguably worse, because it's silent. normalizeHafOperation(row) is still called at :504 on every row before filtering by transactionId, and it still throws a bare Error for any operation that isn't a comment or custom_json. What's changed is what happens next: the new catch classifies that bare error as BROADCAST_STATUS_UNKNOWN (not permanent), reports a transient failure against the node, and the loop continues.

So instead of surfacing a 500 with an internal message, you now get: every poll hits the same unrelated vote_operation in the range, throws, is classified unknown, blames the node, rotates, hits the same rows on the next node, and repeats until confirmationTimeoutMs. The transaction never confirms, no error explains why, and healthy nodes get marked unhealthy for something that isn't their fault.

The fix is the same as before and it's upstream of the classification: filter by transaction_id before normalizing, and pass operationTypes to the search so unsupported rows never come back.

The scope question — and this part is on us, not you

You built what card #33 asked for. It specifies retry with exponential backoff, confirming a broadcast landed, node failover across a configurable node list, and a mainnet/testnet switch. You delivered against that spec, with tests.

The problem is that the card was wrong. HiveLore signs client-side through Keychain and the server holds no key at all — which is why broadcastSignedTransaction has no caller. We asked for a server-side broadcaster in an app that doesn't broadcast from the server. That's our specification error, and I want it recorded that way rather than as anything you got wrong.

So, proposed split — nothing gets thrown away:

Keep in this PR — the confirmation/read-back path. confirmTransactionOperation is genuinely wired through proposals.ts, it's the strongest code here, and it's needed regardless of who broadcasts. Fix B1 and B2 and it merges with #14.

Park, don't delete — broadcast-reliability.ts, the node pool and the retry engine. I'll move it to its own Backlog card as prepared infrastructure, with a note that it's pending a decision on server-side broadcasting. If we later add periodic state-root anchoring (an app account signing a daily hash of canon state), this is exactly what it will need, and it'll be waiting.

Also: please retarget this PR's base from develop to m2-worlds-seed-bible-hub. It currently contains all of #14, so it reads as 41 files when only 14 are yours.

The order hasn't changed: #14 lands first, then this rebased on top.

@mcfarhat mcfarhat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at f0a5a0c8, covering all 17 commits from db50deb. Two independent passes, both executing against real Postgres 16, real Prisma, real migrations — plus my own tiebreak runs where they disagreed. I scoped this to the whole PR rather than a commit range, because I got that wrong twice on this branch already.

First, two decisions from Mohammad that were blocking you, and an apology: you've been waiting since this morning on my "I'll come back on the rest", and the Backlog card I promised to park the broadcaster on was never created. That's on me.


Decisions

1. Canon quorum → count NEEDS_REVISION and ALTERNATE_TIMELINE in the approval denominator.

I ran your policy code and found the two halves of the rule count different populations — totalVotes includes all four choices, approvalDenominator only approve+reject:

1 approve, 4 needs_revision       total=5 denom=1 bps=10000 -> APPROVED_FOR_PUBLICATION
1 approve, 4 alternate_timeline   total=5 denom=1 bps=10000 -> APPROVED_FOR_PUBLICATION
1 approve, 1 reject, 3 needs_rev  total=5 denom=2 bps= 5000 -> NEEDS_REVISION

So four people asking for revision satisfy the quorum and then vanish from the percentage. Worse at scale: 5 approve + 20 needs-revision also canonizes. And note line 3 — one REJECT would have blocked it, so a voter is penalised for choosing the more constructive option.

We considered excluding them from the quorum instead, but that only raises the floor without making dissent count. The ruling is: put them in the denominator. In tallyCanonVotes, approvalDenominator becomes all four choices rather than approve+reject. Approval is then Approve ÷ (all votes cast). Please update the README line that documents the current behaviour too — it currently states this as intended, so it needs to change with the code.

To be clear: this is a spec correction, not a mistake on your part. You implemented what was written down.

2. Strip testnet support completely — we are mainnet-only, always.

This PR introduces a testnet switch that didn't exist before: the HIVE_NETWORK enum and HIVE_TESTNET_* vars (env.ts:41,48-53), HIVE_PUBLIC_TESTNET_CHAIN_ID and the testnet RPC constant (constants.ts:3-4,7), the whole testnet branch with its TST prefix and 60-minute expiration (network-config.ts:9,61-85), the testnet vars in both .env.example files, and the README's "Testnet smoke test" procedure. All of it should go.

One related thing to keep after the strip: wax-client.ts:59 used to hardcode the mainnet chain id and now reads options.chainId ?? HIVE_MAINNET_CHAIN_ID, while validateChainId accepts any 64-char hex. Please assert positively that the mainnet chain id starts beeab0de — otherwise a bad env var silently builds transactions for another chain while every log still says network: 'mainnet'.

Also: the DoD on this card is one real mainnet broadcast plus read-back, with the tx id posted. The PR body cites a blocked testnet broadcast, which is the wrong target. Use your own Hive account with posting authority; ping me if you need RC delegation.


What holds up — verified by execution

Your CI fix is real, and I proved the gate actually gates. Broke an assertion, a TypeScript compile, and threw an unhandled rejection — all three exit 1. run-tests.mjs propagates status correctly and throws if zero test files are discovered, which closes the old shell-glob hole. 156/156 tests pass, and develop genuinely had no test script at all, so your claim was accurate.

Your new tests have real assertive power. Mutation testing caught 4/4: disabling isAmbiguousBroadcastError, deleting the expectedOperation mismatch check, no-op'ing reportTransientFailure, removing the expectedSigner check. These are not tests written to satisfy a reviewer.

Key handling is clean and I went looking hard. Signing is entirely browser-side via Keychain requestCustomJson(..., 'Posting', ...). No key in env, none server-side, zero logging calls anywhere in lib/hive/ or canon-voting.ts, no error object carries one. Posting-only is enforced positively — canon-voting.ts:1021-1031 rejects any confirmed op with required_auths.length > 0. parseNodeList rejects credential-bearing URLs and sanitizeHiveNodeUrl strips userinfo. Nothing needs active or owner.

The migration repair works, and I tested the case that actually worries me — an existing database rather than a fresh one:

develop migrations applied (fails at 20260718154842_init, leaves a failed row)
  -> PR deploy                                    : P3009, exit 1, nothing applied
  -> `migrate resolve --applied` first, PR deploy : all 12 applied, exit 0
     migrate status: up to date     migrate diff: No difference detected

Since resolve --applied is the only way past that baseline today, every working environment has already run it and upgrades cleanly. You also fixed the drift with align_indexer_watermark_index, and you're the only PR that repairs P3018 at all — and the only one whose test suite doesn't crash on AUTH_JWT_SECRET.

Canon governance survived everything else thrown at it against real Postgres: self-voting blocked, double-voting yields one row even under a concurrent race, vote-change works, voting after the 48h window blocked, cross-world access blocked, 4 votes below minimum rejected, exactly 7000bps approves and 6923 doesn't, finalize-before-window blocked, concurrent finalize yields exactly one decision.

Also worth saying: cards #36 and #37 were marked "unassigned, repo-level" and you did both without being asked. That fixed something that had been quietly costing everyone on this project time.


Blockers

1. /canon-transaction/confirm has no authorization at any layer

routes/proposals.ts:389 uses only requireSession. The four sibling mutating routes (:190, :236, :277, :316) all call authorizeWorldPermission. And confirmCanonTransaction never receives a user id at all, so there's no check at the service layer either.

Any authenticated user of the app, with no membership in that world, can confirm anyone's canon decision — and each call spawns up to 60s of paginated outbound block-search. This is new surface: this PR is what first registers these routes in app.ts. Needs authorizeWorldPermission plus a rate limit (the route currently opts out; app.ts registers rate-limiting with global: false).

createCanonTransactionOperation at :355 is also missing it, though the author check inside limits the damage there.

2. Two concurrent confirms write two permanent mainnet records for one decision

The DECISION_ALREADY_CONFIRMED guard (canon-voting.ts:960-976) sits outside the transaction, which opens at :1051, and the final write at :1075 is an unconditional update by id. @@unique([transactionId, operationIndex]) doesn't help — both writes hit the same row. Reproduced with two concurrent calls carrying different tx ids: both returned OK, two HiveEvent rows persisted, the decision points at whichever landed last, and the other on-chain custom_json is orphaned with nothing recording it as a duplicate.

Fix: updateMany({ where: { id, transactionId: null } }) and assert one row affected, or move the read inside the transaction under a row lock.

3. The app invites the duplicate broadcast in the first place

createCanonTransactionOperation (:801-823) checks decision-exists, outcome-approved and signer-is-author — but never checks decision.transactionId. After a decision is fully confirmed on chain, it happily re-issues the identical signable operation, the author signs, and a second permanent custom_json exists. The confirm then 409s — after the artifact is already on mainnet. Your own README says the opposite: "must be reconciled by retrying confirmation for the same transaction ID, not by creating a different transaction."

4. The bounded-retry layer isn't bounded

totalDeadlineMs is checked only at the top of the retry loop (broadcast-reliability.ts:219-221) and never passed down, and isAmbiguousBroadcastError treats every non-permanent failure as ambiguous — so a plain ECONNRESET triggers a full 60s confirmation poll before the first retry. Measured on the shipped defaults with every node timing out:

broadcast attempts = 1 (maxAttempts = 4)
elapsed            = 74,000 ms (declared deadline 30,000 ms)

Both headline claims fail at once: 2.5× the deadline, and one attempt of four. HafClient.getJson also calls fetch with no timeout and no signal, and the confirm route passes no AbortSignal, so a client disconnect doesn't cancel a 60s poll. validateHiveRetryConfig checks totalDeadlineMs >= requestTimeoutMs but never against confirmationTimeoutMs — and the defaults contradict each other (30s vs 60s).

5. Seven route handlers have dead catch blocks

routes/proposals.ts lines 161, 179, 265, 305, 344, 378, 413 all do return someAsyncFn(...) inside try. In an async function the try exits at return, so the rejection never reaches the local catch. Status codes survive by luck (Fastify honours error.statusCode), but the body shape changes and the web client reads the field that breaks:

bare return : {"statusCode":409,"code":"...","error":"Conflict","message":"Already linked..."}
awaited     : {"code":"...","error":"Already linked..."}

So users see "Conflict" instead of the real message on every canon-voting error. Fix is return await. worlds.ts and contributions.ts already do this correctly.

6. Migration needs a runbook line and a CI job for the upgrade path

Your fresh-DB check is well built, but it only ever creates a virgin database — so the P3009 case above is invisible to CI. Please add the migrate resolve --rolled-back (or --applied) recovery step to the docs, and a CI job that applies develop's migration set and then this PR's.


Not blocking, worth knowing

  • Read-back verification can't detect a lying node. It confirms the returned row echoes content we already supplied — no signature check, no cross-node quorum, no head-block sanity check. I got a fabricated row accepted, a negative block number accepted, and a garbage timestamp crash out as an unclassified RangeError. Trust rests on a single hafUrl, and while parseNodeList requires https in production, HIVE_MAINNET_HAF_URL is only z.string().url() — plain http:// passes. That's the one path described as the trust anchor.
  • Confirmation-side failover is inert: DefaultHiveBroadcastTransport.searchBlocks/getHeadBlock drop nodeUrl and always build a client from the single hafUrl, so confirmationPool's rotation and health tracking never engage.
  • The node pool is sticky-first, not round-robin, and semantic failures rethrow without reportTransientFailure — so a node answering fast with wrong data takes 100% of traffic and can't be evicted.
  • The error classifier never reads error.cause, where undici puts the real network error — so fetch failed, ETIMEDOUT and socket hang up all classify as unknown. Consequence: the commonest failure returns "couldn't confirm" rather than "may have landed", which on a permanence decision nudges an operator toward broadcasting a new transaction. Also lower.includes('expired') is tested before any transient check, so an expired TLS certificate fails the whole broadcast as permanent and kills failover.
  • broadcastSignedTransaction never asserts signatures.length > 0 — cheap guard, given the non-custodial claim rests on it.
  • Root test is hardcoded to the api workspace rather than --workspaces --if-present, so when a web suite lands it silently won't run — re-creating card #37 exactly.
  • 20260805170000 is used by both add_auth_rate_limit_buckets and add_contribution_drafts. Name-tiebreak makes the order deterministic today, but it's a latent hazard.
  • broadcast-reliability.test.ts:404-423 is named "rotates unhealthy nodes and recovers after cooldown" but still passes when cooldown never expires — a vacuous assertion.

Sequencing

PR #14 is now closed as superseded — this PR contains it in full, so cards #23, #26, #28 and #30 all track here. I ran every merge order empirically: #22 → #21 → #20 is optimal, and this PR lands first because it's the only one that fixes the migration baseline and the test harness. #22 + #21 together came out fully green — 172/172 tests, clean build, clean deploy, zero drift.

The scope here is large (51 files) and I'm not asking you to split it now — with #14 closed, this is the single PR for that work, and splitting at this point would cost more than it saves. But it's why three of the blockers above sat unnoticed: they're in code nobody can read in one pass.

Six blockers, and most are a few lines each. Ping me when they're up and I'll turn the re-review around quickly.

@KareemN1936

Copy link
Copy Markdown
Collaborator

Pushed the review fixes for this PR.

Commits:

  • 5f18c17 fix(api): close canon confirmation review gaps
  • c64c9ea ci(api): check prisma upgrade migrations

Covered:

  • Added world authorization and route-level rate limiting for canon transaction confirmation.
  • Changed canon approval math so NEEDS_REVISION and ALTERNATE_TIMELINE count in the approval denominator, with README/tests updated.
  • Prevented creating new signable canon operations once a decision is already linked on-chain.
  • Made confirmation writes conditional/idempotent so concurrent confirms cannot race two different permanent tx records onto one decision.
  • Threaded HAF request timeouts/signals, scoped confirmation search to custom_json, paginated read-back, and bounded confirmation by the remaining broadcast deadline.
  • Converted proposal route dead catches to return await so canonical error bodies are preserved.
  • Removed testnet config/docs and positively validate Hive mainnet chain id.
  • Added the migration upgrade CI check from develop plus the documented recovery command.

Verified locally:

  • npm run lint --workspace=@hivelore/api
  • npm run typecheck --workspace=@hivelore/api
  • npm test --workspace=@hivelore/api (157/157)
  • npm run build --workspace=@hivelore/api

One manual DoD remains outside code: I did not run a real mainnet smoke broadcast/confirmation because that needs explicit account + exact-operation + permanence approval.

@mcfarhat mcfarhat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at c64c9ea1. Two independent passes plus my own checks on the two most serious items.

Seven of eight closed, and closed well

The quorum ruling landed exactly right. Ran the real policy code over every boundary case:

1 approve, 4 needs_revision      den 5   2000bps -> NEEDS_REVISION     (was: canonized)
1 approve, 4 alternate_timeline  den 5   2000bps -> ALTERNATE_TIMELINE (was: canonized)
5 approve, 20 needs_revision     den 25  2000bps -> NEEDS_REVISION     (was: canonized)
4 approve, 1 needs_revision      den 5   8000bps -> APPROVED           (correctly still passes)
7 approve, 3 reject              den 10  7000bps -> APPROVED           (boundary intact)
6 approve, 4 needs_revision      den 10  6000bps -> NEEDS_REVISION

Seven for seven, README updated, and your regression test is named "constructive dissent prevents approval instead of disappearing from the denominator" — which tells me you understood the reasoning, not just the diff.

And critically, the change can't rewrite history. I specifically went looking for this, because a governance rule change that retroactively alters already-anchored results would be worse than the bug: the full tally (num/den/bps/threshold/rulesVersion) is snapshotted into ProposalDecision at finalize, the confirm path re-derives nothing and reuses the stored decisionPayloadHash, and no migration touches decisions. Clean.

The concurrency fix is the right pattern — a conditional updateMany claim inside the transaction. Raced: same tx → one success, one idempotent success, exactly one HiveEvent; different tx → clean 409, one link. count: 0 handled properly rather than silently succeeding.

The new timeout does not leave half-written state — the other thing I was worried about, since killing a confirmation mid-flight could lose the record of a transaction that actually landed on mainnet. After a timeout: transactionId null, operationIndex null, zero HiveEvent rows, and a later retry succeeds. The polling happens entirely before the transaction opens. Deadline now holds to within half a second of its declared budget, down from 74s against a declared 30s.

Also confirmed: testnet gone repo-wide (grep returns only the surviving HiveNetworkName = 'mainnet'), chain id now positively asserted, authorization enforced on both routes (non-member → 403, anonymous → 401), re-issue after confirmation → 409, and all seven handlers converted — that return await fix is what actually routes CanonVotingError into your handler instead of Fastify's default, so the status codes genuinely improved. Tests 89 → 157 with no files deleted.

That's a strong round. Three things go back.


1. HIGH — the confirmation path cannot work against real mainnet HAF

I called the live endpoint exactly as HafClient.searchBlocks does:

GET https://api.hive.blog/hafbe-api/block-search?operation-types=18&from-block=…&to-block=…

returned keys : total_blocks, total_pages, block_range, blocks_result
parser accepts: operations_result | operations | results | rows | data   -> NONE match

So parseBlockSearchPage (haf-client.ts:129-133) throws "HAF block-search response did not include operation rows." And it's not just a key name — blocks_result[].operations is [{op_type_id: 18, op_count: 12}], i.e. per-block operation counts, not the operations themselves. The endpoint returns a block summary; the code expects operation rows.

That error classifies as unknown, which pollForConfirmedOperation swallows (only permanent rethrows), so every real confirmation retries every 3s for the full timeout and then 500s.

Your operationTypes: [18] filter is correct — I verified op_type_id 18 is custom_json on live HAF — but it's moot until the response shape is handled. You'll want a different endpoint (something returning actual operations, e.g. an account/operations or transaction lookup) or a second call to fetch the operations for the blocks this returns.

This is also why the card can't be Done yet: the DoD is one real mainnet broadcast plus read-back with the tx id posted, and this path demonstrably hasn't been exercised against mainnet.

2. HIGH — the app will not boot with the .env.example this PR ships

The same change that added totalDeadlineMs >= confirmationTimeoutMs (network-config.ts:137) left the examples stale:

.env.example:45 / apps/api/.env.example:44 / README.md:586
  HIVE_BROADCAST_TOTAL_DEADLINE_MS=30000
  HIVE_CONFIRMATION_TIMEOUT_MS=60000        -> 30000 < 60000 -> buildApp() throws

Your in-code defaults (90s/60s) are fine — it's only the shipped examples and the README that are wrong. Anyone following the setup docs gets a hard crash on boot.

3. HIGH (process) — the new CI job doesn't test the upgrade path

Both passes found this independently. prisma.config.ts:8 pins migrations.path relative to cwd, which overrides the --schema <develop schema> you pass at check-upgrade-prisma-migrations.mjs:95. Proof from running your own script as ci.yml invokes it:

Deploying base branch migration history
  Applying migration `20260805123000_add_world_seed`     <- a PR-only migration
  Applying migration `20260810143000_add_canon_voting`   <- a PR-only migration

It deploys the PR's 12 migrations as the "base", so baseStatus is always 0, the --rolled-back recovery is dead code, the second deploy is a no-op, and the job passes unconditionally. It would stay green if the upgrade were completely broken.

The migration fix itself is correct — I ran the real upgrade by hand and it succeeds with no drift, from both --applied and --rolled-back starting states, and your README documents the recovery command. It's the guard that's broken, not the thing it guards. Fix is to run the base deploy with cwd (or migrations.path) pointing at the extracted develop tree.

Worth saying plainly, because it's the second instance today across the team: a check that cannot fail is worse than no check, because it converts "untested" into "verified" on the dashboard. Please make it fail first — break the upgrade deliberately, watch the job go red, then fix it.


Also worth fixing

  • MEDIUM — confirm authorization is world-level, not actor-is-author. /canon-transaction enforces SIGNER_NOT_AUTHOR, but confirmCanonTransaction never receives an actor id, so an unrelated CONTRIBUTOR can confirm someone else's decision and permanently link it (reproduced, 200). Impact is bounded — payload hash, entity id and posting-auth signer are all still verified, so a third party can't forge the content — but the link is permanent and no WorldAuditLog row is written on confirmation at all, unlike finalize. So there's no record of who did it.
  • MEDIUM — Hive errors surface as HTTP 500. handleCanonVotingError only maps CanonVotingError; HiveBroadcastError falls through to Fastify: {"statusCode":500,"code":"BROADCAST_CONFIRMATION_TIMEOUT","error":"Internal Server Error"}. Ordinary user-side timeouts will fire 5xx alerts. Side effect: the intended 404 HIVE_OPERATION_NOT_FOUND branch is now dead whenever a broadcaster is configured, which is the production default.
  • MEDIUM — the confirm rate limit is per user, not per proposal (3/60s). Reproduced: after three confirms the author was 429'd on a different proposal. Combined with the 500s above requiring retries, that budget is thin.
  • LOW — an author demoted to READER after finalize is permanently locked out (403). Only the author may sign, but the check requires SUBMIT_PROPOSAL and there's no curator override, so an approved decision can never reach the chain.
  • LOW — rulesVersion is stamped but never read. tallyCanonVotes/decideCanonOutcome always use current CANON_VOTING_RULES, and serializeProposalDetail recomputes a live tally with current rules — so after any future rule change, a decided proposal's stored tally and its displayed tally will silently disagree. Also, the denominator change didn't bump rulesVersion; it's still canon-voting-mvp-2026-08-11 from the earlier commit, so that string now means two different rule sets. Harmless pre-merge since nothing is persisted yet — please bump it and read it when re-deriving.
  • LOW — remainingDeadlineMs isn't clamped with Math.max(0, …) at broadcast-reliability.ts:255 the way the ambiguous path at :274 is. NIT — the chain id is pinned by startsWith('beeab0de') rather than equality with HIVE_MAINNET_CHAIN_ID; one === from being a true pin.

Genuinely good round — the parts that were hardest to get right are right. Items 1–3 are what stand between this and merge.

@mcfarhat

Copy link
Copy Markdown
Contributor

Following up on blocker 1 with something more useful than "this is broken" — I dug into what the confirmation path should call, and I think the fix is much smaller than it looks.

Why it can't work today

/block-search on hafbe returns a per-block summary, not operations. Live:

GET api.hive.blog/hafbe-api/block-search?operation-types=18&from-block=…&to-block=…

{"total_blocks":11,"total_pages":6,"block_range":{...},
 "blocks_result":[{"block_num":108931001, ..., "operations":[{"op_type_id":18,"op_count":12}]}]}

That inner operations array is {op_type_id, op_count} — it tells you "block 108931001 contains 12 custom_json operations", never what they were. parseBlockSearchPage looks for operations_result | operations | results | rows | data, finds none of them at the top level, and throws. That error classifies as unknown, pollForConfirmedOperation only rethrows permanent, so it retries every 3s for the whole confirmation window and then fails. Every time.

Your operationTypes: [18] filter is right — I checked, op_type_id 18 is custom_json — the endpoint just doesn't return what the parser needs.

The fix: you already have the transaction id

This is the part worth pausing on. The code scans a 1200-block range looking for an operation whose transaction id you already know — Keychain returns it at broadcast, and confirmBodySchema already accepts transactionId (with blockNumber/operationIndex as optional hints). So the whole scan can collapse into a single lookup:

condenser_api.get_transaction(transactionId)

Verified live against a real custom_json on mainnet, tx 4a98edfae949b1ab66ac24e34982570fce9b3787:

api.hive.blog       -> OK  block_num 108933095  ops=['custom_json']
rpc.mahdiyari.info  -> OK  block_num 108933095  ops=['custom_json']

response keys : block_num, transaction_id, transaction_num, operations, signatures, expiration, ...
operation body: { id, json, required_auths, required_posting_auths }

That gives you everything the verification needs, in one call, with no paging:

  • block_num — what you were scanning to find
  • operations[].json — feeds hashCanonicalJson for the payload comparison, unchanged
  • required_posting_auths / required_auths — feeds your posting-authority check at canon-voting.ts:1021-1031, unchanged
  • transaction_id — lets you assert it matches what was requested (worth adding; the current code trusts the node's value)

So findAndVerifyOperation keeps its logic; only the fetch changes. And "not on chain yet" becomes an explicit signal — the node returns an Unknown Transaction assert error — instead of an ambiguous parse failure, which also fixes the classification problem: that's a clean "keep polling", distinguishable from "the endpoint is wrong".

One caveat to design for: get_transaction needs a node with transaction lookup enabled. api.hive.blog and rpc.mahdiyari.info both answered; three other public nodes were 403/502/timeout while I tested, which is normal node churn — so keep the existing pool/failover around this call rather than hardcoding one URL.

Keep the block-range scan only as a fallback for the case where the tx id genuinely isn't known. If every caller supplies it, you can drop that path and a good chunk of haf-client.ts with it.

Then the DoD

Once this works, please do the actual round trip the card asks for: broadcast one real custom_json with your posting key on mainnet, confirm it through this path, and post the tx id on the card. That's the whole point of the mainnet requirement — it's exactly the check that would have caught this on day one, since no amount of unit testing against a stubbed transport can tell you the endpoint returns the wrong shape.

Posting-authority only, never active or owner. Say the word if you need RC delegation.

mcfarhat added a commit that referenced this pull request Aug 11, 2026
…#23)

The MVP canon rule counted different populations in its two halves: the
quorum counted all four vote choices, while the approval percentage
counted only Approve + Reject.

Measured against the implementation, that let 1 Approve + 4 Needs
Revision satisfy both conditions and canonize permanently on-chain, and
5 Approve + 20 Needs Revision did the same. It also meant a voter
choosing Needs Revision had less effect on the outcome than one voting
Reject, penalising the more constructive option.

Decision by Mohammad, 2026-08-11: count every vote cast in the
denominator so both halves of the rule count the same population.
Already implemented in PR #22.

Co-authored-by: Claude <noreply@anthropic.com>
…iability-fix-kareem

# Conflicts:
#	apps/api/prisma/schema.prisma
#	apps/api/src/app.ts
#	apps/api/src/routes/contributions.test.ts
#	apps/api/src/routes/contributions.ts
#	apps/api/src/routes/worlds.ts
@KareemN1936

Copy link
Copy Markdown
Collaborator

Pushed 22ee3aa for PR #22.

Covered:

  • Merged current develop and resolved the five backend conflicts (schema.prisma, app.ts, contributions/worlds route files/tests).
  • Reworked confirmation polling to prefer condenser_api.get_transaction(transactionId) through the Hive RPC node pool, keeping block-search only as a fallback for transports without transaction lookup.
  • Added a regression test proving transaction lookup is used instead of HAF block-search, while preserving canonical operation comparison.
  • Updated .env.example, apps/api/.env.example, and README so HIVE_BROADCAST_TOTAL_DEADLINE_MS=90000 satisfies the confirmation timeout validator.
  • Fixed the Prisma migration upgrade check to run the base deploy/resolve from the extracted base API workspace, so it uses the base branch migrations rather than this PR's migrations.
  • Tightened Hive mainnet chain id validation to exact equality.

Local validation:

  • npm run typecheck --workspace=@hivelore/api
  • npm run lint --workspace=@hivelore/api
  • npm test --workspace=@hivelore/api -> 174/174 passing
  • npm run build --workspace=@hivelore/api
  • npm run typecheck --workspace=@hivelore/web
  • npm run build --workspace=@hivelore/web
  • Focused broadcast-reliability.test.ts -> 14/14 passing

Still not done by me: the real mainnet custom_json smoke confirmation / tx id, because that needs explicit account/key/permanence approval.

@KareemN1936

Copy link
Copy Markdown
Collaborator

Pushed 28a074e to fix the remaining CI migration-upgrade failure.

What changed:

  • The upgrade check now writes a minimal Prisma config into the extracted base-branch API workspace.
  • Base migrate deploy / migrate resolve now run with that config, so Prisma gets DIRECT_URL while still loading schema + migrations from the extracted develop tree.
  • This avoids the current-branch migrations/config leaking into the base-history phase.

Validation:

  • npm run lint --workspace=@hivelore/api
  • DATABASE_URL=... DIRECT_URL=... npm run typecheck --workspace=@hivelore/api
  • GitHub Quality is green on 28a074e.
  • GitHub Vercel Preview is green on 28a074e.

PR #22 is now conflict-free and checks are green. Remaining blocker is reviewer re-review / approval; I still did not run the real mainnet smoke tx because that needs explicit account/key/permanence approval.

@KareemN1936
KareemN1936 requested a review from mcfarhat August 11, 2026 20:12
… voting

- Implemented `getTransactionOperations` method in `HiveBroadcastTransport` to fetch transaction operations by ID.
- Enhanced `pollForConfirmedOperation` to utilize direct transaction operation lookups.
- Updated `HafOperationRow` type to include `op_in_trx` for better operation indexing.
- Modified `network-config.ts` to increase `totalDeadlineMs` for retries.
- Added new tests for canon transaction confirmation, ensuring atomicity across concurrent clients.
- Introduced `authorizeCanonTransactionPermission` to validate user permissions for canon transaction operations.
- Updated error handling in proposal routes to manage Hive broadcast errors more effectively.
- Created a script to check for Prisma migration upgrades against the base branch.
- Added a new migration to include `CANON_DECISION_CONFIRMED` action in `WorldAuditAction`.

@mcfarhat mcfarhat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at 187789e5. All three blockers are closed, and the CI one is closed in the strongest sense — I proved the check can now fail. Two new items before I merge, one of which is a defect introduced by the confirmation fix.

The three blockers

1. Confirmation against real mainnet — CLOSED. I drove the production path (HiveReliableBroadcaster → DefaultHiveBroadcastTransport.getTransaction → transactionLookupToRows → findAndVerifyOperation) with only fetch stubbed, against shapes captured live from api.hive.blog:

Case Outcome
Happy path, exact live shape CONFIRMED, 1 RPC call
Not on chain yet (live error string) keeps polling, then clean BROADCAST_CONFIRMATION_TIMEOUT
Unknown for 2 polls, then appears CONFIRMED on poll 3
Node returns a different transaction_id fails closed
Wrong signer on chain BROADCAST_REJECTED — signer mismatch
Tampered json payload BROADCAST_REJECTED — payload mismatch

Payload round-trips byte-identical; posting-authority and payload-hash checks both fire. The "Unknown Transaction" case I worried about is genuinely clean — I queried mainnet with a bogus txid, and :182 matches it and returns null, which the poll loop treats as "not yet" rather than a hard failure.

Worth knowing: getTransaction wins unconditionally — pollForConfirmedOperation:562 gates the block-scan branch on input.transport.getTransaction, and the default transport implements it. So the block scan is now dead code in production. That's fine as a resolution, but be aware parseBlockSearchPage is still broken (I re-fed it the live payload — still throws), and indexer/haf-sync.ts:105 still depends on it. Pre-existing, out of scope here, but the standalone indexer can't work against real HAF.

2. .env.example boot crash — CLOSED. Booted the server on your shipped apps/api/.env.example verbatim: HEALTH OK, {"status":"ok"}. The invariant is still real, not deleted — restoring the old value reproduces the original crash at network-config.ts:141. (Separately: the example still can't boot fully verbatim because GOOGLE_REDIRECT_URI= is empty and fails z.string().url() at env.ts:111. I checked — that's pre-existing on develop and not yours. Separate ticket.)

3. CI upgrade job — CLOSED, and I proved it fails. The two deploy steps now read genuinely different migration sets (13 from develop, then 18 from the branch), where before it reported 18/18 twice. Three deliberate breakages all went red: schema drift with no migration, invalid SQL in a PR migration, and — the one that matters — an upgrade-only collision injected into the develop copy, which the fresh check passes cleanly (EXIT 0) and which was impossible to trigger under the old script. Control run on a pristine copy: EXIT 0. That's a real guard now.


Blocking: blockchainTimestamp records the transaction expiration, not the block time

transactionLookupToRows:763:

const timestamp = transaction.timestamp ?? transaction.block_time ?? transaction.expiration;

condenser_api.get_transaction returns neither timestamp nor block_time. I checked this against two different live mainnet transactions — the top-level keys are block_num, expiration, extensions, operations, ref_block_num, ref_block_prefix, signatures, transaction_id, transaction_num. So it always falls through to expiration.

Measured just now on a live transaction:

recorded blockchainTimestamp : 2026-08-12T11:21:27   (= expiration)
actual block time (108957568): 2026-08-12T11:11:33   (get_block_header)
                        skew : +9m54s, always in the future

Expiration is a deadline the signer chose, not a fact about the chain — it's always ahead of the block, and under a different expiry window the error changes size. This gets persisted to ProposalDecision.blockchainTimestamp and HiveEvent.blockchainTimestamp (canon-voting.ts:1157,1199,1207, projection.ts:69), so every canon decision confirmed through this path records an on-chain time that never happened. That's the kind of wrong data that's painful to find later and worse to backfill, which is why I'd rather not merge it.

Two acceptable fixes: fetch get_block_header(block_num).timestamp (one extra call, exact), or drop the expiration fallback entirely and let normalizeHafOperation:47 fail loudly. Either is fine — silently recording the wrong one isn't.

Blocking (one line): the concurrency race test never runs in CI

canon-voting-concurrency.test.ts:200 self-skips on TEST_DATABASE_ADMIN_URL is not configured, and the Test step in ci.yml:78-83 sets only NODE_ENV, AUTH_JWT_SECRET, AUTH_REFRESH_SECRET. Your two migration-check steps do set it (:63, :72) — the test step was just missed. That's the 1 skipped test in the 186.

The test itself is good — I ran it against real Postgres and it passes in 2.84s, asserting fulfilled 1 / rejected 1 and exactly one HiveEvent and one WorldAuditLog. But as shipped it's decorative: CI reports green whether or not the race guard works. Add the same env line to the Test step.


Follow-ups, not blocking

  • No explicit transaction_id mismatch error. It fails closed (good), but burns the whole window and reports a generic timeout with no mention of the mismatch — undiagnosable in production.
  • A node 500ing for the whole window and an unclassifiable RPC error both surface as the same BROADCAST_CONFIRMATION_TIMEOUT, cause unrecorded. Putting the last classified error into diagnostics would fix the debuggability half properly.
  • findAndVerifyOperation:668-677 throws a permanent rejection when a row won't normalize, instead of skipping to the next. On a multi-op transaction with operationIndex omitted (schema marks it .optional()) this hard-fails; with the index supplied it confirms. Should continue, not throw.

Re-verified after the develop merge

Quorum ruling holds across 7/7 boundary cases (one of each choice → denominator 4, 2500bps — confirms all four choices count). Testnet is gone repo-wide: zero case-insensitive matches across every relevant extension, with network-config.ts:58,165 still positively pinning the mainnet chain id. Re-issue after confirmation → 409. Both canon routes authorized. All 7 return awaits present.

Gates: typecheck, lint, format:check all pass. 186 tests / 185 pass / 0 fail / 1 skipped (the one above), up from develop's 122. Mergeable clean.

On attribution

git merge-base develop pr22 equals develop's head, so the three-dot diff is clean — the lore files that appear in the commit-range view are develop's, pulled in by your merge commit 22ee3aa, and none of them are being attributed to you. Your genuinely new work here is broadcast-reliability.ts, canon-voting.ts, the concurrency test, the upgrade-check script, and two migrations.

Fix the timestamp and add the CI env line and I'll merge this. Everything else on the card's acceptance list is now real — the confirmation path works against mainnet shapes, which is what this card was actually about. The remaining checklist gap is the DoD itself: one real mainnet broadcast with the tx id posted on the card.

@KareemN1936

Copy link
Copy Markdown
Collaborator

Pushed 9c0f49a with the two remaining review blockers closed.

Covered:

  • condenser_api.get_transaction now follows with condenser_api.get_block_header(block_num) and uses the block header timestamp for confirmation rows, so persisted blockchainTimestamp no longer falls back to transaction expiration.
  • Added a regression test where transaction expiration differs from block time and the confirmed row keeps the block header timestamp.
  • Added TEST_DATABASE_ADMIN_URL to the CI Test step so the canon confirmation concurrency test no longer self-skips in GitHub Actions.

Verified locally:

  • npm run typecheck --workspace=@hivelore/api
  • npm run lint --workspace=@hivelore/api
  • npm run build --workspace=@hivelore/api
  • npm test --workspace=@hivelore/api without TEST_DATABASE_ADMIN_URL: 186 pass / 0 fail / 1 skipped
  • npx prettier --check .github/workflows/ci.yml apps/api/src/lib/hive/broadcast-reliability.ts apps/api/src/lib/hive/broadcast-reliability.test.ts

I also reran tests with TEST_DATABASE_ADMIN_URL set; the skipped test did start running, then failed locally with ECONNREFUSED 127.0.0.1:5432 because there is no local Postgres service in this shell. CI has the Postgres service for that env var, so it should exercise the test properly there.

- Added `extractRateLimitRouteInfo` function to streamline route info extraction in rate limiting.
- Updated `createPrismaRateLimitStore` to utilize the new extraction function.
- Enhanced canon voting tests to assert blockchain timestamps for confirmed transactions.
- Introduced new tests for Hive broadcast reliability, including transaction lookup and error handling.
- Implemented `getBlockHeader` in `DefaultHiveBroadcastTransport` for improved transaction confirmation.
- Added smoke operation functionality for Hive mainnet, including new tests and environment configurations.
- Created a CLI script for executing Hive mainnet smoke tests with confirmation prompts.
- Added utility functions for building and verifying smoke operations.
- Introduced new types and constants for smoke operations in the Hive module.
- Updated web package to include environment tests for API URL normalization.

@mcfarhat mcfarhat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes requested at bb3d46e7 — both requested items genuinely fixed

Both of the two items I left you are properly closed, and the timestamp one is exactly right. What holds this up is a new regression the timestamp fix introduced, plus unrequested scope that landed a private-key path.

Item 1 — blockchainTimestamp: fixed, delta 0 seconds

The expiration fallback is gone; getTransaction now calls getBlockTimestamp → condenser_api.get_block_header(block_num). Driven end-to-end into real Postgres with a node presenting a +600s expiration:

real block time (get_block_header)     2026-08-15T08:28:51Z
ProposalDecision.blockchainTimestamp   delta 0 s
HiveEvent.blockchainTimestamp          delta 0 s
old fallback would have been           delta +600 s

Mutation: restore the fallback and both persisted values move to +600s. The check has power.

And your failure handling is right — no silent re-fallback. All four modes throw: JSON-RPC error, result: null, HTTP 500, garbage timestamp.

One thing that strengthens the original finding: I measured the skew on two more live transactions and got +57s and +24s, against the +9m54s I saw first. It's signer-chosen, so the error has no stable size — which is exactly why it can't be tolerated rather than corrected.

You also used the same commit to close two follow-ups I hadn't pressed on: a genuine transaction_id_mismatch error, and the last classified failure now carried in diagnostics.

Item 2 — the CI line: fixed, and the test provably runs

From the real log of run 31873566559 on this head: ephemeral DB created, migrations applied, ok 10 - canon decision confirmation is atomic across concurrent PostgreSQL clients, duration_ms: 2728.7, # skipped 0.

Local A/B: with the var, 198/198 and 0 skipped; without it, 197 pass 1 skip and exit 0 — exactly the old green-that-can't-fail. Mutation: make the updateMany claim unconditional → not ok 10. The test bites.

You also closed old non-blocking items unasked — root test → --workspaces --if-present, a real web suite so that script isn't vacuous, and the GOOGLE_REDIRECT_URI boot wart. 122 → 200 tests, zero skips.


Blocking

R1 — the timestamp fix makes transient block-header failures permanently fatal

The new branch in classifyHiveBroadcastError substring-matches 'block header' and sits before every transient check:

PERMANENT BROADCAST_REJECTED   <- 'Hive block header lookup failed: 500 …'
PERMANENT BROADCAST_REJECTED   <- 503
PERMANENT BROADCAST_REJECTED   <- 429
PERMANENT BROADCAST_REJECTED   <- 'Hive block header lookup timed out'
TRANSIENT TRANSIENT_RPC_ERROR  <- 'Hive transaction lookup failed: 500 …'   (control)

Note the control: same HTTP status, different message text, opposite classification. Driven end-to-end, one 503 on get_block_header kills confirmation after 1 attempt in 24 ms of a 12,000 ms window — for a transaction that is on chain and valid. Counterfactual: delete only the 'block header' substring and you get 12 attempts across the full window, then a clean HIVE_OPERATION_NOT_FOUND.

This matters because api.hive.blog is load-balanced — the header call can land on a different backend than the tx lookup, and result: null from a lagging node gets the same permanent treatment.

It's the same shape as the lower.includes('expired') issue from my first review: classifying on transport text rather than meaning. Fix by classifying on your structured reasons (missing_or_invalid_block_timestamp, transaction_id_mismatch), or move the branch below the transient checks.

R2 — a server-side Hive private posting key is now a supported input

My first review recorded "no key in env, none server-side." The new scripts/hive-mainnet-smoke.ts reads HIVE_SMOKE_POSTING_KEY, and both .env.example files now ship a HIVE_SMOKE_POSTING_KEY= slot — in the exact file people copy to .env, which is the file the script reads. Your own README step 4 says "Never hardcode, print, persist, or commit a Hive private key."

The mitigations are real and I want to credit them: an interactive BROADCAST HIVELORE SMOKE gate, posting-authority asserted twice, the key never logged, shell env winning, and no route or CI path reaching it. The ask is small — drop the .env.example slot and keep it shell-only, exactly as your own comment says.


Non-blocking

R3 — the README documents the wrong on-chain bytes. README says {"type":"hivelore_mainnet_smoke","version":1,"purpose":"broadcast-readback verification"}; the code broadcasts {"app":"hivelore","type":"mainnet_smoke","version":1,"purpose":"broadcast_readback_verification"}. Three of four fields differ — and these are irreversible once broadcast.

R4 — the upgrade guard applies 1 of develop's 13 migrations. Not yours, but now measured. The base stage applies 20260718131500_init_hivelore_schema, then 20260718154842_init hits P3018 and rolls back; the base deploy is never re-run after the resolve, so develop's migrations 3–13 never land. A/B with an injected collision: placed before the failing baseline → red; placed after → blind. It's a real guard with a blast radius of one migration. Follow-up ticket, not this PR.

Also: verifyHiveLoreOperation was widened to accept hivelore_smoke ops, and that's the shared verifier the canon confirm path uses. Not exploitable — I drove a hivelore_smoke op through confirmCanonTransaction and it's rejected payload_mismatch by the expectedOperation equality check. Defence in depth holds. But the canonical verifier got looser for a test-only concern; worth a comment saying why.


The DoD checkbox

You ticked "Round-trip smoke test — tx id posted on the card" as complete, and commented "I used my own account and private posting key. everything should work perfectly."

There's no tx id on the card, and the checkbox text is specifically "tx id posted on the card". Separately, every smoke test in the suite is stubbed, so the harness built for this DoD has itself never run against mainnet.

I'm not assuming you didn't run it — you may well have and simply not pasted the result. So: which account, and what's the transaction id? Post the tx id and the block number and I'll verify it on chain in a minute and tick it myself.

The reason this matters more than bookkeeping: that DoD exists precisely because "should work" and "does work" diverge on the exact path you've been fixing all week. Your own expiration bug is the proof — the code looked right, the value was wrong by up to ten minutes, and only a real read-back exposed it.

The same applies to multi-node failover, which you also ticked. If that's verified, say how; if it's reasoned-but-untested, untick it and say so. Either is fine — an honest unticked box is worth more to me than a ticked one I have to audit.


Gates

                develop            this PR
tests           122/122            200/200, 0 fail, 0 skipped
typecheck/lint/format/build   0/0/0/0        0/0/0/0
CI on this head:  Quality success, Vercel Preview success

No lockfile changes at all, so the whole diff is source, docs and tests. Re-verified with no regression: confirmation against live shapes, quorum still can't rewrite history (zero of 13 tally fields changed by confirm, even after injecting post-finalize votes), the race still yields exactly one on-chain record, the timeout still leaves no half-written state — and the new header-failure path is equally clean, because the header call happens before the DB transaction opens.

Fix R1 and drop the .env.example slot, and this is ready.

@KareemN1936

Copy link
Copy Markdown
Collaborator

Pushed the requested follow-up in 47cde8e.

Done:

  • Kept transient block-header lookup failures retryable by removing generic "block header" text matching from the permanent rejection branch. Structured metadata failures still fail closed through their explicit HiveBroadcastError paths, including missing_or_invalid_block_timestamp and transaction_id_mismatch.
  • Added regression coverage for 503 and timeout block-header lookup failures classifying as transient.
  • Removed HIVE_SMOKE_POSTING_KEY= from both checked-in env examples, keeping the private posting key shell-only.
  • Fixed the README smoke payload so the documented irreversible bytes match the script's actual payload.

Verified locally:

  • npm test --workspace=@hivelore/api with local DB env placeholders: 197 pass / 0 fail / 1 skipped; skipped test is the real PostgreSQL concurrency test because TEST_DATABASE_ADMIN_URL is not configured locally.
  • npm run typecheck --workspace=@hivelore/api
  • npm run lint --workspace=@hivelore/api
  • npm run build --workspace=@hivelore/api

Code blockers from the last review should be closed. The remaining card DoD is still the real mainnet smoke tx id + block number; I do not have a private posting key/account in this environment, so that has to be run by the account holder and posted on Trello.

@mcfarhat mcfarhat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. My closing line last round was "Fix R1 and drop the .env.example slot, and this is ready." Both are done, and R3 came with them.

Everything below bb3d46e was covered across the four previous rounds. The only new code since is 47cde8e — 5 files, +16/−9 — so this pass is scoped to that delta plus re-verification of the two blockers.

R1 — transient block-header failures: FIXED

The fix is narrower than either option I suggested, and better for it:

-  if (
-    lower.includes('block metadata') ||
-    lower.includes('block header') ||
-    lower.includes('transaction id mismatch')
-  ) {
+  if (lower.includes('block metadata') || lower.includes('transaction id mismatch')) {

Only the transport-text match is gone. 'block metadata' and 'transaction id mismatch' stay permanent, which is right — those are read-back integrity failures, not transport failures. And a genuinely invalid header timestamp is still permanent, because it's thrown as a structured HiveBroadcastError and short-circuits at the top of classifyHiveBroadcastError. So the meaning is preserved while the 503/timeout/429 cases fall through to transient.

Verified by running it, not reading it. Suite is 20/20. Then I re-introduced the exact bug — a 'block header' → permanent branch ahead of the transient checks — and test 17 fails with expected: 'transient'. The test has power; it isn't just coverage.

R2 — private key slot: FIXED

HIVE_SMOKE_POSTING_KEY is gone from both .env.example files. It survives only in the README and in the script that reads it, i.e. shell-only, exactly as your own README step 4 requires.

R3 — README on-chain bytes: FIXED

The README now reads {"app":"hivelore","type":"mainnet_smoke","version":1,"purpose":"broadcast_readback_verification"}, which matches HIVELORE_MAINNET_SMOKE_PAYLOAD in smoke-operation.ts field for field. I checked the direction of the fix specifically — the docs were corrected to match the code, not the code bent to fit the docs. Right way round, and it matters because those bytes are irreversible once broadcast.

Still open, deliberately not blocking

  • R4 — the upgrade guard applies 1 of develop's 13 migrations. As I said last round, follow-up ticket rather than this PR. Carried to its own card.
  • verifyHiveLoreOperation widened to accept hivelore_smoke. Still not exploitable — confirmCanonTransaction rejects it payload_mismatch on the expectedOperation equality check — but the canonical verifier is looser for a test-only concern and still has no comment saying why. Worth one line whenever this file is next touched.
  • The mainnet DoD is unclosed. The round-trip smoke test checkbox is ticked but no tx id was ever posted, and every smoke test in the suite is stubbed, so the harness has never run against mainnet. That question was put to the original author and never answered. I'm not holding the merge for it — the code is reviewed and green — but I'm not treating it as satisfied either. It's tracked as its own card, and this card goes to Testing rather than Done until a real tx id and block number exist.

Gates

Quality and Vercel Preview both pass on this head. MERGEABLE / CLEAN. No lockfile changes.

Merging. Credit where it's due: the substance here is Perla's, and this last fix is Kareem's.

@mcfarhat
mcfarhat merged commit 46d6b7b into develop Aug 24, 2026
2 checks passed
@mcfarhat
mcfarhat deleted the hive-broadcast-reliability branch August 24, 2026 14:10
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.

3 participants