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

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

Open

Sdoba16 mentioned this pull request May 7, 2026

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

Draft

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 from 1836312 to 5be4511 Compare May 15, 2026 10:38

Added DependencyMapBuilder for safe public API usage

7ff8fd2

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

LesterEvSe approved these changes May 15, 2026

View reviewed changes

		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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Sdoba16 commented May 5, 2026 •

edited

Loading