Make builtins.fetchClosure non-experimental

[manveru]

Motivation

builtins.fetchClosure has been around for quite some time already, and is used by various people without any issues.

I think this should finally be removed from the list of experimental features.

Context

This should be a rather trivial change. Pinging @roberth for review.

Priorities

Add 👍 to pull requests you find important.

[Ericson2314]

I am not sure I am quite ready for this. I would like to "deeply nar hash" input-addresed closures with a single hash.

[roberth]

I am not sure I am quite ready for this. I would like to "deeply nar hash" input-addresed closures with a single hash.

That seems like an extension more than a breaking change. Could you explain the problem that "deeply" hashing would solve?

[Ericson2314]

AFIAK the input addressing vs content addressing check is only on the root store object, we have no guarantees about what it references, and what its contents might be.

As I understand it, the whole business about adding trusted substituters around this was because we were uncomfortable with this stuff in pure eval mode. I think we are now more OK with it, but then I don't really see why listing substituters is associated with this feature: it is generally useful to hint at where things can be obtained, and that should have nothing to do with this or any sort of security story.

Basically I think the current fetchClosure does a decent attempt given the current status quo, but the status quo is not good that this means that many orthogonal concerns are jumbled together in one had to understand feature.

Instead what I would like to see is something like:

1. Whether or not the store objects are input-addressed or content-addressed, there are measures that we can take to be sure what they and their closure contain.
2. builtins.storePath is the only way to create store objects by path, we can make a new record-taking version for more info (e.g. deep/recursive nar hash).
3. A new builtins.hintSubstituters Can hint on any string with context where the store objects it refers to might be gotten. Nix can intelligently show/hide messages based on whether current settings allow said thing to be substituted.

[roberth]

Regarding factoring out a (string context) feature

I don't think the existence of builtins.fetchClosure is harmful to @Ericson2314's objectives.
Given the limited resources and other projects, I do not think developing the machinery you propose is worth prioritizing over other developments like completing RFC 92 and the other "stabilization" or indeed factoring out features in other places such as those that underpin flakes.

The risk of stabilizing this feature is very small, considering the small surface area, and I think we should be careful about picking our battles. If we're going to factor out features, we better do the big ones first, and I wouldn't take this one hostage until that's done.

We have other cases where some builtins aren't necessary anymore, and these builtins have provided massive value to users that allows us to move things forward today.

• derivation (wrapper of better building block derivationStrict)
• filterSource (special case of path)
• fetchTarball, fetchGit, etc (instances of fetchTree, in progress)
• getAttr (iirc the syntax for it was added later)
• hasAttr
• import

Regarding CA

we have no guarantees about what it references, and what its contents might be.

I believe we do actually guarantee that the contents are CA all the way down if the caller doesn't allow input addressing.
We wouldn't even be able to add it to the store otherwise, unless the user is trusted.

If you have a certain scenario in mind where more such enforcement is needed, please elaborate why that should be a blocker.

Otherwise, I'm rather skeptical that we should block this on a hypothetical requirement from a hypothetical CA-using community. That feature is after all still experimental, largely undocumented and painful to use because it's not in cache.nixos.org.

[roberth]

Is this the process for stabilizing a feature? I think we should silently ignore it until well after, because they're harmless and that way users can use mixed Nix versions with the same config without getting annoyed with non actionable warnings.

I guess we did it this way in #8322

Maybe we could have a list of ignored features so that we don't warn for those?

[thufschmitt]

I don't think we did it in #8322, but that seems to be a great idea.
I think we can just keep the experimental flag and mark it as deprecated with no effect. Or have an extra field specifying that it should be enabled by default

[manveru]

I wasn't sure about it, but reverted that part for now, it really doesn't feel nice to give people a warning for this case.
I'm unsure how deprecating works for experimental features though.

[Ericson2314]

I believe we do actually guarantee that the contents are CA all the way down if the caller doesn't allow input addressing.
We wouldn't even be able to add it to the store otherwise, unless the user is trusted.

