Whole-repo review: delete duplicated rules and dispatches - #66
Merged
Conversation
Five sites parsed a satoshi amount as a minimal integer in 0 to MAX_MONEY, each with its own range check and message, and two sites checked a scriptPubKey operand's atom-ness and size cap the same way. Both rules now live in one helper each, _parse_amount and _script_pubkey_atom, so the domain every amount operand shares is stated once. The scriptPubKey helper takes the minimum length because a claimed output's script is non-empty while the self-assert and specifier comparands may be empty, exactly as before. check_seals derived the txid and outputs hash lazily behind None sentinels as an optimization. The reference implementation is the spec artifact and derives both up front instead. Error codes, the vector corpus, and every parse outcome are unchanged. Only the messages behind bad_condition_arg reword, which nothing pins. spec/CONDITIONS.md sections on CREATE_OUTPUT, the self asserts, RESERVE_FEE, the specifier table, and the seals authorize the behavior, which this commit does not touch.
Definitions.add dispatches a declaration form on its keyword, the one place that knows which forms are declarations. compile_program and the REPL's declaration line each carried a copy of that dispatch with the same error message, and the REPL's copy is gone with it. Definitions also answers "name in defs" itself, so the three-space membership test the claim check and the REPL's def command both spelled out is written once. add_defun and add_defun_inline share the body they differed in by one dict. The cons chain unit 6d factored into _cons_onto for list and list* was still hand-rolled in three other places: the call site's argument list, let's rebuilt environment, and the inline tail parameter. All three use the helper now, and _proper_items raises its one error once. Compiled output is byte-identical, which the compiled-programs vectors and the puzzle mod-hash pins verify.
_condition_json was a twenty-branch isinstance chain restating each condition class's fields by hand. The pinned form is the same for every class: the opcode, then each field under its model name, integers as integers, bytes as hex, a specifier as its commitment and fields. The runner now derives that from the dataclass fields and keeps only the one genuine special case, a reserved condition's raw argument nodes serialized to hex. Every conditions vector passes unchanged, so the pinned shapes are exactly as before.
bitlisp-curry and bitlisp-uncurry shipped with unit 5 and the README still counted five.
Resequencing on the 2026-09-05 whole-repo review, decisions by Evan, recorded as skeleton changes under ground rule 6. The vault's serialized program is 2,562 bytes, about 640 vb under the witness discount before the solution, control block, and transaction, against the 526 vb single-exit gate and the evaluation doc's 336 to 374 vb envelope estimate. The commitment scheme (spec/SPEC.md sections 2 and 3, a stub until now) decides that number and shapes how the pool must be written, so it moves from Phase 4 to Phase 3 as unit 8, and the vault's measurement under it follows as unit 9. The pool comes after both, then the offer. The token study leaves the phase. The language is frozen until a puzzle shows a gap that costs witness bytes. The compiler optimizer versus hand-written fallback question is decided on the unit 9 number. Phase 4 scopes the comparator to one honest measurement, a CTV plus CSFS pool exit, and requires the weight mapping before any fits-the-gate claim. Phase 5 pulls a narrow hostile review of spec/VALIDATION.md forward to now, with the fresh-reader exercise owed from Phase 2 folded into it.
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
1 participant
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.
What changed
A first-principles pass over the whole tree, biased toward deletion, plus the execution-plan resequencing that came out of the review. Every code change is behavior-preserving. No spec text moves, no vector moves, and no compiled puzzle byte moves.
conditions.py: the satoshi-amount rule (minimal integer in 0 to MAX_MONEY) was written five times and the scriptPubKey operand rule twice. Each is one helper now.check_sealsderives the txid and outputs hash up front instead of lazily behind None sentinels.compiler.pyandrepl.py:Definitions.adddispatches a declaration on its keyword, replacing identical if-chains incompile_programand the REPL.Definitionsanswersname in defsitself. The cons chain unit 6d factored into_cons_ontoreplaces three remaining hand-rolled copies (call-site arguments, let's environment, the inline tail parameter).tools/run_vectors.py: the condition JSON form walks the dataclass fields instead of a twenty-branch isinstance chain. Reserved conditions keep their one genuine special case.README.md: lists all seven console scripts, not five.docs/execution-plan.md: the resequencing decided 2026-09-05. Unit 8 (commitment scheme, pulled from Phase 4) and unit 9 (the vault measured under it) come before the pool, the token study leaves the phase, the language is frozen, Phase 4 gets one honest comparator and the weight mapping first, and a narrow hostile review of VALIDATION.md moves to now. The evidence is in the commit message: the vault program alone is about 640 vb against the 526 vb gate.One thing deliberately not changed
bitlisp_tools/stepper.pyreproducesmachine.run's loop body verbatim (about 70 lines) and its docstring says so. I built a shared-executor refactor (aMachineclass inmachine.pythatrundrives to completion andDebugMachinedrives a step at a time), all suites green, then reverted it: the execution plan records your 2026-08-14 decision that the debugger stays standalone with the consensus loop untouched. If you want that decision revisited, the case for it is that every evaluator change must now be made twice and only the differential test catches a miss. The case against is thatmachine.pystays a closed function with no class surface for tooling to lean on. Your call, not a cleanup PR's.Review guide
Read the commits in order:
conditions: one amount rule and one scriptPubKey rule(consensus package, spec sections cited in the commit)compiler: one declaration dispatch and one cons chain(tooling)tools: the condition JSON form walks the dataclass fieldsdocs: the README lists all seven console scriptsplan: measure before the pool, review the validator nowVerify independently:
The compiled-programs vectors and both puzzle mod-hash pins prove the compiler emits identical bytes. The conditions vectors prove the JSON form is the same shape it was.