Fix of external SDK crate override bug by Sdoba16 · Pull Request #312 · BlockstreamResearch/SimplicityHL

Sdoba16 · 2026-05-05T10:42:37Z

Closes #310
Previously, users could interact directly with the DependencyMap API in SimplicityHL, they could manually set the crate keyword to any path they want. This happened because the insert method allowed adding the crate alias without throwing any validation errors.

Fix
Was introduced DependencyMapBuilder which collects dependencies and then could with build command it can be built.

While building are made following validations:

entry source must be canonical and non-anonymous
every dependency target must be a canonical directory
every context prefix must be a canonical directory
alias must be a valid dependency identifier
alias must not be keyword
duplicate context+alias remappings rejected
overlapping crate roots are explicitly modeled
resolved files must remain inside their package root

Remapping was made pub(crate) to hide from external API.

resolution.rs was separated into 2 files source.rs and resolution.rs for readability.

apoelstra · 2026-05-06T15:10:54Z

In 3390c42:

I'm confused by this solution. We now how have two methods, DependencyMap::insert and DependencyMap::add_crate_root which differ only in that one is private and one does a check for "crate". Weirdly, the private one does the check but the public one doesn't.

In parallel to this, this commit adds TryFrom<Vec<Remapping>> for DependencyMap which is insufficiently general (it should be FromIterator rather than TryFrom<Vec> IMO, given that all it does is iterate over its input and call self.insert) and does do the check.

We then change main to construct a Vec which we then call TryFrom on, instead of just directly constructing the DependencyMap. What does this gain us?

KyrylR · 2026-05-08T09:18:18Z

@Sdoba16 could you update the description to reflect recent updates?

apoelstra · 2026-05-11T13:29:05Z

Can you squash these commits?

LesterEvSe · 2026-05-11T13:40:59Z

@@ -0,0 +1,106 @@
+use std::path::Path;


Once this file is added, let's move the CanonSourceFile struct and its implementation from driver/mod.rs into this new one

