Repository navigation
Conversation
…l residual risk Hillary's #74 approval flagged that D5's row, §7.2 and the archive header all said apply --mainnet "auto-selects" the plan file -- overstating the mechanism. She read clarinet 3.23.2's actual source (components/clarinet-cli/src/frontend/cli.rs, ApplyDeployment / load_deployment_if_exists) rather than trusting the naming convention. I re-verified the same source independently before writing anything down. Actual behavior: apply --mainnet always recomputes a plan from Clarinet.toml first and diffs it against the on-disk file, prompting Overwrite? [Y/n]. It falls back to the on-disk file SILENTLY only when that recomputation errors -- which happens on any checkout without settings/Mainnet.toml (gitignored, true of every fresh clone). That silent-fallback case, once all 13 paths resolved, is what #74 actually closed: one Enter from broadcast with no deployer key even present. It does NOT close the case where settings/Mainnet.toml exists (a real deploy machine) -- there clarinet always recomputes fresh from Clarinet.toml regardless of this file, so the archive changes nothing. The residual risk on that machine was never this YAML; it's whatever Clarinet.toml currently resolves the 14 funds-bearing contract names to, which today is the contracts/test/ localized copies -- D6, not D5. Corrected in three places: D5's table row, §7.2, and the archive file's own header comment. Added a cross-reference from D6 back to this finding, since D6 was previously framed only as a coverage gap and this makes explicit that it's also what a real mainnet deploy would publish today. No diff to the archived plan's content, no code change. Verified: suite 256/256, clarinet check 211/0, both unchanged; archive YAML still parses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
unixwhisperer
left a comment
There was a problem hiding this comment.
Request changes. Before writing this I stopped trusting source-reading, mine included, and ran clarinet itself. The result corrects my #74 review, corrects #75, and surfaces something more important than either.
How I tested
- Binary: clarinet 3.23.2, the official
clarinet-linux-x64-muslrelease asset, which is the version CI uses. - Isolation: every run is inside
docker run --network none, so nothing can reach a real node. - Keys: where a
settings/Mainnet.tomlis needed, it holds a freshly generated, never-funded throwaway mnemonic (SP32FA8…). - Observing broadcasts: a local mock node on
127.0.0.1:20443logs every request. "Broadcast" below means aPOST /v2/transactionswas observed. In test copies only, the plan'sstacks-nodewas pointed at the mock. - Scope: a toy project covered each clarinet branch in isolation, then the real repo was tested at
56a51b1(before #74) and4af27a6(after #74), taken withgit archive.
Results (real repo)
| # | Machine | Invocation | Before #74 (56a51b1) |
After #74 (4af27a6) |
|---|---|---|---|---|
| R1/R2 | clean clone (no settings/Mainnet.toml) |
apply --mainnet, Enter at every prompt |
unable to compute an updated plan → falls back to the gen-1 plan → Continue [Y/n]? → exits 0 silently, zero requests |
exits 1, zero requests |
| R3 | has Mainnet.toml, key ≠ SP3TGRVG… |
apply --mainnet -d |
gen-1 plan loaded, no prompt → panic at onchain/mod.rs:568 (stx_accounts_lookup.get(expected_sender).unwrap()), nothing signed |
see R4 |
| R3b | has Mainnet.toml, key = plan's expected-sender (swapped in the test copy) |
apply --mainnet -d |
all 13 gen-1 contracts broadcast, no prompt | n/a |
| R4 | has Mainnet.toml, any key |
apply --mainnet -d |
see R3 | generates a 57-contract plan from Clarinet.toml and broadcasts it, no prompt |
| R5b | has Mainnet.toml, any key |
apply --mainnet, Enter, Enter (defaults) |
recompute succeeds offline from the vendored .cache → Overwrite? [Y/n] → same 57-contract plan broadcast |
same (toy T9: generate → Continue → broadcast) |
R4 and R5b produce byte-identical plans (sha256 217564c42bea…).
What this corrects
- A clean checkout could never broadcast, before or after #74. My #74 review said "one prompt away from broadcast", and #75 says "one
Enteraway from broadcast, with no deployer key even present". Both are wrong: afterContinue, apply needssettings/Mainnet.tomlto sign, and without it clarinet returns (R1). - The gen-1 plan could only ever be broadcast by the
SP3TGRVG…key. Clarinet signs each publish with theMainnet.tomlaccount matchingexpected-sender. Any other key panics before signing (R3). All 13 of its names already exist atSP3TGRVG…(checked viaGET /v2/contracts/interface), so a broadcast would be refused as duplicates. That last step is inferred: it wasn't tested against a real node. - The recompute-error fallback is real but didn't trigger here. A toy project with an unfetchable requirement showed it (
unable to compute an updated plan→ stale plan → broadcast afterContinue). In this repo, recomputation succeeds offline because.cacheis vendored. - "Silently" is wrong in one direction and understated in another. The fallback prints an error first. But in R1 the exit is silent: after
Continue, clarinet exits 0 with no message, which an operator could read as success.
So #74 was harmless hygiene. It removed a route only the gen-1 key could use, and one the chain would have refused. It did not reduce the real exposure, which is below.
The real exposure (independent of #74, present on main today)
On any machine with a settings/Mainnet.toml, whatever key it holds, clarinet deployments apply --mainnet publishes the plan clarinet computes from Clarinet.toml, with two Enters by default or with no prompt under -d. That plan has 57 contract publishes, 26 of them from contracts/test/:
- The localized copies of every funds-bearing contract. In these copies, every canonical
SM3VDXK…sbtc-tokenreference (the real bridged sBTC) becomes.sbtc-token, and in this same plan that resolves tocontracts/sbtc-token.clar, the flash-mintable mock (12contract-call?s incontracts/test/flashstack-sbtc-pool-v3.clar, e.g. lines 91, 102; 6 incontracts/test/flashstack-sbtc-core-v2.clar, e.g. lines 69, 80). - Test fixtures:
malicious-token,mock-usdcx,test-receiver-bad,test-pool-v3-receiver-reentrant, …
At SPR9PQANV6…, 54 of the 57 names are free. That includes every undeployed successor: flashstack-pool-v3, flashstack-stx-pool-v3, flashstack-sbtc-pool-v3, flashstack-sbtc-core-v2, flashstack-stx-core-v2. Contract names can't be reused, so one mistaken run from a machine holding that key would permanently occupy the intended mainnet names with test builds bound to a mock sBTC, under the real FlashStack principal.
With the file gone, #74 does make -d reach this plan with no confirmation where it previously panicked for any key other than SP3TGRVG…. I'm not suggesting reverting #74. The stale plan only guarded -d by accident, and the default Enter-Enter path reached the 57-contract plan before #74 anyway.
Requested changes to #75
- Replace the mechanism text in the D5 row, §7.2 and the archive header with what the table above shows. In short: a clean checkout cannot broadcast; the gen-1 plan was only signable by
SP3TGRVG…and every name already exists there; #74 is hygiene, not risk reduction. - Rewrite the D6 addition. It currently says a deploy run "would publish these 14 contracts as their
contracts/test/localized copies". The tested result is 57 publishes, 26 fromcontracts/test/, including test fixtures, with sBTC resolving to the in-plan mock. Keep it cross-linked from D5. - Dates:
2026-09-23→2026-09-26, in three places.
Separately, for you (not #75's scope)
- Does any machine have a
settings/Mainnet.toml, and for which key? That one fact decides whether this is theoretical or live. - Cheap regression guard, which I can write: a test that generates the mainnet plan against a dummy
Mainnet.toml, offline, and fails if anypath:is undercontracts/test/. It runs in seconds here because.cacheis vendored. It goes red today, which is the point. - Operational rule until D6's structural fix lands: never run
clarinet deployments apply --mainnetfrom this repo. Mainnet publishes go through an explicit, reviewed-p <plan>or thescripts/path.
Happy to approve #75 once the mechanism and D6 text match the evidence. Test harness, case logs and the generated 57-contract plan are available if you want to re-run any row.
…orrection) Requested changes from Hillary's #75 review. Two prior passes at this section relied on reading clarinet's source -- mine and hers, independently -- and both were wrong. She then actually ran clarinet 3.23.2 (the CI binary) in a network-isolated container against the real repo, before and after #74, with a mock node logging every broadcast attempt. I re-ran her test (#76, merged) and independently reproduced the core claim from scratch outside the harness before writing any of this down: 57 publishes, 26 from contracts/test/, matching exactly; the sbtc-token references she cited at lines 91/102 of the test sbtc-pool-v3 copy matched exactly; all five undeployed audit-track successor names re-confirmed 404 today. What actually changes: - D5: a clean checkout never broadcasts, before or after #74 (recompute fails, falls back, prompts, exits with zero requests either way). The gen-1 plan could only ever be signed by SP3TGRVG..., whose 13 names already exist on mainnet, so a real broadcast would be refused as duplicates regardless. #74 is hygiene -- it removed a route only that key could use, one the chain would have refused anyway -- not the risk reduction either earlier version of this row claimed. - D6: the real, still-live exposure, unaffected by #74. Any machine with settings/Mainnet.toml, any key, reaches the SAME plan clarinet computes fresh from Clarinet.toml (byte-identical before/after #74) -- 57 publishes, 26 from contracts/test/, sBTC resolving to a flash-mintable mock, 54 of 57 names free including all five successors. Now pinned by tests/mainnet-plan-guard.test.ts (#76). - Archive file header: same correction, so the historical record doesn't repeat either wrong prior explanation. - Dates: 2026-09-23 -> 2026-09-26 in all three places, per review. Verified: suite 258 passed / 1 expected fail (259) across 25 files (unchanged from post-#76 main), clarinet check 211/0 (unchanged), archive YAML still parses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Sorry for the silence — pushed the corrected writeup (426e55e) right after your review but never actually replied here, so this was sitting ambiguous on your end. Fixing that now, days late. Reproduced your core finding independently before accepting it, not just trusting the table: ran your test (2 passed, 1 expected fail), then separately from scratch outside the harness — temp copy of Clarinet.toml/contracts/.cache, same public devnet mnemonic, D5 and D6 rewritten per your exact correction: a clean checkout never broadcasts, before or after #74; the gen-1 plan could only ever be signed by SP3TGRVG… and all 13 names already exist there, so #74 was hygiene, not risk reduction; the real exposure is D6, unaffected by #74, same plan before and after (sha256-identical). Dates fixed to 09-26 in all three places. On your four asks:
Re-requesting your review on this. |
unixwhisperer
left a comment
There was a problem hiding this comment.
Re-reviewed at 316f576: my merge of main (e0367e0), which brought in #83–#86. The net diff vs main is still only the archive header and CONTRACT_INVENTORY.md. CI is green except Dependency Audit, which is the accepted braces advisory (GHSA-vfj7-8cjw-p6xm) from #85 and unrelated to this PR.
Against my 2026-09-28 asks:
- ✅ D5 row, §7.2, archive header — mechanism. The clean checkout never broadcasts (exit 0 / exit 1, zero requests), the gen-1 plan was signable only by
SP3TGRVG…, and #74 was hygiene rather than risk reduction. All three now match the tested results. - ✅ D6 rewrite. It now covers the 57-publish plan, the fixtures, the sBTC-to-mock resolution, the five successor names and the cross-link to D5.
- ✅ Dates. No
2026-09-23remains in either file.
Two things still don't match the evidence:
1. -d was not the same before and after #74. This is the one place where #74 changed behaviour. In row R3 of my test table (pre-#74, settings/Mainnet.toml with any key other than SP3TGRVG…), apply --mainnet -d loaded the gen-1 plan and panicked at onchain/mod.rs:568 before signing. Only after #74 (R4) does -d reach the fresh 57-publish plan and broadcast it with no prompt. The default Enter-Enter path is the one that was identical before and after (R5b). Currently all three places say the default path and -d "both" reach the fresh plan with the "same result before and after #74":
CONTRACT_INVENTORY.mdD5 row (line 111)- §7.2 (around line 205)
- archive header (lines 29–31)
Suggested wording: the default path (Enter, Enter) reached the fresh Clarinet.toml plan both before and after #74 (same plan, sha256 217564c42bea…); -d panicked before signing pre-#74 for any key but SP3TGRVG…, and since #74 broadcasts that plan with no confirmation. This doesn't argue for reverting #74, for the same reason as before: that panic was protecting -d by accident.
2. "26" is stale since this merge. On current main, the guard pins 28 contracts/test/ paths, because #83 and #86 each added a receiver (#88, if merged, makes it 30). D6 says the guard "fails if the known 26 changes". The 57/26/54 figures are fine as the dated 2026-09-26 measurement, but the guard sentence should either say "the known set" without a number or carry an as-of date. deployments/README.md on main has the same stale 26. That file is outside this PR, so I'm happy to fix it separately.
Both are wording-only. I'll approve as soon as they're in.
Separate question, not blocking: on the Mainnet.toml your comment found (mnemonic fails BIP-39 validation), was that confirmed as a stale placeholder? It's still the open question on the D6 finding. If it is just a placeholder, it may be worth deleting so the "any machine with Mainnet.toml" condition simply doesn't hold there.
…ary's 2026-10-05 re-review Two wording-only fixes, exactly as requested: 1. The default `apply --mainnet` path (Enter, Enter) and `-d` were NOT identical before/after #74 -- only the default path was. `-d` panicked before signing pre-#74 (any key but SP3TGRVG...) and broadcasts with no confirmation since #74. Fixed in all three places: CONTRACT_INVENTORY.md's D5 row, its SS7.2, and the archive header -- the last wasn't explicitly named but has the identical conflation in the same paragraph. 2. D6's "fails if the known 26 changes" is stale -- the guard's KNOWN_TEST_PATHS is at 28 after #83/#86's merges. Reworded to point at the guard as the live count rather than repeating a number that drifts. Same fix applied to the archive header's copy of the same figure. deployments/README.md has the same stale 26 but is outside this PR, per Hillary's own note -- left for a separate fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Follow-up to #74, per your own review comment there. You flagged that D5's row, §7.2 and the archive header all said
apply --mainnet"auto-selects" the plan file — overstating it — and cited clarinet 3.23.2's actual source rather than the naming convention. I re-verified the same source independently (components/clarinet-cli/src/frontend/cli.rs,ApplyDeployment/load_deployment_if_exists) before writing anything down, and it matches what you described exactly.The correction
apply --mainnetalways recomputes a plan fromClarinet.tomlfirst and diffs it against the on-disk file, promptingOverwrite? [Y/n]. It falls back to the on-disk file silently only when that recomputation errors — which happens on any checkout withoutsettings/Mainnet.toml(gitignored, true of every fresh clone).So #74 closes exactly that silent-fallback case: once all 13 paths resolved, it was one
Enterfrom broadcast with no deployer key even present. It does not close the case wheresettings/Mainnet.tomlexists (a real deploy machine) — there, clarinet always recomputes fresh fromClarinet.tomlregardless of this file. On that machine the residual risk was never this YAML; it's whateverClarinet.tomlcurrently resolves the 14 funds-bearing contract names to, which today is thecontracts/test/localized copies — D6, not D5.Changes
*corrected 2026-09-23*.deployments/archive/gen1-mainnet-plan-2026-09-22.yaml): same correction, so the record itself doesn't repeat the overstatement.apply --mainnetwould publish today on any machine withsettings/Mainnet.toml.Not touched
No diff to the archived plan's content, no code change, docs only.
Verification
Suite 256/256,
clarinet check211/0, both unchanged. Archive YAML still parses.🤖 Generated with Claude Code