-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement baggage #4873
Implement baggage #4873
Conversation
Something is very messed up here git-wise. Somehow this PR has sucked in a bunch of commits that should already be part of the main branch and there are a lot of spurious changes in the list of changed files. I think I'll need some help sorting this out before this is actually ready for review. |
707417f
to
e4358bd
Compare
Git madness appears to have been resolved. Review away! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let me know what you think about the two recommended changes: if they're impossible to pull off, I'm ok with landing without them, but I'd like to see if we could do it.
@@ -1183,7 +1184,12 @@ function build( | |||
); | |||
|
|||
// here we finally invoke the vat code, and get back the root object | |||
const rootObject = buildRootObject(harden(vpow), harden(vatParameters)); | |||
baggage = collectionManager.provideBaggage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should call provideBaggage
earlier, just after the m.unserialize(vatParametersCapData)
, for two reasons:
- it won't be metered, just in case that somehow makes a difference
- the baggage
vref
is allocated before userspace gets control, so it should be pretty stable, which I think is going to make debugging a lot easier (we don't need to checkbaggageID
before interpreting DB dumps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately put the creation of the baggage immediately before the invocation of buildRootObject
so that I could be sure, without having to think too deeply about it, that the environment was properly set up (as it would have to be since you need to be able to create collections inside buildRootObject
). This was mostly a labor saving move on my part, since the choice of where to do this seemed otherwise relatively arbitrary. A quick check says moving it to where you suggest shouldn't be a problem, aside from the inevitable rejiggering of some of the tests due to the resulting change in order of DB accesses.
]); | ||
const syscall = ['vatstoreGetAfter', priorKey, lowerBound]; | ||
if (upperBound) { | ||
syscall.push(upperBound); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I can imagine what prompted this, but I'd prefer to avoid variable-length syscall objects (e.g. VatSyscallVatstoreGetAfter
is always a length-4 array, with the last property being of variable type, rather than sometimes it's length-3 and sometimes it's length-4). Was the problem resolved by the djson
fix you made? Or could we resolve it by doing something like if (!upperBound) { syscall[3] = null; }
or something?
I know we have a general problem (#4390) with null-vs-undefined as we convert through both the kernel/worker pipe and to/from the transcript on the vat-manager side, and this local issue is a consequence of that technical debt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with dsjon
was something I tripped over but wasn't ultimately the problem (though it was real). Requiring the syscall array to be fixed length is very doable, but the practical effect would simply be to move this bit of conditional logic from the place the syscall array is constructed to the place where it is used, so I'm not sure that would be a win.
91cb337
to
415529e
Compare
This implements the primary vat-side interface for upgrade (#4325, #4382). However, it does not yet implement the set of post upgrade checks (#3062) that will be required (e.g., check upon return from `buildRootObject` to verify that all the durable kinds have had behavior associated with them or have been explicitly dismissed); the ability to test or otherwise exercise the latter portion of the upgrade machinery will have to wait until the kernel-side interfaces (#1848, #3062) are in place. I am going to tentative declarely that this PR closes #4325, leaving the remaining work to be covered by #3062.
415529e
to
990e7d8
Compare
This implements the primary vat-side interface for upgrade (#4325, #4382). However, it does not yet implement the set of post upgrade checks (#3062) that will be required (e.g., check upon return from
buildRootObject
to verify that all the durable kinds have had behavior associated with them or have been explicitly dismissed); the ability to test or otherwise exercise the latter portion of the upgrade machinery will have to wait until the kernel-side interfaces (#1848, #3062) are in place.I am going to tentatively declare that this PR closes #4325, leaving the remaining work to be covered by #3062.
Note to reviewers: the actual baggage implementation per se consists of the (relatively small) changes to
collectionManager.js
andliveslots.js
. The bulk of the rather large number of file changes in this PR are test enhancements or fixes, though implementation testing did surface a couple of other problems that are also addressed herein:It turns out that you can't use the vatstore at all from vats running with the
nodeWorker
ornode-subprocess
worker types due to how the syscall communications channel between the worker and the kernel works. This was a known limitation of these worker types, but since the baggage mechanism requires the vatstore in all cases, this limitation renders these vat types essentially unusable. Fortunately they are basically unused except for four tests which mostly just verify their existence; these tests have been disabled until we decide to either eliminate these vat worker types or make significant changes to how they communicate with the kernel process.There was a problem with parameter encoding for
vatstoreGetAfter
that introduced a mismatch in some cases between how it was encoded for kernel<->worker communication and how it was encoded for transcript replay, which caused spurious anachrophobia errors on replay. This has been addressed by tweaks todjson.js
andsupervisor-helper.js
.