Repository navigation
Hive broadcast reliability - #22
Conversation
…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.
…g confirmations and authority validation
- 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.
|
Vercel preview is ready: https://hivelore-qh3v3d9do-kareemn1936s-projects.vercel.app Commit: 1f2fe43 |
|
@perlaimad reviewed the delta only — this branch is exactly one commit ahead of 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. That said, the one path actually wired into the application cannot confirm a real transaction today. BlockersB1 —
|
|
@perlaimad addendum — my earlier review missed a commit, which is my mistake. I scoped it to
|
mcfarhat
left a comment
There was a problem hiding this comment.
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 singlehafUrl, and whileparseNodeListrequires https in production,HIVE_MAINNET_HAF_URLis onlyz.string().url()— plainhttp://passes. That's the one path described as the trust anchor. - Confirmation-side failover is inert:
DefaultHiveBroadcastTransport.searchBlocks/getHeadBlockdropnodeUrland always build a client from the singlehafUrl, soconfirmationPool'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 — sofetch failed,ETIMEDOUTandsocket hang upall classify asunknown. 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. Alsolower.includes('expired')is tested before any transient check, so an expired TLS certificate fails the whole broadcast as permanent and kills failover. broadcastSignedTransactionnever assertssignatures.length > 0— cheap guard, given the non-custodial claim rests on it.- Root
testis 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. 20260805170000is used by bothadd_auth_rate_limit_bucketsandadd_contribution_drafts. Name-tiebreak makes the order deterministic today, but it's a latent hazard.broadcast-reliability.test.ts:404-423is 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.
|
Pushed the review fixes for this PR. Commits:
Covered:
Verified locally:
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
left a comment
There was a problem hiding this comment.
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-transactionenforcesSIGNER_NOT_AUTHOR, butconfirmCanonTransactionnever receives an actor id, so an unrelatedCONTRIBUTORcan 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 noWorldAuditLogrow is written on confirmation at all, unlike finalize. So there's no record of who did it. - MEDIUM — Hive errors surface as HTTP 500.
handleCanonVotingErroronly mapsCanonVotingError;HiveBroadcastErrorfalls through to Fastify:{"statusCode":500,"code":"BROADCAST_CONFIRMATION_TIMEOUT","error":"Internal Server Error"}. Ordinary user-side timeouts will fire 5xx alerts. Side effect: the intended404 HIVE_OPERATION_NOT_FOUNDbranch 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_PROPOSALand there's no curator override, so an approved decision can never reach the chain. - LOW —
rulesVersionis stamped but never read.tallyCanonVotes/decideCanonOutcomealways use currentCANON_VOTING_RULES, andserializeProposalDetailrecomputes 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 bumprulesVersion; it's stillcanon-voting-mvp-2026-08-11from 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 —
remainingDeadlineMsisn't clamped withMath.max(0, …)atbroadcast-reliability.ts:255the way the ambiguous path at :274 is. NIT — the chain id is pinned bystartsWith('beeab0de')rather than equality withHIVE_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.
|
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
That inner Your The fix: you already have the transaction idThis 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 Verified live against a real That gives you everything the verification needs, in one call, with no paging:
So One caveat to design for: 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 Then the DoDOnce this works, please do the actual round trip the card asks for: broadcast one real Posting-authority only, never active or owner. Say the word if you need RC delegation. |
…#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
|
Pushed Covered:
Local validation:
Still not done by me: the real mainnet custom_json smoke confirmation / tx id, because that needs explicit account/key/permanence approval. |
|
Pushed What changed:
Validation:
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. |
… 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
left a comment
There was a problem hiding this comment.
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_idmismatch 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 intodiagnosticswould fix the debuggability half properly. findAndVerifyOperation:668-677throws a permanent rejection when a row won't normalize, instead of skipping to the next. On a multi-op transaction withoperationIndexomitted (schema marks it.optional()) this hard-fails; with the index supplied it confirms. Shouldcontinue, 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.
|
Pushed Covered:
Verified locally:
I also reran tests with |
- 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.
…core/hivelore into hive-broadcast-reliability
…oadcast reliability
mcfarhat
left a comment
There was a problem hiding this comment.
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.
|
Pushed the requested follow-up in Done:
Verified locally:
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
left a comment
There was a problem hiding this comment.
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.
verifyHiveLoreOperationwidened to accepthivelore_smoke. Still not exploitable —confirmCanonTransactionrejects itpayload_mismatchon theexpectedOperationequality 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.
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.