fix(swingset): update lmdb #5488

mhofman · 2022-06-01T05:57:42Z

closes: #5031
best reviewed commit-by-commit

Description

Switches from node-lmdb to lmbd-js which seem more actively maintained. Disable useWritemap option which seem to be causing some issues (but not the only cause of them), as it seems no longer necessary.

This PR does the minimal amount of code changes in the swingstore by synchronous creating a transaction. This works with the new API because we already ensured that we only ever had one transaction at a time. Once inside a transaction, read after writes are guaranteed to contain the data from the transaction.

However to adapt to the more asynchronous nature of lmdb-js, this PR updates the API of the swingstore to make commit and close asynchronous. While closing a synchronous transaction is not actually asynchronous, it would be too much of a hack to make it happen synchronously. Since the committers were already all asynchronous, this was a low impact change.

lmdb-js also seem to have a couple differences in behavior, including not passing through the mapSize option or encoding strings (both keys and values) as UTF-8. The former will result in a non preallocated DB file (previously sparse) growing in size with usage. The latter has no practical impact since the LMDB key size was also increased.

Drive by fix of getKeys iterator interruption which were previously not closing cursors. I've decided to pass errors through, as these really should not happen.

Security Considerations

The max key size logic and checks in collections is now no longer matching reality as it assumed a UTF-16 encoding of strings. However since lmdb-js bumps the max key size, it is impossible to generate keys that are too large, and a new test asserts this.

Documentation Considerations

The collections documentation was amended.

Testing Considerations

Fixed some swingstore and swingset tests to handle async commit, and handle parallel execution.
Added a test for lmdb max key size.
Ran a full daily loadgen test on my machine, which used to crash reliably.

michaelfig · 2022-06-01T16:58:18Z

packages/SwingSet/docs/run-policy.md

