chore: Make Swingset use Nat 4.0.0 #2420

katelynsills · 2021-02-13T02:01:07Z

This PR changes Swingset to use Nat 4.0.0.

packages/SwingSet/src/natNum.js

erights · 2021-02-15T16:56:03Z

FUDCo

Mostly OK, but a couple of issues. (1) If we've updated our toolchain to cope with BigInt literals I'd prefer to use them for the numeric literals. (2) The getAllIds thing in the counter won't work at scale, as it is not iterating out of secondary storage. (1) is mainly cosmetic, but (2) needs to be fixed before this can land.

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

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

packages/SwingSet/src/devices/command.js

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

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

Chris-Hibbert

one question for my edification.

The changes to TimerService seem fine.

packages/SwingSet/src/devices/timer-src.js

packages/SwingSet/src/vats/vat-timerWrapper.js

warner · 2021-02-15T19:23:19Z

I haven't been able to look deeply at this yet (I'll keep trying), but my two cents so far:

fromStr, toStr, increment are great
I'd rather see Nat() instead of a new natNum(), and I agree with @erights that we don't need to spend any time retaining the ability for swingset to use floats instead of bigints
I don't understand the need for JSONstringify, the one place it's used should only be observing strings, never numbers, so something (possibly me) is confused
I'd much rather see 0n than ZERO
I'm suspicious of the kernelKeeper.js createStartingState changes, because someone who wants to know what the starting state includes must look in more than one place (no longer entirely contained inside createStartingKernelState()). I think I'd change const kpidNum = kpC.allocateNextID(); to something like const kpidNum = getNextID('kp.nextID');, using a shared function that closes over storage and getRequired, rather than a bunch of discrete functions that also close over the key name.
as @FUDCo pointed out, getAllIDs could be slightly more efficient, especially if the numberspace is sparse (i.e. most vats have been terminated, so their state has been deleted): instead of returning a full (giant) list and then filtering it out, we should do the filtering as we go, so we're only traversing the giant list once instead of twice. That said, we only use this for debugging and unit tests, for which the list is very small, and we wouldn't expect this to be efficient in production anyways (we would need a different kind of database to be able to iterate over all vats when there's a million of them)

katelynsills · 2021-02-15T23:00:01Z

This PR is erroring in cosmic-swingset due to the use of JSON.stringify there. The errors are:

  TypeError#7: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at JSON.stringify (packages/tame-metering/src/tame.js:183:20)
    at saveState (packages/cosmic-swingset/lib/ag-solo/start.js:151:21)
    at processKernel (packages/cosmic-swingset/lib/ag-solo/start.js:163:13)
    at async deliverInboundCommand (packages/cosmic-swingset/lib/ag-solo/start.js:193:7)

And

  TypeError#4: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at JSON.stringify (packages/tame-metering/src/tame.js:183:20)
    at stringify (packages/SwingSet/src/kernel/json-stable-stringify.js:64:19)
    at stringify (packages/SwingSet/src/kernel/json-stable-stringify.js:84:21)
    at stringify (packages/SwingSet/src/kernel/json-stable-stringify.js:84:21)
    at stableStringify (packages/SwingSet/src/kernel/json-stable-stringify.js:93:5)
    at Map.map.commit (packages/cosmic-swingset/lib/ag-solo/fake-chain.js:29:18)
    at saveChainState (packages/cosmic-swingset/lib/launch-chain.js:122:26)
    at blockManager (packages/cosmic-swingset/lib/block-manager.js:164:17)
    at async unqueuedSimulateBlock (packages/cosmic-swingset/lib/ag-solo/fake-chain.js:118:7)

Handing over to @warner to fix the cosmic-swingset issues.

katelynsills · 2021-02-15T23:37:44Z

There is another Swingset error when trying to run the Swingset tests in ERTP:

  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)

FUDCo · 2021-02-16T00:20:51Z

There is another Swingset error when trying to run the Swingset tests in ERTP:

  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)

This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.

erights · 2021-02-16T00:26:25Z

There is another Swingset error when trying to run the Swingset tests in ERTP:
  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)
This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.

Assuming that I keep my promise from the meeting this morning, and have a Marshal.stringify and Marshal.parse as alternatives to JSON.*, can the test just use those? Any reason not to?

FUDCo · 2021-02-16T00:28:52Z

There is another Swingset error when trying to run the Swingset tests in ERTP:
  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)
