fix(script): encode validity upper bounds as exclusive - #2190
Conversation
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change removes era-dependent strict upper-bound handling. Merge Risk: ⚪ Minimal · up to Finite transaction validity upper bounds are now consistently encoded as exclusive across eras, with matching test expectations. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
chrisguiney
left a comment
chrisguiney
left a comment
There was a problem hiding this comment.
Thanks for picking this up — and for catching your own regression in a19d0de before anyone had to ask. That said, I do not think this PR should merge as it stands, and the reason is more about what it claims than about anything being broken.
The net production change is one whitespace character
Filtering comment lines out of the context.go diff leaves exactly this:
- strictUpperBound bool
+ strictUpperBound bool
The closure expression !t.lowerBoundPresent && !t.strictUpperBound is byte-identical to origin/main, so the title ("encode validity upper bounds as exclusive") and the body ("Fix TTL-only validity ranges to use exclusive upper bounds") describe a change that is not in the diff.
That is because the two commits cancel. ee76def removed the era gate outright — deleted eraIdConway, stubbed the era check to _ = tx; return true, hardcoded the closure to false, and reduced strictValidityUpperBound to an ignored _ bool. a19d0de then restored all of it, line for line. Cubic flagged the intermediate state at P1 and it was right: that version would have given Alonzo/Babbage TTL-only transactions an exclusive upper bound, diverging from transVITime's PV1.to for historical script validation — the same class of divergence #2160 describes, pointed the other way.
Closes #2160 does not hold
924de23 fix(ledger): era-gate ScriptContext validity upper bound is already on origin/main and is an ancestor of this branch, so the issue is fixed. Merging this would close #2160 against a commit that changed no behaviour and break the trail back to the commit that actually fixed it.
Verified good
- The era gate is still pinned. I mutated
eraIdConwayfrom 6 to 7 andTestValidityRangeUpperBoundByEra/Conwayfails, so that coverage survived the edit. go build ./...,go vet ./ledger/...,gofmt,golangci-lint run ./ledger/common/script/...(0 issues) andgo test ./...all pass.- No concurrency surface — pure value encoding, no goroutines or shared state — and no performance impact.
I also checked the two things that looked like they might be silent traps, and neither is: adding or renaming an era in the table fails loudly rather than passing with a wrong expectation, and a caller that omits the new variadic argument fails loudly on the pre-Conway cases. Both are maintainability points, not correctness ones, and I have marked them as such inline.
Suggested disposition
Closing this and letting 924de23 close #2160 is the cleanest option. If the comment volume is the real motivation — and some of those blocks genuinely were long — a re-scoped PR titled as a docs/test cleanup would be welcome, keeping the two facts called out inline, restoring strictUpperBound on the Conway test cases so the duplicated pairs test different things, and dropping the Closes line. Happy to review that quickly.
Process note
Cubic reviewed at 22:03Z, before a19d0de landed at 22:47Z, so it has not seen the head commit. Its one remaining open thread is now a false positive — the comment it flags reads correctly again after the revert — and CodeRabbit's green check says "Review skipped: incremental reviews are disabled".
| // era or later, false for Alonzo/Babbage. Plutus V3 only exists in Conway | ||
| // and later, so V3 contexts always set this true. | ||
| strictUpperBound bool | ||
| strictUpperBound bool |
There was a problem hiding this comment.
This is where the change lands, and it is worth being explicit about what it is: after filtering comments out of this file's diff, the only remaining line is this field's alignment. !t.lowerBoundPresent && !t.strictUpperBound below is unchanged from origin/main.
So what this file actually does is drop the ~30 lines that explained why the encoding is era-dependent. Trimming those was defensible — they were long — but two of the facts in them are the ones I would most want to keep:
- Why pre-Conway must stay inclusive. cardano-ledger#3043 could not change Alonzo/Babbage
transVITimebecause it would alter historical on-chain script validation, so the corrected exclusive bound was gated to Conway. Without that written down, the natural reading of this field is that the pre-Conway branch is a bug — which is exactly the changeee76defmade in this PR beforea19d0detook it back out. - Why the gate is on the era and not the Plutus version. A V1/V2-keyed gate looks equivalent and is not: it would wrongly apply pre-Conway behaviour to Conway-era V1/V2 scripts. That one is genuinely hard to rediscover.
Would you be open to keeping those two, in a shorter form? Something like three lines pointing at cardano-ledger#3043 and at transVITime vs transValidityInterval would carry the load without the current length.
| { | ||
| name: "unbounded - conway", | ||
| tr: TimeRange{strictUpperBound: true}, | ||
| tr: TimeRange{}, |
There was a problem hiding this comment.
These two cases are now byte-identical — same TimeRange{}, same want — and the same is true of "lower only (upper absent) - conway" and "- preconway" just above, because strictUpperBound was dropped from both Conway variants.
Each pair reads as covering two eras and covers one twice. That is a little worse than it looks: strictUpperBound is what makes these Conway cases Conway, so if the era-invariant shapes ever stop being era-invariant, nothing here would notice.
Either restoring strictUpperBound: true on the two Conway variants, or collapsing each pair into one case named for the shape rather than the era, would fix it. I would lean toward restoring the field, since these are exactly the cases that prove the gate does not affect the era-invariant shapes.
| // `!lowerBoundPresent` for every era, giving Conway-era TTL-only transactions | ||
| // an INCLUSIVE upper bound and mis-computing script execution units. | ||
| func TestTimeRangeToPlutusDataUpperBoundEraDependent(t *testing.T) { | ||
| // TestTimeRangeToPlutusDataUpperBound pins era-dependent upper-bound encoding. |
There was a problem hiding this comment.
The comment this replaces recorded the actual bug — that gouroboros previously emitted !lowerBoundPresent for every era, giving Conway-era TTL-only transactions an inclusive upper bound and mis-computing ExUnits — and it carried the era-vs-Plutus-version note I mentioned on context.go.
That text is the regression record for #2160. Removing it in the same PR that says Closes #2160 reads backwards: the issue's own value is largely in that explanation, and this is the place a future reader would look for it.
The rename from TestTimeRangeToPlutusDataUpperBoundEraDependent is a small loss too — the old name said which property the table pins, and ...UpperBound alone does not.
| start *uint64, | ||
| end *uint64, | ||
| upperBoundOnlyIsClosed bool, | ||
| upperClosed ...bool, |
There was a problem hiding this comment.
A required bool became variadic here. I checked whether that could go wrong silently and it cannot: both current callers pass the argument, and a caller that omitted it would get closed = false, which mismatches the pre-Conway upper-only expectation and fails loudly. So this is not a trap.
It does still trade a compile-time check for a runtime default in a helper whose entire job is to state the expected closure, and I could not find a caller that benefits from the argument being optional. If there is one I have missed, I would rather know — otherwise keeping the plain bool seems strictly better here.
| require.NoError(t, err) | ||
| strictUpperBound := build.name == "V3" || | ||
| !era.upperBoundOnlyIsClosed | ||
| era.name == "Conway" || era.name == "Dijkstra" |
There was a problem hiding this comment.
Minor, and I checked before raising it: this fails loudly rather than silently. Adding a post-Conway era to the table, or renaming an existing one, produces a mismatch against the production encoding and the test fails — so correctness is not at risk.
It is a maintainability step back from the upperBoundOnlyIsClosed column it replaces, though. The expectation now lives ~110 lines from the table that defines the eras, and each era row no longer states its own expected behaviour, so adding one means editing two places instead of filling in a column. The old shape also documented the property per era at the point where the era is introduced, which is the part I would miss most.
2 participants
Fix TTL-only validity ranges to use exclusive upper bounds.
Closes #2160
Tests:
go test ./ledger/common/script