Introduce `StoreReferences` and `ContentAddressWithReferences` #3746

Ericson2314 · 2020-06-25T05:49:50Z

Change itself

`StoreReferences`

This formalizes references where we don't necessarily know what we are called out, so self references are tracked with a bool not a StorePath in the set.

This is used with FixedOutputInfo (part of StorePathDescriptor) and internally the building logic.

`ContentAddressWithReferences`

This is useful to make any sort of content-addressed store path, whether it has references or not. It also makes initializing ValidPathInfo easier, by making sure the path and ca fields are in sync.

This is a sum type, not a pair of ContentAddress and References, because different types of content-addressing as different rules about what sorts of references are allowed.

Motivation

Constructing `ValidPathInfo`s

The chief motivation today is constructing ValidPathInfo. Today, for content-addressed paths, we have to:

Compute the path from some CA info.
Set that path in the ValidPathInfo
Set a slightly different CA in the ValidPathInfo
Set references in the ValidPathInfo

It is easy for those steps where the same information must be provided again and again to drift out of sync.

With this change there is one data type for all content-addressed info (hashes and reference) that one can create with nice designated initializers (AKA explicit field names) and then use that to construct the ValidPathInfo all at once. This keeps everything in sync.

Unblocking future feature work

Derivations can output "text-hashed" data #3959 leverages this to support text-hashed derivations. This is a key part of accepted RFC 92 Tracking issue for RFC 92: Dynamic derivations #6316.
Use StorePathDescriptor to fetching from CA-only stores #3754 leverages this to allow fetching from stores which only support content addressing. This was done as part of the IPFS / git hashing work, and something that we would hopefully return to with [RFC 0133] Git hashing and Git-hashing-based remote stores rfcs#133.

Deduplicating the `addToStore` methods

We currently have many method variations to add store objects to a store. In #4030 @roberth deduplicated this some in a new revision of the remote store protocol. With this + #3959, we can do something similar to the Store interface itself, cutting down on boilerplate.

Context

More philosophically, the idea we are inching towards is that ValidPathInfo and NarInfo are better as interfaces types (for talking to the SQLite DB and binary cache stores, respectively) than core data model types. For the CA world, StorePathDescriptor + Realisations is much better, as we are separating the trust free parts (what the data is) from the trustful parts (realisations, signatures etc.)

I think that even the older input-addressed code can be reformulated along these lines, but rest assured I would not attempt to relegate ValidPathInfo and NarInfo to the boundaries until I am confident I know how not make the input-addressed case worse.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

Further refinements to the content addressing data types are made in #3959, so it might be good to check that for reference.

roberth

I suppose this might be a little bit nicer representation in cases where you're special-casing the self reference, but we now have on the order of 40 call sites that are copying the references set, regardless of whether a self-reference is present. I've underestimated the weight of references before, so perhaps I'm a bit too wary. There's generally an order of magnitude more of those than store paths though.
If you could find more cases where the self-reference can/should be omitted when reading, that would help a lot. If this PR helps a lot with clarity it's also a win.

Found some surface level things, but this is not a proper review :)

roberth · 2020-10-07T17:31:50Z

src/libstore/path-info.hh

+    ValidPathInfo(const Store & store,
+        StorePathDescriptor && ca, Hash narHash);
+


It'd make more sense to me if this was a protected method on Store. I'm not 100% what the Valid in ValidPathInfo should mean, but regardless, I'd expect the bigger object of the two (Store) to manipulate smaller data object (ValidPathInfo), not the other way around.

So Store is here only used to get the store dir to compute a path from this info. "Valid" should probably just be dropped from the name, there are no special invariants here beyond what meets the eye (e.g. if something says "nar hash", it should be a hash of some nar, etc.).

#6236 would factor out a data type for "just" these store path functions, precisely so we can make clearer in the types for situations like this when some powerful IO might be done vs just a little more configure information is needed.

src/libstore/content-address.hh

Ericson2314 · 2020-10-08T03:06:06Z

I suppose this might be a little bit nicer representation in cases where you're special-casing the self reference, but we now have on the order of 40 call sites that are copying the references set, regardless of whether a self-reference is present. I've underestimated the weight of references before, so perhaps I'm a bit too wary. There's generally an order of magnitude more of those than store paths though.

That makes sense. I'm happy to just std::shared_ptr everything.

Ericson2314 · 2020-10-13T04:16:22Z

@roberth upon further thought, I think all the referencesToSelf stuff is just used at interface boundaries where there is already some O(n) traversal, so this at last no worse than a constant factor slowdown. Does that sound right to you?

