fix(ledger): apply the collateral rules only to phase-2 transactions

[chrisguiney]

Reported as blinklabs-io/dingo#3896.

Collateral pays for phase-2 script execution that fails. A transaction that runs
no phase-2 scripts has nothing for it to cover, but the collateral witness rules
were applied to any transaction that declared collateral — so declaring
collateral it did not need made an otherwise valid transaction invalid.

How it surfaced

A from-genesis Preview replay wedged at slot 15148509 (epoch 175):

tx 9ce59ee0dc6abee0de759c33b4b0f59dedda2f2eb56b89b6978b5d098100ac44
  validation failure at slot 15148509:
  transaction: collateral input must be key-locked

114 occurrences, node no longer following the chain.

The transaction:

body     inputs, outputs, fee 177117, collateral [811c9029...674f#0]
witness  2 vkey witnesses
         1 native script
         no plutus_v1/v2/v3, no redeemers

Its collateral input really is script-locked — that part of the check is right:

addr_test1wrpe58z89f3kwtq2cslsqvvw9hzdep2jy2ykx9kty8xnamqycrwy6
header 0x70 -> type 7, enterprise(script)

Koios confirms the transaction is on chain at block height 670981 with no
collateral consumed, so it was phase-2 valid and the collateral was never
touched.

The change

ValidateCollateralVKeyWitnesses previously returned early only when no
collateral was declared. It now also returns when the transaction runs no
phase-2 scripts.

The condition is the presence of redeemers, not of Plutus scripts in the
witness set. A script supplied by a reference input is not in the witness set,
so gating on that would skip the check for exactly the transactions that most
need it. Every phase-2 execution carries a redeemer wherever its script came
from.

What I have not confirmed

I have not verified this against cardano-ledger, and that is worth a
reviewer's attention before merge.

What is certain is that the current behaviour is wrong: the block is canonical
and a node applying this rule cannot follow the chain. What I am inferring is
the precise condition the ledger uses — whether it gates on phase-2 scripts
being present, on a non-empty redeemer set, or on something else again.

Redeemers seemed the most defensible reading, and it is strictly narrower than
removing the check: a transaction that does run phase-2 scripts is still held to
the rule, which the second test asserts. But if the reference implementation
gates differently, this should follow it rather than the reading that happens to
make this block pass.

Tests

TestCollateralKeyLockedOnlyForPhase2 uses the real transaction's shape and the
real collateral address, and covers both directions: script-locked collateral is
accepted without phase-2 scripts, and still rejected with them, so the guard
cannot become a way to skip the rule. Without the change the first case fails
with collateral input must be key-locked.

go build ./..., go vet ./ledger/... and go test ./... (39 packages) pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_019cXreCqyqbGJuDtvgYyqQN

---

Summary by cubic

Applies the collateral witness rules only to transactions that run phase-2 scripts, fixing a node that stopped following the chain when a transaction declared unnecessary collateral.

The fix extends beyond the original key-locked rule: the total_collateral balance check is also gated, and redeemers found in sub-transaction witness sets count, so Dijkstra transactions cannot bypass the rules.

Details

• Gates the rules on the presence of redeemers rather than Plutus scripts in the witness set, so reference-input scripts are still covered.
• Counts redeemers in sub-transaction witness sets too, so Dijkstra transactions can't bypass the rule.
• Adds a regression test covering both accepted and rejected cases, resolving the rules from the production slice so the tests exercise what the node actually runs.

^{Written for commit aac1220. Summary will update on new commits.}

[coderabbitai]

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 7711ab8f-45aa-4e7a-aa67-c132daf78d61

📥 Commits

Reviewing files that changed from the base of the PR and between 88145ad and aac1220.

📒 Files selected for processing (5)

• ledger/babbage/collateral_phase2_test.go
• ledger/babbage/rules.go
• ledger/babbage/rules_test.go
• ledger/common/witness.go
• ledger/conway/rules_test.go

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chrisguiney]