@@ -88,7 +88,7 @@ while(1) {
  processInboundIO();
  const policy = make100CrankPolicy();
  await controller.run(policy);
-  commit();
+  await commit();


Please also update callers of commit() in packages/solo/src and packages/cosmic-swingset/src.

They were already awaiting!

michaelfig · 2022-06-01T17:06:52Z

Here's what I see missing await:

agoric-sdk/packages/solo/src/start.js

Line 251 in 01cdbc9

commit();

agoric-sdk/packages/solo/src/reset-state.js

Line 14 in 3c7e57b

commit();

mhofman · 2022-06-01T18:06:53Z

Here's what I see missing await:

agoric-sdk/packages/solo/src/start.js

Line 251 in 01cdbc9

commit();

That one is a JSONStore, which is a different API. I'm open to make it apparent async as well for consistency.

agoric-sdk/packages/solo/src/reset-state.js

Line 14 in 3c7e57b

commit();

This one has already been updated:

agoric-sdk/packages/solo/src/reset-state.js

Line 14 in 35dc1da

await commit();

mhofman · 2022-06-02T03:37:34Z

That one is a JSONStore, which is a different API. I'm open to make it apparent async as well for consistency.

@michaelfig I did make it consistent since it didn't really hurt. The only somewhat similar API is now the chain-store, but that one seem to rely on a synchronous commit right now, and I didn't feel like untangling that.

patches/ordered-binary+1.2.5.patch

warner

Seems good to me, but please get @FUDCo 's approval too, he knows the DB layer much better than me.

packages/cosmic-swingset/src/entrypoint.js

packages/solo/src/entrypoint.js

packages/solo/src/pipe-entrypoint.js

packages/swing-store/test/test-state.js

warner · 2022-06-04T21:39:11Z

packages/SwingSet/src/kernel/state/storageWrapper.js

-    } else {
-      const result = v2.value;
-      v2 = it2.next();
+  /** @type {IteratorResult<any> | null} */


Please coordinate with @gibson042 since his #5294 moves this function around.

warner · 2022-06-04T21:46:17Z

packages/access-token/src/json-store.js

@@ -198,7 +198,7 @@ function makeJSONStore(dirPath, forceReset = false) {
  /**
   * Commit unsaved changes.
   */
-  function commit() {
+  async function commit() {


Does this need to be async? This little "json store" doesn't overlap much with the SwingStore, but I suppose if there's any chance of both appearing in the same program, I can see a consistency argument.

Yeah I wasn't sure. Can easily revert this specific commit.

I guess I'm ambivalent.

packages/swing-store/src/swingStore.js

warner · 2022-06-04T22:10:08Z

packages/swing-store/test/test-state.js

+  t.teardown(cleanup);
+  t.is(isSwingStore(dbDir), false);
+  const store = initSwingStore(dbDir);
+  store.kvStore.set('€'.repeat(254), 'Money!');


Let's assert both sides of the size limit (i.e. assert that 255 3-byte-UTF-8 chars fails), since the success/failure of writes needs to be part of consensus. This isn't an exact constraint (we'd need a mix of encoded sizes, plus a healthy variety of surrogate pairs, to exercise this thoroughly), but I think it's good to be in the habit of knowing what the precise rules are.

I think that means this DB change is consensus-breaking (there are non-trivial unicode strings which would pass the earlier encoding scheme's limit but not the new one, or vice versa), but I'm ok with making that now.. better than later.

255 3-byte-UTF-8 chars fails

It won't fail. The upper limit is actually 1971 bytes I believe.

I think that means this DB change is consensus-breaking

I don't believe so. Nothing is writing large keys since the only user controlled key size is the collections keys which is independently checked.

Ah, ok, so the previous limits were (pardon my js/python pseudocode) str.length < X && len(str.encode('utf-16')) < Y, and the new limits are str.length < X && len(str.encode('utf-8') < Z, but for all possible strings, the .length < X limit (imposed by liveslots) is tighter than the encoded limit (imposed lower down). So landing this can't make any failing userspace behavior start to succeed, or succeeding userspace behavior start to fail.

As long as that criteria is met, I guess we don't need more stringent testing at this layer.

Yes, this test is specifically about checking this assumption. That the worst case str = threeByteUtfChar.repeat(X) then len(str.encode('utf-8')) < Z. Aka 3 * X < Z

packages/swingset-runner/src/kerneldump-entrypoint.js

FUDCo

One minor stylistic quibble but aside from that this looks very good. I'm surprised (and happy) that the changes in the underlying LMDB API could be accommodated with such a light touch.

FUDCo · 2022-06-06T18:33:03Z

packages/swing-store/src/swingStore.js

-    cursor.close();
+    /** @type {import("lmdb").RangeOptions} */
+    const rangeOptions = {};
+    if (start) rangeOptions.start = start;


I'm not sure why our linter didn't pick this up, but please write this as

if (start) { rangeOptions.start = start; }

-- always use braces
-- no putting the if consequence on same line as the conditional

Consistent API with swing-store

mhofman added the force:integration Force integration tests to run on PR label Jun 1, 2022

michaelfig reviewed Jun 1, 2022

View reviewed changes

mhofman force-pushed the mhofman/5031-update-lmdb branch 2 times, most recently from bcca078 to 8edac53 Compare June 2, 2022 03:21

mhofman requested review from FUDCo and warner June 2, 2022 03:35

mhofman marked this pull request as ready for review June 2, 2022 03:36

mhofman force-pushed the mhofman/5031-update-lmdb branch from 8edac53 to 35e40e3 Compare June 2, 2022 03:42

mhofman commented Jun 2, 2022

View reviewed changes

patches/ordered-binary+1.2.5.patch Outdated Show resolved Hide resolved

warner approved these changes Jun 4, 2022

View reviewed changes

FUDCo approved these changes Jun 6, 2022

View reviewed changes

mhofman added 14 commits June 6, 2022 21:22

fix(swingset): hasher types

03a27e4

fix(swing-store): IterableIterator types

48836dd

fix(swingset): handle getKeys iterator interruption

7e1cef8

refactor(swing-store): async commit and close

f9e84c7

fix(swing-store): fix tests to use proper temp dbs

e899f5c

feat(swing-store): Switch to lmdb-js

89adc87

fix(swing-store): disable useWritemap

691a4bd

refactor(access-token): make JSON store commit async

1d458ed

Consistent API with swing-store

fix(swingset): close store iterators when clearing them

fc4d3e9

fix(telemetry): close store iterator on has check

6199337

fix(swingset): Propagate iterator close errors

3bd1821

chore(swing-store): Add test for max key size

89b036b

chore(swingset): clarify collection max key size

167ee40

fix(swingset): avoid relying on dependency patch

b14cdab

mhofman force-pushed the mhofman/5031-update-lmdb branch from fa931fe to b14cdab Compare June 6, 2022 21:22

mhofman added the automerge:rebase Automatically rebase updates, then merge label Jun 6, 2022

mhofman removed the force:integration Force integration tests to run on PR label Jun 6, 2022

mergify bot merged commit 168da21 into master Jun 6, 2022

mergify bot deleted the mhofman/5031-update-lmdb branch June 6, 2022 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(swingset): update lmdb #5488

fix(swingset): update lmdb #5488

mhofman commented Jun 1, 2022 •

edited

Loading

michaelfig Jun 1, 2022

mhofman Jun 1, 2022

michaelfig commented Jun 1, 2022

mhofman commented Jun 1, 2022

mhofman commented Jun 2, 2022

warner left a comment

warner Jun 4, 2022

warner Jun 4, 2022

mhofman Jun 4, 2022

warner Jun 6, 2022

warner Jun 4, 2022

mhofman Jun 4, 2022

warner Jun 6, 2022

mhofman Jun 6, 2022

FUDCo left a comment

FUDCo Jun 6, 2022

fix(swingset): update lmdb #5488

fix(swingset): update lmdb #5488

Conversation

mhofman commented Jun 1, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Jun 1, 2022

mhofman commented Jun 1, 2022

mhofman commented Jun 2, 2022

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Jun 1, 2022 •

edited

Loading