Skip to content

fix: correct inverted native script timelock comparisons#778

Merged
scarmuega merged 2 commits into
mainfrom
fix/718-timelock-comparison
May 21, 2026
Merged

fix: correct inverted native script timelock comparisons#778
scarmuega merged 2 commits into
mainfrom
fix/718-timelock-comparison

Conversation

@scarmuega

@scarmuega scarmuega commented May 21, 2026

Copy link
Copy Markdown
Member

Summary

eval_native_script in pallas-validate/src/phase1/shelley_ma.rs evaluated both native-script timelock opcodes with inverted comparisons:

  • InvalidBefore(n) was satisfied when the transaction's validity interval started before slot n — it should be satisfied when it starts at or after n.
  • InvalidHereafter(n) was satisfied when the validity interval ended after slot n — it should be satisfied when it ends at or before n.

This matches evalTimelock in the Mary/Allegra ledger spec (mary-ledger.pdf p.20). The result was consensus-relevant validation that both accepts invalid transactions and rejects valid ones.

The issue reporter spotted InvalidBefore; InvalidHereafter was inverted the same way and is fixed here too. The None (unbounded) arms were already correct and are unchanged.

Changes

  • Flip the two comparison operators in eval_native_script.
  • Add a #[cfg(test)] mod tests exercising the previously untested timelock branches: invalid_before, invalid_hereafter (after / exactly-at / before the lock, plus the missing-bound case), and nested_timelocks (ScriptAll / ScriptAny recursion).

Verification

  • New unit tests: 3 passed
  • Existing shelley_ma integration tests: 25 passed — no regression

Fixes #718

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Corrected timelock validation semantics: validity-interval bounds for before/after checks now evaluate in the intended direction, affecting when native scripts are considered satisfied.
  • Tests

    • Added unit tests covering before/after bound checks and recursive propagation through combined timelock scripts to validate the updated behavior.

Review Change Stack

`eval_native_script` evaluated both timelock opcodes with inverted
comparisons. `InvalidBefore(n)` was satisfied when the validity
interval started before slot `n` (should be at or after), and
`InvalidHereafter(n)` when it ended after `n` (should be at or
before), per mary-ledger.pdf p.20.

Adds unit tests covering both opcodes and nested combinators.

Fixes #718

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f7af2664-2c99-4655-8190-84ad608c3703

📥 Commits

Reviewing files that changed from the base of the PR and between 6ee057b and 34a9f12.

📒 Files selected for processing (1)
  • pallas-validate/src/phase1/shelley_ma.rs

📝 Walkthrough

Walkthrough

This PR inverts the comparison logic for ShelleyMA native-script timelocks (InvalidBefore / InvalidHereafter) to match the spec and adds unit tests that exercise boundary and recursive composition behaviors.

Changes

ShelleyMA Timelock Semantics

Layer / File(s) Summary
Timelock validity predicates
pallas-validate/src/phase1/shelley_ma.rs
InvalidBefore(val) and InvalidHereafter(val) now check validity-interval bounds in the opposite direction, aligning with Shelley MA specification for native script evaluation.
Timelock validation tests
pallas-validate/src/phase1/shelley_ma.rs
Unit tests cover InvalidBefore and InvalidHereafter satisfaction at/inside/outside bounds, and validate bounds threading through ScriptAll and ScriptAny compositions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 A little rabbit hops with glee,
I flipped the bounds so scripts agree.
Timelocks now read right, no more fright,
Tests hop in to prove the light.
Cheers for correctness — nibble of delight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: correct inverted native script timelock comparisons' directly and clearly summarizes the main change: fixing inverted comparison logic in native script timelock evaluation.
Linked Issues check ✅ Passed The code changes directly address all objectives from issue #718: flipping comparison operators for InvalidBefore and InvalidHereafter, aligning with the Mary/Allegra ledger spec, and adding comprehensive unit tests.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the native script timelock evaluation logic and adding unit tests to validate the fix, with no unrelated modifications introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/718-timelock-comparison

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scarmuega scarmuega merged commit 3e1738e into main May 21, 2026
10 checks passed
@scarmuega scarmuega deleted the fix/718-timelock-comparison branch May 21, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Eval time lock - native script

1 participant