Acceptance on the real chain.

The wedged node was restarted on its own data directory, so it had only to get past the block that stopped it:

committed tip 15148378 -> 15184583
collateral rejections 0   (was 114)

Block 15148509 now applies and the node is following the chain again.

That confirms the change resolves the observed divergence. It does not confirm the condition is the one cardano-ledger uses — the caveat in the description stands, and a reviewer who knows that code should check it against the reference rather than against this block.

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[chrisguiney]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[wolf31o2]

Answering the "what I have not confirmed" section: the reading is right, and it is redeemers.

cardano-ledger Alonzo.Rules.Utxo.feesOK, Part 2:

unless (null $ tx ^. witsTxL . rdmrsTxWitsL . unRedeemersL) $
  validateCollateral pp txBody utxoCollateral

Babbage's feesOK gates validateTotalCollateral with the identical guard, and Conway inherits Babbage. The gate is a non-empty redeemer map, not the presence of Plutus scripts in the witness set -- so gating on redeemers is exactly what the reference does, and the reference-input reasoning in the description is the right reason for it.

One remaining member of the same class inline. Branch is on fab2538, four commits behind main.

[wolf31o2]

Two notes on the class this belongs to.

One member is still ungated. In the reference, validateCollateral / validateTotalCollateral is a group, and all of it sits behind the redeemer guard. gouroboros already gates three of them on len(WsRedeemers.Redeemers) == 0 -- UtxoValidateInsufficientCollateral, UtxoValidateCollateralContainsNonAda, UtxoValidateNoCollateralInputs, in both alonzo and babbage. babbage.UtxoValidateCollateralEqBalance is not gated: it returns early only when TotalCollateral() is nil or zero. But it is Part 6 of validateTotalCollateral, inside the same guard. A transaction with declared collateral, a total_collateral field and no redeemers is still held to it, which is the same false-rejection shape this PR is closing. Suspected, not observed on chain -- but it is the one place left where the class is incomplete.

babbage.UtxoValidateTooManyCollateralInputs is correctly outside the guard: validateTooManyCollateralInputs is called from the UTXO transition, not from feesOK. No change wanted there.

The guard now covers two different rules. This helper does more than validateScriptsNotPaidUTxO, which only tests vKeyLocked. It also requires a matching vkey witness for each collateral input, and that requirement comes from UTXOW's witsVKeyNeeded (Alonzo includes collateral inputs), which is not redeemer-gated in the reference. So skipping it here is a missed rejection rather than a false one -- the safe direction, and not worth blocking on, but worth knowing the guard is broader than the reference's.

[chrisguiney]

Both correct. Fixed in 5252120.

The ungated member. babbage.UtxoValidateCollateralEqBalance now returns early on !common.TransactionRunsPhase2Scripts(tx) (ledger/babbage/rules.go:653), matching the guard the other three already carry.

It uses the interface-level helper rather than tmpTx.WitnessSet.WsRedeemers deliberately: Conway and Dijkstra both delegate into this one function, and the helper counts sub-transaction redeemers, so Dijkstra gets the same answer its own collateral rules get from redeemerCount(tx).

I audited the class per era rather than assuming the copies agree:

era	CollateralEqBalance	TooManyCollateralInputs
alonzo	not defined, not wired — no total_collateral field in the Alonzo body	not defined
babbage	was ungated, now gated	ungated, untouched
conway	thin delegate to babbage — inherits the gate	ungated, untouched
dijkstra	wires conway.UtxoValidateCollateralEqBalance — inherits the gate	ungated, untouched

So babbage was the only place the rule has a body, and the other two eras route through it. The other three members were already gated in all four eras (alonzo/babbage on len(...Redeemers) == 0, conway on WsRedeemers.Len() == 0, dijkstra on redeemerCount(tx) == 0).

