Repository navigation
0.41.1 — a failed keychain read must not mint a new wallet - #115
Merged
Merged
Conversation
0.41.0's keychainLoad() returned null for two different facts: the keychain has no entry, and the keychain could not be read. Under BLOCKRUN_KEYCHAIN=strict the plaintext file is already gone, so a transient read failure (locked keychain, ACL denial, timeout) was taken as "no wallet here" and fell through to getOrCreateWallet(), which does exactly what its name says. The funded wallet was still in the keychain; the MCP just started signing with a fresh empty one. keychain.ts names this scenario as the worst possible failure in a comment at the read site. Nothing acted on it. Reads now return a discriminated found / absent / error. Only absent may create a wallet; an error with no file left throws with the keychain fault and a pointer to BLOCKRUN_WALLET_KEY. auto mode was never exposed, since the file it keeps is found first. Also replaces a tautological guard I added in 0.40.x: it asserted auth - margin < submit + budget + timeout + auth, which has auth on both sides and holds for every input. The deadline calculation moved into pollDeadline() and the test exercises it — budget binds today, the authorization takes over above the 445s crossover, and a poll attempted at the deadline is refused. Writing that test found the crossover is 445s, not the 420s I first assumed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects, both of which I introduced in the last two releases and found on a second review pass.
1.
strictmode could mint a new wallet and orphan the funded onekeychainLoad()returnednullfor two different facts: the keychain has no entry, and the keychain could not be read.Under
BLOCKRUN_KEYCHAIN=strictthe plaintext file is already gone. So a transient read failure — locked keychain, ACL denial, timeout — was read as "no wallet here" and fell through togetOrCreateWallet(), which does exactly what its name says. The user's funded wallet was still sitting in the keychain; the MCP just started signing with a fresh empty one.keychain.tsnames this scenario, at the read site, as the worst possible failure:The comment identified the hazard. Nothing acted on it.
Reads now return a discriminated
found/absent/error. Onlyabsentmay create a wallet; anerrorwith no file left throws with the keychain fault and a pointer toBLOCKRUN_WALLET_KEY. A loud failure is recoverable; a silently substituted wallet is not.automode was never exposed — the file it keeps is found first, before any keychain read. Onlystrictusers could hit this, and only 0.41.0 shipped it.2. A guard I wrote in 0.40.x asserted nothing
authMsappears on both sides, so this is true for every possible input. It was decoration presented as a structural guarantee — the same failure I flagged in #110's original test, reproduced by me.The deadline calculation moved into
pollDeadline(), and the test now exercises real code: the poll budget binds at today's constants, the authorization takes over above the crossover, and a poll attempted at the deadline is refused rather than issued.Writing it honestly immediately paid: the crossover is 445s, not the 420s I had assumed, and the test failed until I corrected it. It then caught a second wrong claim — I had asserted a full-length poll starting at the deadline must finish before
validBefore, which is false and irrelevant, sincepollTimeoutForreturns 0 there. The real invariant is that the deadline stops a full margin short ofvalidBefore.Validation
npm test— 362 passednpm run typecheck,npm run build— cleankeychain-precedence.test.tsfailspoll.test.tsand the video clamp test failEach guard in this PR has been shown to fail when the code it protects is removed.