Also (as I've amended the PR description to say), consider checking out #3959 where further change the content address types. If those changes look good to you, I might back-port them to this PR so that one can just be about the derivations.

I'm happy to make the constructor change and rename fields, but I think I want to confirm with @edolstra before I spend the time making those sorts of final touches.

roberth · 2020-10-13T07:21:36Z

... constant factor slowdown. Does that sound right to you?

It seems to improve code quality, so I don't think it has to be an issue. It's not I/O so I think we have some constant factor budget to spend. I'll assume most copies are short-lived as well, so memory footprint shouldn't be an issue, and if I assume wrongly, they were already copied before this PR, because that's the only way to retain them.

This reverts commit ee9eb83.

Ericson2314 · 2023-02-28T17:44:47Z

The thing I'm not clear on, however, is the point of the change. It's adding a lot of accidental complexity, and I don't see the advantages. Care to elaborate a bit on that?

Sure.

First of all, does the part about trying to initialize a ValidPathInfo in one go make sense? That is the most "self-contained" motivation.

As to the other part, a problem I see today is that the fixed output hashing and text hashing uses very separate code paths despite being basically two instances of the same idea:

separate functions to construct store paths
separate functions to add to store

Meanwhile, we a good bit of duplicated code in make-content-addressed.cc and local-derivation-goal.cc, as you observed two years ago.

Finally, my RFCs, without further cleanups, would make this problem worse:

RFC 92 means the local-derivation-goal.cc stuff must also support text hashing.
RFC 133 adds git hashing, which means instead of 2 similar things we now have 3, and even more duplication if we stay the current course.

This RFC doesn't fix these latter problems by itself --- it is just added more data structures. But i think in doing so it sets us up to.

Derivations can output "text-hashed" data #3959 allowing text hashing in more places becomes quite simple.
I would continue what @roberth started in Overhaul wopAddToStore #4030, getting rid of addTextToStore (or at least making it a trivial non-virtual wrapper).

The simplest thing to do might be just looking at #3959 to see the extant instance of this stuff being put to use.

Also improve content-address.hh API docs.

We weren't because this ancient PR predated it! This is actually a new version of the pattern which addresses some issues identified in NixOS#7479.

src/libstore/binary-cache-store.cc

src/libstore/local-store.cc

src/nix/profile.cc

Co-authored-by: Robert Hensing <[email protected]>

roberth · 2023-04-17T13:27:51Z

src/libstore/store-api.hh

roberth · 2023-04-17T13:28:54Z

src/nix/profile.cc

+                    .others = std::move(references),
+                    .self = false,
+                },
+            },


This is pre-existing tech debt.

roberth

Independently glad that you make these code improvements and that we're making a good step towards dynamic derivations, which is crucial for the sustainability of Nixpkgs.

nixos-discourse · 2023-04-18T07:19:34Z

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

https://discourse.nixos.org/t/2023-04-17-nix-team-meeting-minutes-49/27379/1

nixos-discourse · 2023-04-20T02:48:24Z

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

https://discourse.nixos.org/t/rfc-92-status-update/27441/1

nixos-discourse · 2023-04-20T02:49:29Z

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

https://discourse.nixos.org/t/rfc-92-status-update/27441/1

Ericson2314 force-pushed the path-info branch from 10da8e0 to 48c4cd0 Compare June 25, 2020 15:38

meditans force-pushed the path-info branch 2 times, most recently from 48c4cd0 to d785c1a Compare June 25, 2020 17:46

Ericson2314 added 2 commits June 25, 2020 17:49

Pull out PathReferences super class

1722ae6

WIP: store separate hasValidPath bool

71e4c9c

Ericson2314 force-pushed the path-info branch from d785c1a to 71e4c9c Compare June 25, 2020 17:59

Ericson2314 changed the title ~~Super WIP: More datatype changes -- contains #3689~~ WIP: ValidPathInfo distinguishes references from self-references Jun 25, 2020

Ericson2314 added 3 commits June 29, 2020 17:59

Backport fix from NixOS#3754 branch

a9c0ea3

Fix some things in remote store

70ed47c

Fix nar info parsing

6683406

Ericson2314 changed the title ~~WIP: ValidPathInfo distinguishes references from self-references~~ ValidPathInfo distinguishes references from self-references Jun 29, 2020

Ericson2314 added 6 commits August 1, 2020 00:06

Merge remote-tracking branch 'upstream/master' into path-info

0e9e599

Merge remote-tracking branch 'upstream/master' into path-info

36dd792

Remove stray tabs

e61061c

Merge remote-tracking branch 'upstream/master' into path-info

45ca7c3

Merge remote-tracking branch 'upstream/master' into path-info

dae4409

Use PathReferences more widely

f8d562c

Ericson2314 changed the title ~~ValidPathInfo distinguishes references from self-references~~ PathReferences and StorePathDescriptor Oct 7, 2020

Ericson2314 changed the title ~~PathReferences and StorePathDescriptor~~ Introduce PathReferences and StorePathDescriptor Oct 7, 2020

roberth reviewed Oct 7, 2020

View reviewed changes

Ericson2314 force-pushed the path-info branch from 3845df9 to f8d562c Compare October 8, 2020 04:07

Ericson2314 added 2 commits October 12, 2020 20:48

Merge remote-tracking branch 'upstream/master' into path-info

a0f369a

Organize content-address.hh a bit better

39c11c5

Ericson2314 force-pushed the path-info branch from 120c67b to 39c11c5 Compare October 13, 2020 03:43

