-
Notifications
You must be signed in to change notification settings - Fork 212
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
feat(cosmic-swingset): execute during voting time #6741
base: master
Are you sure you want to change the base?
Conversation
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.
Looking really good! Please consider my comments at your discretion.
const mailboxStateMap = buildMailboxStateMap(mailboxStorage); | ||
const activeMailboxStateMap = { |
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.
If mailboxStateMap
is used only to create activeMailboxStateMap
, and otherwise should not be used, I would prefer:
const mailboxStateMap = buildMailboxStateMap(mailboxStorage); | |
const activeMailboxStateMap = { | |
const rawMailboxStateMap = buildMailboxStateMap(mailboxStorage); | |
const mailboxStateMap = { |
packages/vats/src/bridge.js
Outdated
for (const { bpid, result } of notifies) { | ||
const resolveKit = pendingResults.get(bpid); | ||
if (!resolveKit) { | ||
throw Fail`Unknown bridge result promise ${bpid}`; |
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 think it would be better to gather up all the unknown bpids, and throw an error describing them all at once. Up to you whether it's best to resolve/reject all known bpids before or after that throw.
bffa810
to
319405e
Compare
319405e
to
bfd588a
Compare
bfd588a
to
474a026
Compare
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.
Thanks for addressing my comments! I'll review it again when you ping me again.
@@ -16,6 +16,7 @@ const makeQueueSize = (key, size) => ({ key, size }); | |||
// experience if they don't. | |||
|
|||
export const BeansPerBlockComputeLimit = 'blockComputeLimit'; | |||
export const BeansPerInterBlockComputeLimit = 'interBockComputeLimit'; |
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.
export const BeansPerInterBlockComputeLimit = 'interBockComputeLimit'; | |
export const BeansPerInterBlockComputeLimit = 'interBlockComputeLimit'; |
And symbol for good measure
await fromBridge
Review feedback
Prevent any interaction with cosmos if not processing a blocking send
IntraBlock -> InterBlock
after-commit spans
a0a2f32
to
28895cb
Compare
Note to self: We need to store the blockParams of the committed block in swingStore so that a restarting node can resume and perform the right amount of computrons for the "inter-block" run before processing the next block. We can't use the new block's params since the value may have change (that would be a very unfortunate race to restart a node exactly during a param change, but it's possible). The params needs to be state-synced as a synchronizing node needs to perform the "inter-block" work like every other node. Edit: it doesn't need to be stored in swingstore, we can just read and write in vstorage directly. Writes can be done from golang, and while that may be more efficient (only read necessary is on restart), it would result in a strange split of duties between golang and JS. Edit2: we should abandon any between block actions when starting from an upgrade as all validators are guaranteed to have restarted and not executed anything yet. |
refs: #6447
Best reviewed commit-by-commit
Description
Building on top of #6683 and #6730, this introduces a bridge device implementation which uses asynchronous results, allowing to unlock queuing of chainSend while JS is not actively processing a
blockingSend
from golang.With some new queueing in cosmic-swingset, this allows executing swingset during voting time.
Security Considerations
This PR introduces guards to guarantee that Swingset activity will not cause calls into golang when not expected (outside of a
blockingSend
call).While executing swingset during voting time (outside of
blockingSend
), the chain sends are effectively queued in memory by cosmic-swingset, which is safe since they are flushed at the beginning of the next block. Any work performed during voting time is committed as part of the next block.Documentation Considerations
Some code comments updated, but there is probably some docs that bit-rotted because of this change.
Testing Considerations
The changes are mostly constrained to cosmic-swingset and related, which does not have good unit test coverage. However I ran the loadgen on this change, and it works well, having confirmed loadgen tasks now run decently faster. I need to do more analysis on the block timings to see if intra block work ever negatively impacts the following block.