This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.
Assuming that I keep my promise from the meeting this morning, and have a Marshal.stringify and Marshal.parse as alternatives to JSON.*, can the test just use those? Any reason not to?

Will Marshal.stringify support the JSON.stringify replacer?

erights · 2021-02-16T00:30:13Z

There is another Swingset error when trying to run the Swingset tests in ERTP:
  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)
This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.
Assuming that I keep my promise from the meeting this morning, and have a Marshal.stringify and Marshal.parse as alternatives to JSON.*, can the test just use those? Any reason not to?
Will Marshal.stringify support the JSON.stringify replacer?

No. Does this test case use a replacer?

FUDCo · 2021-02-16T00:31:05Z

There is another Swingset error when trying to run the Swingset tests in ERTP:
  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)
This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.
Assuming that I keep my promise from the meeting this morning, and have a Marshal.stringify and Marshal.parse as alternatives to JSON.*, can the test just use those? Any reason not to?
Will Marshal.stringify support the JSON.stringify replacer?
No. Does this test case use a replacer?

Yes. In this case we're using JSON.stringify for legibility and don't care a whit about round-tripability. (In this case, the replacer is being used to truncate long strings)

erights · 2021-02-16T00:33:15Z

Earlier, did you mean replacer when you said reviver?

FUDCo · 2021-02-16T00:33:56Z

Earlier, did you mean replacer when you said reviver?

Yes. The code erroneously says reviver when it means replacer and I was unwittingly propagating the mistake.

erights · 2021-02-16T00:36:31Z

Earlier, did you mean replacer when you said reviver?

Yes. The code erroneously says reviver when it means replacer and I was unwittingly propagating the mistake.

Gotcha. That is indeed a good reason not a use a marshal-based stringify. So don't wait for me.

erights

LGTM

warner · 2021-02-16T19:29:20Z

The cosmic-swingset test failure looks due to the swingset Mailbox device using Nat to create the sequence numbers (and ack) in the outbound mailbox data. This mailbox data is then given to the host application for delivery to a peer. The host application knows more about it's own abilities to transfer data to a peer, so we make it responsible for packing the mailbox data into whatever form it can accomodate. Swingset (specifically the mailbox device) is responsible for making that data convenient to pack.

In the cosmic-swingset (chain) case, it is recorded in the chain's state vector, which is a key-value store whose values are strings (bytes, really). The "peer" is a client which queries the chain for this small portion of its state vector, verifies the inclusion proofs, then unpacks the string into data that can be submitted into another mailbox device. The host application (cosmic-swingset) uses JSON.stringify to do the packing, and the client (our ag-solo chain-follower library, in this case, although we should anticipate others) uses JSON.parse to do the unpacking.

In the (future) solo-to-solo comms protocol, this will run over an encrypted TCP socket, so we'll have the same need for a packed single-string representation of the mailbox data, and JSON.stringify would be an eminently reasonable thing to use there.

The mailbox data contains:

ackNum: a number representing the high-water mark of sequence numbers seen coming from the other side. It means "you are allowed to forget and never resend all messages whose seqnum is equal or lower than this".
A list of [seqnum, message] tuples, where seqnum is a positive integer, and message is a string. The messages are delivered in-order without gaps, and the outbound mailbox data will include a message until the receiving side acks it.

I don't think we need BigInts for these. Using a plain JSON Number limits the lifetime of a comms connection to 2^53 (which is fairly comfortably large for this purpose, we get max one comms message per crank, say 1ms/crank gives us like 300k years), but allows clients to use a plain JSON parser for receiving the mailbox data, which makes the spec a lot simpler.

So I'm inclined to think the solution is to change our packages/SwingSet/src/devices/mailbox.js to use Nat as an assertion (fail loudly if the seqnum or ack field is corrupted or getting within a factor of 2 of the ambiguity threshold), but then "downcast" to a Number before allowing the mailbox data to escape out to the host application.

warner · 2021-02-16T19:40:40Z

I'm working on a patch to make those mailbox changes now.

…nums Host applications call `exportMailbox()`, and are then responsible for conveying that data to a peer application. We can make their lives more convenient by returning something that can be serialized with a plain JSON.stringify. This changes `exportMailbox()` to emit plain Numbers for the sequence numbers, even though we store them internally as BigInts (validated with `Nat`). Likewise we accept Numbers from the host but convert them (with Nat) into BigInts for local processing. refs #2420

warner · 2021-02-16T20:02:44Z

That patch appears to fix the cosmic-swingset tests, but I still see non-fatal errors that seem to be coming from CapTP, so I'd like @michaelfig to weigh in on whether they're relevant or not:

