Enable compiling modules without library paths

[Fumuran]

This PR makes it possible to compile Module with no LibraryPath provided. As a result of this NamedProcedure now can be created without id.

Tasks:

• Add tests
• Update changelog

[bobbinth]

I think I've realized why we can't use LibraryPath::anon_path() as the default library path for modules without library paths. The issue is that if we compile two separate modules using the same context, we will get ID collisions if the modules contain procedures with similar names. For example, let's say we want to compile u64 and u128 modules. Both may have add procedures. These procedures would be different, but they would end up with the same ID: anon::add.

[Fumuran]

Is it generally correct situation when we have two modules without Library Path which contain functions with the same names? How user is supposed to use and even distinguish these functions?
Current implementation won't allow to compile two anon modules — compilation will fail here, so it should be changed anyway.
Maybe we can add some kind of module index to the anon library path to make them distinguishable. Something like anon1::add and anon2::add for u64 and u128 in your example. Or something like anon-<first four chars in AccountCode hash>::add.

[bobbinth]

Is it generally correct situation when we have two modules without Library Path which contain functions with the same names? How user is supposed to use and even distinguish these functions?

We can call them using procedure MAST roots (see here). This is actually how they will be called in the context of the rollup.

Maybe we can add some kind of module index to the anon library path to make them distinguishable. Something like anon1::add and anon2::add for u64 and u128 in your example. Or something like anon-<first four chars in AccountCode hash>::add.

I don't think that's needed. As long as two procedures have different MAST roots, we can call them using call.0x12345... instruction.

[Fumuran]

Ok, so we won't have problems with their calls: it will be handled by their unique hashes.

Now we will have error in the circular module dependency check since we have #anon or even None Library path, but we can just exclude modules with anon path from this check.

But I don't understand why we will have ID collisions: if we have module without Library path, the ID of the local procedures will be None and we will add to the cache only their root and Procedure here (to the procedures map in the ProcedureCache here). And then we push these procedures to the proc_cache in the Assembler here, but even if we will push the procedure with the None ID and the name that is already in the cache, the procedure field inside it will be different, so, if I understood it right, we won't have a collision here.
The only theoretical place where a problem could occur is in the ensure_procedure_is_in_cache function call, but it is used only in the exec_imported and call_imported functions, so if user will call functions from modules without path only by procedure's hash, these functions will never be used.

[bobbinth]

Maybe it'll work as you described - let me look into this in more detail later today. For now, could we add a test which complies two different anon modules which have procedures with the same name but different implementation?

[bobbinth]

Looks good! Thank you! I left a few minor comments inline.

Overall, I think this approach could work, but I would like to see how the approach with removing id from NamedProcedure would look like. I have a feeling that it might be cleaner but I might be wrong. Could you try to implement it in a separate commit?

[bobbinth]

@Overcastan - could you also see if it would make sense to adopt some tests mentioned in 0xPolygonMiden/miden-base#235 (comment)? I wonder if we still have this bug somewhere.

[Fumuran]

could you also see if it would make sense to adopt some tests mentioned in 0xPolygonMiden/miden-base#235 (comment)? I wonder if we still have this bug somewhere.

I tried to reproduce the bug with cache collision, but I didn't manage to do it - everything worked as it should. I can look at this problem in more detail, but it seems to me that in this case it is better to create a separate issue.

[bobbinth]

Looks good! Thank you! I left a couple of small comments inline. Once these are addressed, we can merge.

I do prefer this version as I think it is a bit cleaner - but to be honest, they are not that different. So, if you strongly prefer the previous version, we can revert.

[bobbinth]

We should also update the comment above.

[bobbinth]

very minor nit: I'd probably remove the blank line between these two lines.

[Fumuran]

I don't have a strong preference — at first I thought that moving out the id field would make code more complicated, but in the end it turned out to be a clean solution. So l'd leave this version.