ci: verify MAST roots on patch releases - #3305
Conversation
bdedf8b to
6ce653c
Compare
|
@huitseeker @mmagician - could you take a look at this PR? |
mmagician
left a comment
There was a problem hiding this comment.
LGTM, happy to merge as-is once we integrate agglayer checks, and we can optimally think about the versioning follow up later.
| edition = "2024" | ||
|
|
||
| [dependencies] | ||
| miden-protocol-current = { package = "miden-protocol", path = "../crates/miden-protocol" } |
There was a problem hiding this comment.
We should also check for roots from the agglayer crate
| miden-standards-current = { package = "miden-standards", path = "../crates/miden-standards" } | ||
|
|
||
| # The release wrapper rewrites these tags to the latest release tag on main. | ||
| miden-protocol-previous = { package = "miden-protocol", git = "https://github.com/0xMiden/protocol", tag = "v0.15.4" } |
There was a problem hiding this comment.
So this requires manually updating the script each time to use the correct comparison version, right?
Ok for now but ideally we'd either read it from Cargo.toml or something similar
There was a problem hiding this comment.
No manual updates are actually needed: the script is only run through check-masm-root-stability.sh which finds the latest release tag and rewrites the tag = "..." entries in a temp copy before running it. The tag in the script is actually a placeholder.
| pub fn collect_roots() -> Roots { | ||
| let mut roots = Roots::new(); | ||
|
|
||
| let protocol = ProtocolLib::default(); |
There was a problem hiding this comment.
ProtocolLib::default() covers assets/protocol.masl, but the kernel roots are generated separately from tx_kernel.masl into TransactionKernel::PROCEDURES.
A patch release could change a $kernel public procedure root and this check would still pass.
| let standards = StandardsLib::default(); | ||
| collect_library(standards.as_ref(), &mut roots); | ||
|
|
||
| collect_library(&agglayer_library(), &mut roots); |
There was a problem hiding this comment.
This covers the aggregate AggLayer library, but not the bridge/faucet component wrappers or the account code commitments generated from them.
A patch could change BRIDGE_CODE_COMMITMENT or FAUCET_CODE_COMMITMENT by changing the wrapper exports or companion component set while all agglayer_library() roots stay the same.
| ./scripts/check-msrv.sh | ||
|
|
||
| - name: Check MAST root stability | ||
| if: ${{ inputs.mode == 'dry-run' && github.ref == 'refs/heads/main' }} |
There was a problem hiding this comment.
This only runs for dry-runs on main, so the new check is skipped by workspace-publish.yml (mode: publish) and by dry-run pushes to release/**.
That leaves a path where a patch release from a release branch, or a direct published release, can publish without this MAST root gate.
Should this condition key off the release line check inside check-masm-root-stability.sh instead of github.ref == 'refs/heads/main'? If worried about this check blocking release, you can add an escape hatch similar to 0xMiden/miden-vm@c10619f
There was a problem hiding this comment.
Good catch. I changed it so the step now runs in both modes on every ref, and the gating is done by the script. It skips itself when there is no baseline tag or when the version change is a patch release.
On the escape hatch: unlike miden-vm, here the publish workflow only triggers on release: published, so there is no dispatch where we can set the input. So, instead, I added a check on a SKIP_MASM_ROOT_CHECK repository variable, so it can be set to true to skip the check if needed.
|
@huitseeker - could you take a look through this again? |
| fn run() -> Result<(), String> { | ||
| let previous = previous::collect_roots(); | ||
| let current = current::collect_roots(); | ||
| compare_roots(previous, current) |
There was a problem hiding this comment.
I think we also want to compare the kernel commitment, which does not directly get covered by the library because of the dynamic invocation.
cf51d1f to
f21d9ce
Compare
igamigo
left a comment
There was a problem hiding this comment.
Looks good! I think the only remaining points I have are:
- Would be great to smoke test various scenarios with this somehow, but it seems like it could be hard to do self-contained. Maybe we can open a PR to test out different sequences (bumping patch version and changing methods, etc).
- I think this PR will not enable additions, even when they might make sense. For example, there might be good reasons to backport a new standard or something like that, and I think some of these changes would not allow that (unless I'm mistaken). Is this the case? And is there something we want to do about it?
Addressed this on 832f454. Now the check does not fail if a new procedure is added, is only shown as a warning. However, adding a procedure to any account component or to the tx kernel would fail since the set of procedures is also checked as a whole. |
igamigo
left a comment
There was a problem hiding this comment.
LGTM! I think the current state covers most cases. I do wonder if we should really add all procedure roots of an account component and also the commitment (the commitment should be enough). Maybe in terms of code organization it would be worth separating every scenario and filter accordingly: backporting standards should be allowed, adding procedures to kernel/account code/note scripts should be breaking, anything else should remain stable.
I also still think we should ideally test this so let's create an issue or see if we can reproduce this in a discard PR
|
I haven't reviewed this in detail yet - but would be good to document the exact logic we are using here (maybe in a Markdown file somewhere). |
Added a Also rebased this PR to |
This PR adds a step to the
workspace-releaseaction to validate that the MAST roots are the same as the ones present on the previous release. The check is skipped on major/minor updates, only runs on patch releases. It will run on pushes to main or on a release publish.It collects the root from the public procedures from the standards and protocol libraries and the account components. This can change to use the packages but they are yet to be added in the current release on
main.Context: #3224 (comment)