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

Kernel opening should be provable and confirmed on auxiliary #448

Merged
merged 33 commits into from
Nov 15, 2018

Conversation

deepesh-kn
Copy link
Contributor

fixes: #407
fixes: #408

As there is lot of cyclic interdependencies of the contract. I tried to avoid KernelGateway to be increase it. So there was a small change in flow. Now AuxiliaryBlockStore communicates with PollingPlace instead of KernelGateway.

Incase you do not agree with the flow. Lets discuss I can make a quick change.

Here is the flow.

  • a new kernel opens on origin
  • the origin state root that includes the new, open kernel is reported and finalized on auxiliary
  • anyone makes a Merkle proof of the open kernel in the KernelGateway
  • in the success case, the AuxiliaryBlockStore is informed that a new kernel opening was proven, AuxiliaryBlockStore stores the information about the kernel including the future validator sets
  • when the auxiliary dynasty increases the second time (d+2), the AuxiliaryBlockStore confirms the kernel opening
  • AuxiliaryBlockStore informs the PollingPlace about the new set of validators, and increase the meta-block height

The points that differs here are from the store are

  • KernelGateway informs the AuxiliaryBlockStore instead of PollingPlace about the future change in the set of validators.
  • When the auxiliary dynasty increases the second time (d+2), the metablock height and validators are updated in pollingplace. This is now called by auxiliaryBlockStore instead of KernelGateway.

There are interdependencies of the contracts. It is not resolved in this PR. So once it is resolved then function setKernelGateway(address _kernelGateway) should be removed from AuxiliaryBlockStore.sol

@schemar
Copy link
Contributor

schemar commented Nov 5, 2018

There are interdependencies of the contracts. It is not resolved in this PR. So once it is resolved then function setKernelGateway(address _kernelGateway) should be removed from AuxiliaryBlockStore.sol

Maybe add a follow-up ticket?

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Very interesting 👍
I added some comments in-line. But someone who has more experience with the gateways definitely should review it. The most important part (KernelGateway) I could not check very thoroughly.

contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/test/core/MockKernelGateway.sol Outdated Show resolved Hide resolved
test/core/kernel_gateway/constructor.js Outdated Show resolved Hide resolved
test/core/kernel_gateway/constructor.js Outdated Show resolved Hide resolved
test/core/polling_place/updateMetaBlockHeight.js Outdated Show resolved Hide resolved
test/core/auxiliary_block_store/reportOpenKernel.js Outdated Show resolved Hide resolved
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Thanks Deepesh! I still have some questions. And we should also wait for Sarvesh's feedback next week.

Note: Please end all doc comments with a period ..

contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryBlockStore.sol Show resolved Hide resolved
contracts/core/KernelGatewayInterface.sol Outdated Show resolved Hide resolved
contracts/lib/BytesLib.sol Outdated Show resolved Hide resolved
contracts/test/core/BlockStoreMock.sol Show resolved Hide resolved
test/core/polling_place/TestPollingPlace.sol Show resolved Hide resolved
Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Overall approach looks correct to me. 👍 🚀

Some comments and feedback inline.

@deepesh-kn @schemar
We should re-think for circular dependency between contracts (Polling place -> Block store -> Kernel Gateway). Specifically for justify method call from PollingPlace to BlockStore.

contracts/core/KernelGateway.sol Show resolved Hide resolved
contracts/core/KernelGateway.sol Show resolved Hide resolved
contracts/core/KernelGateway.sol Show resolved Hide resolved
contracts/core/KernelGateway.sol Outdated Show resolved Hide resolved
contracts/core/KernelGateway.sol Outdated Show resolved Hide resolved
test/core/kernel_gateway/activate_kernel.js Show resolved Hide resolved
test/core/kernel_gateway/get_active_kernel_hash.js Outdated Show resolved Hide resolved
test/core/kernel_gateway/get_open_kernel_hash.js Outdated Show resolved Hide resolved
test/core/kernel_gateway/prove_block_opening.js Outdated Show resolved Hide resolved
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

I have only tiny things left. Looks really good 🚤💨

contracts/core/AuxiliaryBlockStore.sol Outdated Show resolved Hide resolved
contracts/core/BlockStore.sol Outdated Show resolved Hide resolved
contracts/core/KernelGateway.sol Show resolved Hide resolved
contracts/core/PollingPlace.sol Show resolved Hide resolved
contracts/core/PollingPlace.sol Show resolved Hide resolved
contracts/test/core/BlockStoreMock.sol Show resolved Hide resolved
contracts/test/core/BlockStoreMock.sol Show resolved Hide resolved
contracts/test/core/MockKernelGateway.sol Outdated Show resolved Hide resolved
test/core/polling_place/TestPollingPlace.sol Show resolved Hide resolved
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

There are still some small questions, please see in-line.

contracts/core/AuxiliaryTransitionObjectInterface.sol Outdated Show resolved Hide resolved
contracts/core/AuxiliaryTransitionObjectInterface.sol Outdated Show resolved Hide resolved
contracts/core/BlockStore.sol Outdated Show resolved Hide resolved
contracts/core/OriginTransitionObjectInterface.sol Outdated Show resolved Hide resolved
contracts/core/OriginTransitionObjectInterface.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

🏎💨

Copy link
Contributor

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 🚁 🥇 🙌

Great work 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kernel opening should be confirmed on auxiliary Kernel opening should be provable on auxiliary
3 participants