Skip to content
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): add swingset 'crankhash' to state vector #3626

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

warner
Copy link
Member

@warner warner commented Aug 9, 2021

This 'crankhash' covers all kernel state changes, so if any two validators
diverge, their crankhashes should diverge too. By storing them into the
cosmos-sdk state vector, the AppHashes will diverage as well, causing at
least one of them to fall out of consensus. This should let us catch
divergence as soon as possible.

closes #3442

@warner warner added the cosmic-swingset package: cosmic-swingset label Aug 9, 2021
@warner warner added this to the Testnet: Metering Phase milestone Aug 9, 2021
@warner warner requested a review from michaelfig August 9, 2021 07:30
@warner warner self-assigned this Aug 9, 2021
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Looks good in principle. I presume the crankhash is a rolling hash so that sampling it just once at the end of the block is useful?

@warner
Copy link
Member Author

warner commented Aug 9, 2021

Looks good in principle. I presume the crankhash is a rolling hash so that sampling it just once at the end of the block is useful?

Right. Each crank's DB changes are (non-confusably) concatenated and hashed into the "crankhash" for that one crank, then the DB .crankhash is updated to be the hash of the previous .crankhash plus the new crank's hash. So the current DB .crankhash should be sensitive to the entire history.

This includes the v$NN.source static vat sources, but no transcript entries, so it omits dynamic vat sources.

@warner
Copy link
Member Author

warner commented Aug 9, 2021

Looks good in principle.

And in the specific.. am I writing the hash into the cosmos (consensus-checked) state vector correctly?

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Need to change the crankhash property to be named value instead.

packages/cosmic-swingset/src/chain-main.js Outdated Show resolved Hide resolved
@warner warner marked this pull request as ready for review August 10, 2021 23:55
Base automatically changed from 3442-activity-hash to master August 12, 2021 21:49
@warner warner requested a review from michaelfig August 12, 2021 21:51
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM. Please consider my question, though.

@@ -232,6 +233,9 @@ export async function launch(
blockHeight,
blockTime,
});
if (setActivityhash) {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why is this not a part of the above bootstrapBlock function as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, good question, I hadn't thought about it. I think that'd be fine, it would capture/signal any divergence during the initial controller.run() that sets up the javascript world. Without it, we'd signal that divergence during the second block instead of during the first. The activityhash is updated for every commitCrank (including the one that initializeKernel() does, making its starting value effectively a hash of the config object), so even if we don't read it at that point, it's still sensitive to the initial state.

I'll add that.

This 'activityhash' covers all kernel state changes, so if any two validators
diverge, their activityhashes should diverge too. By storing them into the
cosmos-sdk state vector, the AppHashes will diverage as well, causing at
least one of them to fall out of consensus. This should let us catch
divergence as soon as possible.

closes #3442
@warner warner merged commit d63810f into master Aug 13, 2021
@warner warner deleted the 3442-store-crankhash branch August 13, 2021 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmic-swingset package: cosmic-swingset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"kernel activity hash": add some amount of swingset state into the cosmos AppHash
2 participants