Hardening follow-ups: silent-placeholder error, locktime/feeUtxo footguns, Java funding parity#139
Open
icellan wants to merge 12 commits into
Open
Hardening follow-ups: silent-placeholder error, locktime/feeUtxo footguns, Java funding parity#139icellan wants to merge 12 commits into
icellan wants to merge 12 commits into
Conversation
lowerLoadProp's placeholder fallback coerced a property with no matching constructor slot (paramIndex === -1) onto slot 0, silently splicing an unrelated constructor argument's deploy-time bytes into the locking script. Turn that silent-wrong-code path into a hard error with a clear diagnostic and source location. Real constructor-param properties (readonly, or mutable state fields whose initial value is spliced at deploy) have paramIndex >= 0 and are unaffected — zero golden churn. Behavior-changing: needs a 6-tier port (stack lowering).
…ptions (#134, #118, #131) Gap 1: add DeployOptions (fundingSigner + satoshis/changeAddress) and a deploy(Provider, Signer, DeployOptions) overload. TransactionBuilder signs the deploy funding inputs with fundingSigner while the funding-UTXO lookup and default change address stay on the deploy signer, mirroring the TS SDK's DeployOptions.fundingSigner. Unset => deploy signer (zero behaviour change). Gap 2: thread fundingSigner (#134), feeUtxo (#118), and sequence (#131) through the multi-signer prepareTerminalCall path so it matches the primary callTerminal path. With a deterministic (RFC 6979) signer the prepared/finalized terminal tx is byte-identical to the primary callWithOptions tx.
A locktime gate (extractLocktime) is only consensus-enforced if the covenant also asserts the spending tx is non-final (extractSequence < 0xffffffff); otherwise an all-final-sequence tx bypasses the timelock. Add an advisory validator WARNING (non-fatal, via the existing diagnostics channel) when a public method reads the tx locktime — directly or transitively through a private helper — but never asserts a sequence-finality guard. Also covers the currentBlockHeight() sugar, which desugars to extractLocktime. Warning-only: no bytecode or golden change. Needs a 6-tier port (validators).
A CallOptions.feeUtxo is consumed entirely as fee — the terminal covenant binds the exact output set, so there is no change output and any excess is burned. Add a runtime console.warn (SDK logging convention) when the supplied feeUtxo is > 5x the terminal tx's estimated fee AND more than ~1000 sats of excess would be burned. Advisory only; best-effort (a fee-rate lookup failure skips the warning, never blocks the call). Does not add a change output (would break the covenant). TS SDK only — 6-SDK port is a follow-up.
(a) CallOptions.feeUtxo doc comment: the whole UTXO is spent as fee (no change) — size it to the intended fee; excess is burned. Note the SDK's oversize warning. (b) contract-patterns.md: a locktime-gated covenant must pair extractLocktime with an extractSequence < 0xffffffff assertion to be consensus-enforced — an all-final-sequence tx bypasses nLockTime otherwise.
H1 (#119 tail): stack lowering lowerLoadProp panics when a load_prop names a property with no constructor slot (Go's scan loop never breaks, paramIndex == count of ctor-params) instead of silently splicing an unrelated constructor argument onto slot 0. Legit ctor-param props unaffected — zero golden churn. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Go tier.
H1 (#119 tail): _lower_load_prop raises when a load_prop names a property with no constructor slot (find_index returns nil) instead of silently coercing it onto slot 0. Legit ctor-param props unaffected — zero golden churn. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Ruby tier.
H1 (#119 tail): lower_load_prop resolves the constructor slot via a new ctor_param_index_or_panic helper used by both placeholder arms; it panics when position() finds no slot instead of coercing onto slot 0. Legit ctor-param props unaffected — zero golden churn. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Rust tier.
H1 (#119 tail): lowerLoadProp throws when a load_prop names a property with no constructor slot (ctorProps.indexOf < 0) instead of silently coercing it onto slot 0. Legit ctor-param props unaffected — zero golden churn. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Java tier.
H1 (#119 tail): _lower_load_prop raises when a load_prop names a property absent from a non-empty registered constructor-param list instead of silently coercing it onto an unrelated slot. The raise is gated on ctor_params being non-empty because the Python TS-format parser drops inline 'readonly n' parameter-properties (they never enter ContractNode.properties); gating preserves the historical slot-0 placeholder for that parser-gap case and keeps zero golden churn while still catching the real ghost case. Legit ctor-param props unaffected. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Python tier.
H1 (#119 tail): lowerPropertyRead returns LowerError.LoadPropNoConstructorSlot when a load_prop names a property with no constructor slot (loop never breaks, found == false) instead of silently emitting a placeholder for an unrelated slot. Legit ctor-param props unaffected — zero golden churn. H2 (#131): validator emits an advisory warning when a public method reads extractLocktime (or currentBlockHeight) transitively but never asserts extractSequence < 0xffffffff. Bound accepts literal_int as well as literal_bigint since the Zig frontend lowers small bigints to literal_int. Warning-only, no bytecode change. Ports TS commits 482bcc5 (H1) and 8ff8f19 (H2) to the Zig tier.
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.
Hardening follow-ups: silent-placeholder error, locktime/feeUtxo footguns, Java funding parity
Small, mostly-advisory hardenings surfaced by the adversarial reviews during the #106–#134 remediation (#136, #138). No consensus/bytecode behavior changes for existing contracts — zero conformance golden churn.
H1 —
load_propwith no constructor slot now hard-errors (all 7 tiers) — #119 tailThe stack lowerer's
load_propfallback silently coerced a property with no deploy-time slot onto constructor slot 0 — pushing an unrelated arg's placeholder (a silent-wrong-code path). It now fails loudly with a diagnostic naming the property. Proven unreachable by any current fixture (instrumented: 4,852 fallback hits, none on the error path), so zero golden churn; legitimate constructor-param and mutable-slot properties are unaffected. Ported to all 6 non-TS tiers.constructor(readonly n)param-properties never enter the property list) means Python errors only when the ctor-param list is non-empty — the real ghost case still errors; the parser gap is flagged as a separate follow-up.H2 — locktime soundness warning (all 7 tiers) — #131 follow-up
A method reading
extractLocktime(orcurrentBlockHeight()) is only consensus-enforced if the covenant also assertsextractSequence(preimage) < 0xffffffff(else a hand-built all-final-sequence tx mines immediately despite a future nLockTime — the #131 consensus-review finding). The compiler now emits an advisory warning (transitive through private helpers) when the sequence guard is missing. Warning-only → no bytecode change. Ported to all 6 non-TS tiers.H3 — feeUtxo oversize warning + docs — #118 follow-up
CallOptions.feeUtxois consumed entirely as the miner fee (no change output — inherent to the exact-output covenant binding). The SDK now warns when afeeUtxodwarfs the estimated fee (best-effort, never blocks the call), and thefeeUtxodoc + a contract-patterns callout document both this and the H2 locktime/sequence pairing. (TS SDK; 6-SDK warning port is a minor follow-up.)H4 — Java SDK funding parity — #134 / #118
DeployOptions.fundingSignerwired for Javadeploy()(was TS-only) — funding inputs signed by a separate funding signer, defaulting to the connected signer.prepareTerminalCall/finalizeCallpath now honorsfundingSigner(deploy()/call() sign P2PKH funding inputs with the connected contract signer — no separate funding signer; propose CallOptions.fundingSigner #134),feeUtxo(Terminal transactions cannot pay miner fees: add CallOptions.feeUtxo #118), andsequence(buildCallTransaction emits all-final input sequences, so CallOptions.locktime is not consensus-enforced #131) — verified byte-identical to the primarycallWithOptionspath.prepareCallpath would need a larger rewrite to threadsequence(it uses a simpler builder with no funding/OP_PUSH_TX two-pass) — scoped out.Verification (all green, zero golden churn)