Ericson2314 added 2 commits October 13, 2020 15:36

Merge remote-tracking branch 'upstream/master' into path-info

34b3bfa

Fix conditions for ca-references

10e81bf

Ericson2314 mentioned this pull request Dec 14, 2020

Trustful IPFS Store #3727

Draft

Ericson2314 added 3 commits February 28, 2023 11:49

Clarify store path grammar and improve comment on makeType

123b11f

Revert "Remove some designated initializers"

85bb865

This reverts commit ee9eb83.

No inheritance for TextInfo and FixedOutputInfo

d381248

Ericson2314 mentioned this pull request Mar 10, 2023

Feature request: Add support for golang HashDir/h1/sha256 in fixed output derivations. #8021

Open

fricklerhandwerk added the RFC Related to an accepted RFC label Mar 13, 2023

thufschmitt self-requested a review March 30, 2023 19:48

Ericson2314 added 5 commits March 30, 2023 16:28

Merge remote-tracking branch 'upstream/master' into path-info

aa99005

Also improve content-address.hh API docs.

Fix warning

a6d00a7

Use "raw pattern" for content address types

c51d554

We weren't because this ancient PR predated it! This is actually a new version of the pattern which addresses some issues identified in NixOS#7479.

Add comparison methods for content addresses

5d56e2d

Merge remote-tracking branch 'upstream/master' into path-info

fd21f9d

roberth reviewed Apr 17, 2023

View reviewed changes

src/libstore/binary-cache-store.cc Show resolved Hide resolved

roberth reviewed Apr 17, 2023

View reviewed changes

src/libstore/binary-cache-store.cc Show resolved Hide resolved

Ericson2314 commented Apr 17, 2023

View reviewed changes

src/libstore/local-store.cc Show resolved Hide resolved

Ericson2314 commented Apr 17, 2023

View reviewed changes

src/nix/profile.cc Show resolved Hide resolved

Explain various .self = false,

537e871

Co-authored-by: Robert Hensing <[email protected]>

github-actions bot added fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store labels Apr 17, 2023

roberth reviewed Apr 17, 2023

View reviewed changes

src/libstore/store-api.hh

Copy link

Member

roberth Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

roberth reviewed Apr 17, 2023

View reviewed changes

src/nix/profile.cc

.others = std::move(references),

.self = false,

},

},

Copy link

Member

roberth Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre-existing tech debt.

roberth approved these changes Apr 17, 2023

View reviewed changes

roberth enabled auto-merge April 17, 2023 13:31

roberth merged commit e641de0 into NixOS:master Apr 17, 2023

Ericson2314 deleted the path-info branch April 17, 2023 14:00

Ericson2314 mentioned this pull request Jul 5, 2023

Simplify ContentAddress #8650

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `StoreReferences` and `ContentAddressWithReferences` #3746

Introduce `StoreReferences` and `ContentAddressWithReferences` #3746

Ericson2314 commented Jun 25, 2020 •

edited

Loading

roberth left a comment

roberth Oct 7, 2020

Ericson2314 Jan 6, 2023

Ericson2314 commented Oct 8, 2020

Ericson2314 commented Oct 13, 2020

roberth commented Oct 13, 2020

Ericson2314 commented Feb 28, 2023

roberth Apr 17, 2023

roberth Apr 17, 2023

roberth left a comment

nixos-discourse commented Apr 18, 2023

nixos-discourse commented Apr 20, 2023

nixos-discourse commented Apr 20, 2023

		ValidPathInfo(const Store & store,
		StorePathDescriptor && ca, Hash narHash);

Introduce StoreReferences and ContentAddressWithReferences #3746

Introduce StoreReferences and ContentAddressWithReferences #3746

Conversation

Ericson2314 commented Jun 25, 2020 • edited Loading

Change itself

StoreReferences

ContentAddressWithReferences

Motivation

Constructing ValidPathInfos

Unblocking future feature work

Deduplicating the addToStore methods

Context

Checklist for maintainers

roberth left a comment

Choose a reason for hiding this comment

roberth Oct 7, 2020

Choose a reason for hiding this comment

Ericson2314 Jan 6, 2023

Choose a reason for hiding this comment

Ericson2314 commented Oct 8, 2020

Ericson2314 commented Oct 13, 2020

roberth commented Oct 13, 2020

Ericson2314 commented Feb 28, 2023

roberth Apr 17, 2023

Choose a reason for hiding this comment

roberth Apr 17, 2023

Choose a reason for hiding this comment

roberth left a comment

Choose a reason for hiding this comment

nixos-discourse commented Apr 18, 2023

nixos-discourse commented Apr 20, 2023

nixos-discourse commented Apr 20, 2023

Introduce `StoreReferences` and `ContentAddressWithReferences` #3746

Introduce `StoreReferences` and `ContentAddressWithReferences` #3746

Ericson2314 commented Jun 25, 2020 •

edited

Loading

`StoreReferences`

`ContentAddressWithReferences`

Constructing `ValidPathInfo`s

Deduplicating the `addToStore` methods