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

Move all DA layer information into Kernel. Prevent user-space access to Kernel State #1251

Merged
merged 55 commits into from
Dec 22, 2023

Conversation

preston-evans98
Copy link
Member

@preston-evans98 preston-evans98 commented Dec 21, 2023

Description

This PR moves the chain-state module and related functionality into the kernel, and makes a handful of tweaks to ensure that the kernel values are inaccessible:

  • Add a new KernelModule trait analagous to Module, but with no CallMessage
  • Enforce that #[derive(ModuleInfo)] can only be called on structs that implement Module or KernelModule
  • Add a new #[derive(KernelModuleInfo)] macro that allows structs to use the #[kernel_module] attribute in addition to the existing attributes like #[module], #[state]. This macro implements ModuleInfo for KernelModules. Modules referenced by #[kernel_module] are required to implement the KernelModule trait.

After this PR, the Kernel is now...

  • Inaccessible to regular modules, without the need for complicated KernelWorkingSets. The KernelWorkingSet and related functionality will be removed in a follow up PR.
  • Fully responsible for chain-state management.
  • Able to access DA information via KernelSlotHooks, which are invoked by the StfBlueprint
  • Fully responsible to implement BlobSelector. The runtime no longer has this functionality.

Remaining work:

  • Completely re-write the integration tests for chain-state (module-system/module-implementations/integration-tests/src/chain_state/tests.rs and module-system/module-implementations/sov-chain-state/tests/all_tests.rs)
  • Completely re-write the integration tests for blob-selection
  • Remove DA-related information from the normal SlotHooks
  • Remove KernelWorkingSet and related types/functionality
  • Implement kernel logic to manage the user-space view of DA layer information
  • Simplify initialization logic for kernel and runtime
  • Re-enable chain-state RPC
  • Document Kernel and it's responsibilities
  • Document the method for soft-confirmations

Linked Issues

Testing

These changes are largely covered by existing tests. Improved integration tests will be added in a follow up.

@preston-evans98 preston-evans98 marked this pull request as ready for review December 21, 2023 17:28
@preston-evans98 preston-evans98 changed the title Preston/kernel state3 Move all DA layer information into Kernel. Prevent user-space access to Kernel State Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 127 lines in your changes are missing coverage. Please review.

Comparison is base (4497a7f) 80.1% compared to head (798dbbc) 79.1%.

Additional details and impacted files
Files Coverage Δ
...module-implementations/sov-chain-state/src/call.rs 100.0% <100.0%> (ø)
...ule-implementations/sov-chain-state/src/genesis.rs 100.0% <100.0%> (ø)
...odule-implementations/sov-chain-state/src/query.rs 0.0% <ø> (ø)
.../sov-modules-api/src/containers/versioned_value.rs 57.0% <100.0%> (+10.8%) ⬆️
module-system/sov-modules-api/src/lib.rs 100.0% <ø> (ø)
module-system/sov-modules-macros/src/lib.rs 100.0% <100.0%> (ø)
...ule-system/sov-modules-rollup-blueprint/src/lib.rs 92.2% <100.0%> (+2.2%) ⬆️
module-system/sov-modules-stf-blueprint/src/lib.rs 90.6% <100.0%> (+0.6%) ⬆️
...tem/sov-modules-stf-blueprint/src/stf_blueprint.rs 87.6% <ø> (ø)
...implementations/sov-attester-incentives/src/lib.rs 14.7% <0.0%> (ø)
... and 12 more

... and 9 files with indirect coverage changes

Copy link
Contributor

@vlopes11 vlopes11 left a comment

Choose a reason for hiding this comment

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

Non-blocking; Consider implementing a "KernelContext" without the mutable reference to a "WorkingSet." Instead, make this context accessible under the "Context" umbrella. When initiating a kernel call, pass the desired WorkingSet to the context, which will subsequently forward it to the KernelContext. This design can also be scaled for new hook implementations.

The proposed KernelContext would solely store height data internally and be functional regarding working set state modifications (accepting its mutable state as an argument). By implementing this abstraction, module developers will gain the ability to invoke specific kernel functions while being confined by the safeguards built into the context itself. Consequently, we establish a clear separation between user space and kernel space.

@preston-evans98
Copy link
Member Author

Consider implementing a "KernelContext" without the mutable reference to a "WorkingSet." Instead, make this context accessible under the "Context" umbrella. When initiating a kernel call, pass the desired WorkingSet to the context, which will subsequently forward it to the KernelContext. This design can also be scaled for new hook implementations.

This is an interesting idea. It's a bit out of scope for this PR, but it might solve some problems in the next one where we try to remove KernelWorkingSet entirely.

@preston-evans98 preston-evans98 added this pull request to the merge queue Dec 22, 2023
Merged via the queue into nightly with commit 180ecd0 Dec 22, 2023
15 of 16 checks passed
@preston-evans98 preston-evans98 deleted the preston/kernel-state3 branch December 22, 2023 18:52
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.

3 participants