CapTP test fixture exception: (Error#1)
Nested error
  Error#1: board does not have id: (a string)
     at fullRevive (packages/marshal/src/marshal.js:871:34)
    at unserialize (packages/marshal/src/marshal.js:925:19)
    at CTP_RETURN (packages/captp/lib/captp.js:329:16)
    at dispatch (packages/captp/lib/captp.js:388:9)
    at WebSocket.<anonymous> (packages/cosmic-swingset/test/captp-fixture.js:60:11)
    at WebSocket.emit (events.js:315:20)
    at WebSocket.EventEmitter.emit (domain.js:467:12)
    at Receiver.emit (events.js:315:20)
    at Receiver.EventEmitter.emit (domain.js:467:12)
    at Socket.emit (events.js:315:20)
    at Socket.EventEmitter.emit (domain.js:467:12)
  
  Error#1 ERROR_NOTE: Received as error:captp:unknown#1
Error#1 ERROR_NOTE: Rejection from: (Error#2) : 17 . 0
Nested error under Error#1
  Error#2: Event: 15.2
     at t.throwsAsync.message (packages/cosmic-swingset/test/test-home.js:36:20)
    at packages/cosmic-swingset/test/test-home.js:35:11
    at async Promise.all (index 1)
  
CapTP test fixture exception: (RangeError#3)
Nested error
  RangeError#3: id is probably a typo, cannot verify CRC: (a string)
     at fullRevive (packages/marshal/src/marshal.js:871:34)
    at unserialize (packages/marshal/src/marshal.js:925:19)
    at CTP_RETURN (packages/captp/lib/captp.js:329:16)
    at dispatch (packages/captp/lib/captp.js:388:9)
    at WebSocket.<anonymous> (packages/cosmic-swingset/test/captp-fixture.js:60:11)
    at WebSocket.emit (events.js:315:20)
    at WebSocket.EventEmitter.emit (domain.js:467:12)
    at Receiver.emit (events.js:315:20)
    at Receiver.EventEmitter.emit (domain.js:467:12)
    at Socket.emit (events.js:315:20)
    at Socket.EventEmitter.emit (domain.js:467:12)
  
  RangeError#3 ERROR_NOTE: Received as error:captp:unknown#2
RangeError#3 ERROR_NOTE: Rejection from: (Error#4) : 18 . 0
Nested error under RangeError#3
  Error#4: Event: 17.1
     at t.throwsAsync.message (packages/cosmic-swingset/test/test-home.js:41:20)
    at packages/cosmic-swingset/test/test-home.js:40:11

We landed #2429 (stop including crankNumber in transcripts) which should remove the need for the special bigintJSONStringify in src/kernel/state/vatKeeper.js once this is rebased onto current trunk. Once CI passes with that last patch, I'll do a rebase and remove bigintJSONStringify.

…nums Host applications call `exportMailbox()`, and are then responsible for conveying that data to a peer application. We can make their lives more convenient by returning something that can be serialized with a plain JSON.stringify. This changes `exportMailbox()` to emit plain Numbers for the sequence numbers, even though we store them internally as BigInts (validated with `Nat`). Likewise we accept Numbers from the host but convert them (with Nat) into BigInts for local processing. refs #2420

michaelfig · 2021-02-17T02:27:38Z

That patch appears to fix the cosmic-swingset tests, but I still see non-fatal errors that seem to be coming from CapTP, so I'd like @michaelfig to weigh in on whether they're relevant or not:

Those are totally expected. Non-fatal errors are not test failures in any of our tests, IIUC.

warner · 2021-02-17T03:20:42Z

@FUDCo said this didn't address the testLogger problem, but the ERTP tests now seem to pass without complaint, so I think we're ready to go, once trunk has settled down and CI finishes. This version uses plain Numbers for the external mailbox interface.

In the mailbox device, we use a plain Number, not BigInt, for external mailbox seqnums. Host applications call `exportMailbox()`, and are then responsible for conveying that data to a peer application. We can make their lives more convenient by returning something that can be serialized with a plain JSON.stringify. We make `exportMailbox()` emit plain Numbers for the sequence numbers, even though we store them internally as BigInts (validated with `Nat`). Likewise we accept Numbers from the host but convert them (with Nat) into BigInts for local processing.

Chip said it was ready to go except for the testLogger problem, which seems to no longer be a problem

erights · 2021-02-18T03:57:55Z

There is another Swingset error when trying to run the Swingset tests in ERTP:
  TypeError#1: Do not know how to serialize a BigInt
     at JSON.stringify (<anonymous>)
    at eval (packages/SwingSet/src/kernel/kernel.js:141:42)
This one is the test logger, which uses JSON.stringify to write args legibly to the in-memory test log. It already uses a quick-n-dirty reviver, so I wonder, since this is just for tests, if the reviver could just replace occurrences of BigInt with the nearest equivalent Number.

Yuck. If you're gonna do something that violent, replace it with the string of digits.

katelynsills added the Beta label Feb 13, 2021

katelynsills requested review from warner, FUDCo and Chris-Hibbert February 13, 2021 02:01

katelynsills self-assigned this Feb 13, 2021

katelynsills added this to the Beta Launch milestone Feb 13, 2021

erights requested changes Feb 13, 2021

View reviewed changes

packages/SwingSet/src/natNum.js Outdated Show resolved Hide resolved

This was referenced Feb 13, 2021

Add Counter abstraction to Swingset #2400

Closed

Remove Pointless Excessive Abstraction #2425

Merged

FUDCo previously requested changes Feb 15, 2021

View reviewed changes

Chris-Hibbert approved these changes Feb 15, 2021

View reviewed changes

packages/SwingSet/src/devices/timer-src.js Outdated Show resolved Hide resolved

warner reviewed Feb 15, 2021

View reviewed changes

packages/SwingSet/src/vats/vat-timerWrapper.js Outdated Show resolved Hide resolved

katelynsills force-pushed the 2400-swingset-counter branch 4 times, most recently from 6acf1b2 to 6c3c656 Compare February 15, 2021 22:27

katelynsills changed the title ~~chore: introduce natNum library and counter abstraction in kernelKeeper~~ chore: Make Swingset use Nat 4.0.0 Feb 15, 2021

katelynsills assigned warner and unassigned katelynsills Feb 15, 2021

erights approved these changes Feb 16, 2021

View reviewed changes

warner force-pushed the 2400-swingset-counter branch from db2bdc7 to fecb32c Compare February 16, 2021 20:20

warner requested a review from FUDCo February 16, 2021 20:21

warner force-pushed the 2400-swingset-counter branch from fecb32c to a10c1ec Compare February 17, 2021 03:18

warner removed the request for review from FUDCo February 17, 2021 03:18

warner force-pushed the 2400-swingset-counter branch from a10c1ec to ce6fb23 Compare February 17, 2021 03:21

warner enabled auto-merge (squash) February 17, 2021 03:24

warner merged commit 3da2426 into master Feb 17, 2021

warner deleted the 2400-swingset-counter branch February 17, 2021 03:34

katelynsills mentioned this pull request Feb 18, 2021

Error in SwingSet when using BigInt in ERTP #2463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Make Swingset use Nat 4.0.0 #2420

chore: Make Swingset use Nat 4.0.0 #2420

katelynsills commented Feb 13, 2021 •

edited

Loading

erights commented Feb 15, 2021

FUDCo left a comment

Chris-Hibbert left a comment

warner commented Feb 15, 2021

katelynsills commented Feb 15, 2021

katelynsills commented Feb 15, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021 •

edited

Loading

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

erights left a comment

warner commented Feb 16, 2021

warner commented Feb 16, 2021

warner commented Feb 16, 2021 •

edited

Loading

michaelfig commented Feb 17, 2021

warner commented Feb 17, 2021

erights commented Feb 18, 2021

chore: Make Swingset use Nat 4.0.0 #2420

chore: Make Swingset use Nat 4.0.0 #2420

Conversation

katelynsills commented Feb 13, 2021 • edited Loading

erights commented Feb 15, 2021

FUDCo left a comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

warner commented Feb 15, 2021

katelynsills commented Feb 15, 2021

katelynsills commented Feb 15, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021 • edited Loading

erights commented Feb 16, 2021

FUDCo commented Feb 16, 2021

erights commented Feb 16, 2021

erights left a comment

Choose a reason for hiding this comment

warner commented Feb 16, 2021

warner commented Feb 16, 2021

warner commented Feb 16, 2021 • edited Loading

michaelfig commented Feb 17, 2021

warner commented Feb 17, 2021

erights commented Feb 18, 2021

katelynsills commented Feb 13, 2021 •

edited

Loading

FUDCo commented Feb 16, 2021 •

edited

Loading

warner commented Feb 16, 2021 •

edited

Loading