LesterEvSe · 2026-05-11T13:43:54Z

            K: Into<String>,
        {
-            let mut dependency_map = DependencyMap::new();
+            let mut builder = crate::resolution::DependencyMapBuilder::new();


Use DependencyMapBuilder instead of crate::resolution::DependencyMapBuilder

LesterEvSe · 2026-05-11T13:46:15Z

+        let canon_root = crate::source::CanonPath::canonicalize(&root).unwrap();
+        let dependency_map = crate::resolution::DependencyMapBuilder::new()


Same here: use the name directly and remove the crate:: prefix

LesterEvSe · 2026-05-11T13:52:20Z

-        dependency_map.insert(context, drp.into(), target).unwrap();
-
-        dependency_map
+        crate::resolution::DependencyMapBuilder::new()


Found crate again. Let’s nix these everywhere else too

LesterEvSe · 2026-05-11T13:59:03Z

+/// A router for resolving dependencies across multi-file workspaces.
+///
+/// Mappings are strictly sorted by the longest `context_prefix` match.
+/// This mathematical guarantee ensures that if multiple nested directories
+/// define the same dependency root path, the most specific (deepest) context wins.
+#[derive(Debug, Clone, Default)]
+pub struct DependencyMap {
+    /// External dependency remappings (e.g., `use math::...`)
+    remappings: Vec<Remapping>,
+    /// Package roots for resolving `crate::...` (sorted by longest path match)
+    package_roots: Vec<CanonPath>,
 }


Add a bit more explanation of the architecture here. Why do we need Vec<CanonPath> here?

LesterEvSe · 2026-05-11T14:01:08Z

+    pub fn with_entry_root(mut self, root: CanonPath) -> Self {
+        self.entry_root = Some(root);
+        self


Let’s pull this into new() and get rid of the Option for entry_root

LesterEvSe · 2026-05-11T14:03:32Z

                let resolved = Self::build_and_verify_path(&remapping.target, &parts[1..])
                    .map_err(|failed_path| {
-                        let err = if drp_name == CRATE_STR {
-                            Error::FileNotFound(failed_path)
-                        } else {
-                            Error::ExternalFileNotFound(drp_name.to_string(), failed_path)
-                        };
-                        RichError::new(err, *use_decl.span())
+                        RichError::new(
+                            Error::ExternalFileNotFound(drp_name.to_string(), failed_path),
+                            *use_decl.span(),
+                        )
                    })?;

-                self.check_local_file_imported_as_external(
-                    drp_name,
-                    current_file,
-                    &resolved,
-                    use_decl,
-                )?;
+                if !resolved.starts_with(&remapping.target) {
+                    return Err(RichError::new(
+                        Error::ExternalFileNotFound(
+                            drp_name.to_string(),
+                            resolved.as_path().to_path_buf(),
+                        ),
+                        *use_decl.span(),
+                    ));
+                }
+
+                self.check_local_file_imported_as_external(current_file, &resolved, use_decl)?;


Extract this into a separate function

stringhandler · 2026-05-13T11:46:07Z

-        self.as_path().starts_with(path.as_path())
-    }
+        // Sort package roots by length descending to ensure longest prefix match
+        crate_roots.sort_by(|a, b| {


Should this not have a then_by sort for paths with the same length? It could probably then be used instead of the above sort at line 138

stringhandler · 2026-05-13T11:47:44Z

-}
-
 impl DependencyMap {
    pub fn new() -> Self {


Should this method maybe be private and add documentation to only use the builder. It seems like it is quite complicated to create this struct

stringhandler · 2026-05-13T11:48:19Z

    /// This mathematically guarantees that the first match we find is the most specific.
    fn sort_mappings(&mut self) {
-        self.inner.sort_by(|a, b| {
+        self.remappings.sort_by(|a, b| {


Also feels like there should be a second sorting clause for paths with the same length

LesterEvSe · 2026-05-15T15:29:10Z

ACK 7ff8fd2; tested locally

apoelstra

ACK 7ff8fd2; successfully ran local tests

KyrylR · 2026-05-21T07:06:30Z

+    DependencyPathNotFound(String),
+    DependencyNotADirectory(String),
+    ReservedDependencyKeyword(String),
+    DuplicateDependencyAlias(String, String),
+    InvalidDependencyIdentifier(String),
    Internal(String),


In the context of #325 (comment) should we reiterate on errors here?

@gerau what do you think? (from my PoV with this PR we are adding more work for you)

This PR is already ACK'ed and tested. My vote is for merging as is.

I'm okay with that

This PR is already ACK'ed and tested. My vote is for merging as is.

When possible I wanna avoid surprises for devs who are working on features

KyrylR · 2026-05-21T07:31:12Z

+/// This struct must always be constructed via [`DependencyMapBuilder`].
+/// Builder guarantees that the vector is never empty and contains no duplicates, so we can safely sort without worrying about edge cases.
+#[derive(Debug, Clone, Default)]
+pub struct DependencyMap {


This still derives Default, which leaves a public way to construct an invalid map without DependencyMapBuilder

Yeah, you're right, so I will just delete deriving Default, it won't break anything

KyrylR

ACK 70cebfb;

(waiting for local tests to finish before merging)

KyrylR · 2026-05-21T09:28:12Z

ACK 70cebfb;

(waiting for local tests to finish before merging)

The diff to 7ff8fd2 is minimal, therefore I believe there is no need for re review from others

KyrylR

ACK f2af871; successfully ran local tests, review

… and `TemplateProgram` in `new_with_dep` method cdaab4d Change SourceFile to CanonSourceFile in CompiledProgram and TemplateProgram to forbid passing SourceFile::anonymous (Sdoba16) Pull request description: Closes #311 Oblige user to use `CanonSourceFile` in the `CompiledProgram` and `TemplateProgram` in `new_with_dep` method to prevent passing `SourceFile::anonymous`. This PR should be merged after the #312 to prevent conflicts as it follows its implementation. ACKs for top commit: LesterEvSe: ACK cdaab4d; tested locally apoelstra: ACK cdaab4d; successfully ran local tests Tree-SHA512: 592ffa99a93851f280eed18c0a4819ef9148097d7f915ea37e0f30bb5fc14147c1fc15c1bc32c9ef7e0e4f3505bacff89df7cbbc2cefeab25cc5f6cd36c0e5b2

Sdoba16 requested review from KyrylR and LesterEvSe May 5, 2026 10:42

Sdoba16 marked this pull request as ready for review May 5, 2026 10:49

Sdoba16 requested a review from delta1 as a code owner May 5, 2026 10:49

Sdoba16 self-assigned this May 5, 2026

KyrylR mentioned this pull request May 6, 2026

Bug: Users can override the crate keyword as a DRP name #310

Closed

Sdoba16 mentioned this pull request May 7, 2026

Require to use CanonSourceFile in the CompiledProgram and TemplateProgram in new_with_dep method #315

Merged

Sdoba16 force-pushed the fix/crate-override branch from 4414d2d to 438d6ff Compare May 11, 2026 13:38

LesterEvSe requested changes May 11, 2026

View reviewed changes

Sdoba16 force-pushed the fix/crate-override branch 2 times, most recently from 6ed86cb to 1836312 Compare May 13, 2026 10:33

stringhandler reviewed May 13, 2026

View reviewed changes

Sdoba16 force-pushed the fix/crate-override branch 2 times, most recently from 5be4511 to 7ff8fd2 Compare May 15, 2026 15:22

LesterEvSe approved these changes May 15, 2026

View reviewed changes

apoelstra approved these changes May 20, 2026

View reviewed changes

KyrylR reviewed May 21, 2026

View reviewed changes

Comment thread src/lib.rs

KyrylR reviewed May 21, 2026

View reviewed changes

Sdoba16 force-pushed the fix/crate-override branch from 7ff8fd2 to 70cebfb Compare May 21, 2026 09:21

KyrylR approved these changes May 21, 2026

View reviewed changes

Added DependencyMapBuilder for safe public API usage

f2af871

Sdoba16 force-pushed the fix/crate-override branch from 70cebfb to f2af871 Compare May 21, 2026 09:35

KyrylR approved these changes May 21, 2026

View reviewed changes

KyrylR merged commit f3168af into BlockstreamResearch:master May 21, 2026
11 checks passed

		let canon_root = crate::source::CanonPath::canonicalize(&root).unwrap();
		let dependency_map = crate::resolution::DependencyMapBuilder::new()

Conversation

Sdoba16 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented May 6, 2026

Uh oh!

KyrylR commented May 8, 2026

Uh oh!

apoelstra commented May 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LesterEvSe commented May 15, 2026

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KyrylR left a comment

Choose a reason for hiding this comment

Uh oh!

KyrylR commented May 21, 2026

Uh oh!

KyrylR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sdoba16 commented May 5, 2026 •

edited

Loading