UtxoValidateTooManyCollateralInputs is unchanged in every era — the diff does not mention it.

Two existing fixtures had to change: TestUtxoValidateCollateralEqBalance in both babbage and conway built transactions with no redeemers, so under the new gate they would have passed while testing nothing. Each gained a redeemer.

The broader guard. Agreed, and it is now written down rather than left implicit — see the doc comment on ValidateCollateralVKeyWitnesses in ledger/common/witness.go. It names both halves (validateScriptsNotPaidUTxO, inside the guard; UTXOW's witsVKeyNeeded, not gated in the reference), states that gating both makes it a missed rejection rather than a false one, and gives the reason that matters — it can only accept something the reference rejects, so it cannot wedge a node on a canonical block. It also records that splitting the witsVKeyNeeded half back out to run ungated is the correct end state, left out of scope here.

Also rebased onto current main (88145ad).

[wolf31o2]

Accept-case, reject-case and the sub-transaction direction are all here, and the accept-case fails without the change -- this is the right shape. Two smaller points: it drives babbage.UtxoValidateCollateralVKeyWitnesses directly rather than resolving it from babbage.UtxoValidationRules, so nothing pins that the rule is still in the production slice; and the fixture transaction has no TxTotalCollateral, so it would not notice the UtxoValidateCollateralEqBalance gap noted on witness.go.

[chrisguiney]

Both fixed in aac1220.

Production wiring is now pinned. The cases resolve the rule out of babbage.UtxoValidationRules by function identity and fail if it is not there, instead of calling the exported function directly:

func productionRule(t *testing.T, name string, want common.UtxoValidationRuleFunc) common.UtxoValidationRuleFunc {
	wantId := reflect.ValueOf(want).Pointer()
	for _, rule := range babbage.UtxoValidationRules {
		if reflect.ValueOf(rule).Pointer() == wantId {
			return rule
		}
	}
	t.Fatalf("%s is not wired into babbage.UtxoValidationRules", name)
	return nil
}

The TxTotalCollateral gap is covered. New TestCollateralEqBalanceOnlyForPhase2 uses a fixture that sets TxTotalCollateral to 1 ADA against a 100 ADA collateral input with no collateral return — a genuine mismatch, so the rule rejects it whenever it actually runs. Three cases: no redeemers accepts, redeemers reject, and redeemers only in a sub-transaction reject.

Each case was proven to fail without its fix. The mutations:

1. Delete the redeemer guard from babbage.UtxoValidateCollateralEqBalance →
   TestCollateralEqBalanceOnlyForPhase2/no_phase-2_scripts fails with incorrect total collateral field: provided 100000000, total collateral 1000000. This is the case that would have caught the gap you noted.
2. Remove UtxoValidateCollateralVKeyWitnesses and UtxoValidateCollateralEqBalance from UtxoValidationRules (leaving the functions defined) →
   both tests fail with ... is not wired into babbage.UtxoValidationRules. Under the old shape they both still passed, which is exactly the hole.
3. Make TransactionRunsPhase2Scripts read only the top-level witness set →
   the sub-transaction case of both tests fails.

Full suite green and make lint at 0 issues on aac1220.

[chrisguiney]

Addressed @wolf31o2's review. Head is now aac1220, rebased onto current main (88145ad).

The ungated member of the class. babbage.UtxoValidateCollateralEqBalance now sits behind the redeemer guard, as Part 6 of validateTotalCollateral inside feesOK. It used the interface-level helper rather than the Babbage-typed witness set, because Conway and Dijkstra delegate into this one function and the helper also counts sub-transaction redeemers.

Audited per era rather than assuming the copies agree — this family is duplicated and has drifted before:

era	CollateralEqBalance	result
alonzo	not defined, not wired (no total_collateral in the Alonzo body)	nothing to do
babbage	had a body, ungated	gated
conway	thin delegate to babbage	inherits the gate
dijkstra	wires conway.UtxoValidateCollateralEqBalance	inherits the gate

The other three members were already gated in all four eras. UtxoValidateTooManyCollateralInputs is untouched everywhere — it is called from the UTXO transition, not feesOK.

The broader guard, documented. No behaviour change. The doc comment on ValidateCollateralVKeyWitnesses now records that the helper covers both validateScriptsNotPaidUTxO (inside the guard) and UTXOW's witsVKeyNeeded (not redeemer-gated in the reference), so gating both is a missed rejection rather than a false one — it can only accept something the reference rejects, never reject a canonical block. It also notes that splitting the witsVKeyNeeded half back out is the correct end state, out of scope here.

Tests. The phase-2 cases now resolve the rule out of babbage.UtxoValidationRules by function identity and fail if it is not wired, so dropping a rule from the production slice can no longer pass. New TestCollateralEqBalanceOnlyForPhase2 uses a fixture with TxTotalCollateral set to a genuine mismatch (1 ADA declared against a 100 ADA collateral input, no collateral return), covering accept / reject / sub-transaction.

Each new case was proven to fail without its fix:

1. Delete the guard from UtxoValidateCollateralEqBalance → the accept case fails with incorrect total collateral field: provided 100000000, total collateral 1000000.
2. Remove the two rules from UtxoValidationRules, leaving the functions defined → both tests fail with ... is not wired into babbage.UtxoValidationRules; under the previous shape both still passed.
3. Reduce TransactionRunsPhase2Scripts to the top-level witness set → the sub-transaction case of both tests fails.

Two pre-existing fixtures changed: TestUtxoValidateCollateralEqBalance in babbage and conway built transactions with no redeemers, so under the new gate they would have passed while testing nothing. Each gained a redeemer.

Validation on aac1220: go test ./... -count=1 passes (39 packages, exit 0); make lint reports 0 issues across the root module and all nine example modules, with a fresh GOLANGCI_LINT_CACHE.

Not re-run: the Preview replay from the earlier acceptance comment. That evidence still covers the key-locked rule, which is unchanged here. The CollateralEqBalance gate is the suspected-not-observed case @wolf31o2 flagged, and it has unit coverage only — I have no chain evidence of a transaction with total_collateral, declared collateral and no redeemers.

Cubic's sub-transaction finding was re-verified against this head and remains fixed; replied on the thread.

[wolf31o2]

Non-blocking, for a follow-up issue: this paragraph checks out against cardano-ledger master.

getAlonzoWitsVKeyNeeded is getShelleyWitsVKeyNeeded certState utxo txBody \Set.union` reqSignerHashes, and Shelley's inputAuthorsfolds overspendableInputsTxBodyF, which Alonzo defines as allInputsTxBodyF = inputs `Set.union` collateralInputs (eras/alonzo/impl/src/Cardano/Ledger/Alonzo/TxBody.hs:243-248). That set feeds UTXOW, not feesOK`, so the reference does require a vkey witness per key-locked collateral input regardless of redeemers.

So the gap is real: after this change a no-redeemer transaction with an unwitnessed key-locked collateral input is accepted here. Safe direction, and gouroboros does not check spend-input authors at all today, so the asymmetry is narrower than it looks. Worth an issue so the split does not stay only in this comment.

[chrisguiney]

Filed as #2224, with your cardano-ledger derivation recorded — getAlonzoWitsVKeyNeeded unioning reqSignerHashes onto getShelleyWitsVKeyNeeded, and inputAuthors folding over Alonzo's allInputsTxBodyF = inputs ∪ collateralInputs, feeding UTXOW rather than feesOK.

It states the consequence as you framed it: a missed rejection rather than a false one, and narrower than it looks because gouroboros does not check spend-input authors at all today, so collateral is not uniquely unchecked.

Agreed that it should not live only in a comment. The helper's doc comment now records the current state and points at the split as the end state; the issue carries the reference derivation and the fix.