From a quick look, runFetchClosureWithContentAddressedPath only checks the root object after the fact. So if the keys happens to be trusted and the transitive paths are input-addressed, it will just work. This can naturally lead to some hard-to-debug "why did it stop working?" situations.

I am very open to the idea that I am picking an unnecessary battle :). But I don't think the main thing I mentioned, the "deep nar hashing" would be too hard either?

[thufschmitt]

I agree with @roberth, let us not have the best be the enemy of the good. This is a valuable feature for a number of people and we shouldn't delay it forever because we could possibly make it better at some point in the future

[Ericson2314]

To flip it around, if builtins.fetchClosure with inputAddressed = true; works with pure eval, then I don't see why builtins.storePath cannot also work with pure eval. At that point, is anyone blocked on this --- does anyone actually need the substituter-specifiying part? Or was it just a hoop peopled jumped through to write code that worked work with flakes?

Stepping back a bit, this primop is currently 3 things in 1 (for understandable reasons, don't get me wrong). Which way(s) do people actually use it?

[Ericson2314]

Otherwise, I'm rather skeptical that we should block this on a hypothetical requirement from a hypothetical CA-using community. That feature is after all still experimental, largely undocumented and painful to use because it's not in cache.nixos.org.

Just to be clear, I very much don't mean to be saying anything like "just wait for content-addressed derivations". I think this whole business of downloading proprietary/black-box software is more complex than it needs to be because we haven't decided on what store objects actually are, but conversely anything we do decide on has to include input-addressed ones too.

Put another way, I think that asking people to do no input-addressing any time soon is unrealistic --- even in a CA by default world, there will be things for which store path rewriting doesn't work, and thus we'll need some predetermined (whether input addressed or randomized, hardly matters) store paths. The "deep NAR hash" is basically admitting we need to be able to refer to entire closure of block-box store objects whether those are content-addressed, input-addressed, or a mix; that we can't wait for everywhere content-addressed store paths to save us.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-15-nix-team-meeting-minutes-86/33171/1

[Ericson2314]

After talking to @roberth more, #5868 (comment) this is my preferred solution --- I think builtins.storePath is good enough, and far simpler, and allowing users to use it will take the pressure off stabilizing builtins.fetchClosure before I think that one is ready.

[tomberek]

From Nix Team discussion:

• this is a known problem with the current implementation
• disagreement about making builtins.storePath pure
• recent changes the fetchClosure API (see latest changes), we should give people a chance to review
• not a current priority for the team compared to nix-cli + flakes
• needs additional design regard deeply hashing NARs (@Ericson2314)
• fetching from a Nix cache fills a need similar to fetchTree, but from substituter, not a repository
• fetching from a Nix cache does not track dependencies as well
  • When "inputAddressed = true" there are very few guarantees
  • CA promises could be better, but currently do not include the references, this means that a fake/incorrect narinfo would allow you to depend on more or less things than you should
• most of team is inclined to merge,

Remaining hesitancy and questsions

• can this feature create a situation where libraries or frameworks depend on it in such a way that users end up with problems that are hard for them to diagnose and fix?
• danger to become an anti-feature?
• makes store URLs a part of the language
• because this can/will be used to not expose the original derivation, it makes debugging issues harder; obtaining input-addressed store paths from mixed sources means non-deterministic output is harder to diagnose
• needs CA paths to be checked if it is deeply CA

[manveru]

Just wanted to say that we've been using this function for months now in https://github.com/input-output-hk/capkgs and it significantly speeds up our development and deployment.
While I agree that it's not an ideal solution because of the before mentioned concerns, right now there is no better way we found to work around extremely lengthy evaluation times especially for haskell.nix related derivations that often involve IFD.

[Ericson2314]

#11919 finally wrote up the design for "deep NAR hashing". The idea is since one of these hashes can verify a closure, regardless of whether it contains any input-addressed stuff, it is a solution for making fetchClosure deterministic for input-addressed store objects.