cosmos-sdk icon indicating copy to clipboard operation
cosmos-sdk copied to clipboard

feat: ADR 038 plugin system

Open egaxhaj opened this issue 3 years ago • 12 comments

For https://github.com/cosmos/cosmos-sdk/issues/10096

Implements https://github.com/cosmos/cosmos-sdk/pull/11175

On top of https://github.com/cosmos/cosmos-sdk/pull/10639

This is an extension and refactor of the existing ADR-038 streaming service work to introduce a plugin system to the SDK and load streaming services using this system rather than building them into the SDK binary.

The plugin system introduced here is meant to be extensible, so that if other components/features of the SDK wish to be included as plugins in the future they can extend the base plugin interface defined here and leverage this plugin building and loading/preloading system.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.

I have...

  • [x] included the correct type prefix in the PR title
  • [ ] added ! to the type prefix if API or client breaking change
  • [x] targeted the correct branch (see PR Targeting)
  • [x] provided a link to the relevant issue or specification
  • [ ] followed the guidelines for building modules
  • [x] included the necessary unit and integration tests
  • [x] added a changelog entry to CHANGELOG.md
  • [x] included comments for documenting Go code
  • [ ] updated the relevant documentation or specification
  • [ ] reviewed "Files changed" and left comments if necessary
  • [ ] confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.

I have...

  • [ ] confirmed the correct type prefix in the PR title
  • [ ] confirmed ! in the type prefix if API or client breaking change
  • [ ] confirmed all author checklist items have been addressed
  • [ ] reviewed state machine logic
  • [ ] reviewed API design and naming
  • [ ] reviewed documentation is accurate
  • [ ] reviewed tests and test coverage
  • [ ] manually tested (if applicable)

egaxhaj avatar Apr 19 '22 18:04 egaxhaj

Since adr038 is designed as a plugin system I would opt to having various plugins hosted in different repos rather than the core sdk. You mentioned having limited support to maintain this feature going forward. I would like to limit the amount of new features landing in the sdk without a clear maintenance team and responsibility. Adding features into the sdk and having the sdk team maintain it if things go wrong is not sustainable at this time. Happy to chat about this but for now I'm leaning on excluding this from core sdk.

tac0turtle avatar Apr 22 '22 08:04 tac0turtle

Closed by accident.

egaxhaj avatar Aug 01 '22 16:08 egaxhaj

I agree with @marbar3778's first comment up top. Since these are plugins, they should be optional additions instead of required tag-a-longs. This is primarily an issue with the kafka plugin; the kafka go library doesn't support ARM, and requires special external setup and build treatment on ARM machines. Really, this holds true for any plugin that would require importing a library not needed anywhere else in the SDK.

Plus, having the plugins in a separate repo would better demonstrate how someone could build and add their own plugin.

@marbar3778

Our intent is to host these in a separate repository as discussed here. However, we would still be faced with the similar problems as stated above with other third party libraries in the future, would we not?

Another area of concern is how the plugin injection system works. Underneath it all, the plugin system relies on Go's plugin.Plugin to open and load plugins. Go's plugin system comes with a few restrictions.

  • the pre-compiled plugins must have package main as their package name or they'll not compile.
  • the same Go version must be used between the pre-compiled plugins and the code loading them or they'll fail to load. This will require all plugins to be pre-compiled on each Go version bump of the SDK.
  • supported only on Linux, FreeBSD, and macOS

Instead, why not take a similar approach as in the ABCI protocol and use gRPC to push the data out to another process? This other gRPC process can listen on localhost.

  • we are now able to leverage a common design theme that the SDK already uses
  • plugin authors can now leverage the language of their choice
  • no more having to solve issues about third party dependencies
  • no more separate repository to deal with
  • simple design

Take a look at hashicorp/go-plugin. We can borrow from their approach.

Our requirements are to push state-change and ABCI request response events for BeginBlock, EndBlock, DeliverTx (FinalizeBlock in ABCI++) out to an external system. These are already processed events, our approach needs to be elegant and simple and most importantly should not deal with any of the issues mentioned above.

A few questions have come up about overhead. If this is a huge deterrent for those concerned, then state-listening, when enabled, can be a no-op if a node is running as a validator. Same as when enabling grpc-web is a no-op if grpc is disabled.

egaxhaj avatar Aug 02 '22 00:08 egaxhaj

Underneath it all, the plugin system relies on Go's plugin.Plugin to open and load plugins. Go's plugin system comes with a few restrictions . . .

Not just restrictions! Like e.g. package netchan, Go's stdlib package plugin was always experimental, and has been effectively abandoned for years. It's not an appropriate choice for anything other than toy projects.

There are basically two viable approaches for plugins in Go: external processes and RPCs i.e. hashicorp/go-plugin as @egaxhaj mentions, or compile-time options i.e. caddy-server/caddy. I'd consider this refactor a blocking requirement.

peterbourgon avatar Aug 07 '22 22:08 peterbourgon

Instead, why not take a similar approach as in the ABCI protocol and use gRPC to push the data out to another process? This other gRPC process can listen on localhost.

im a huge fan of this approach. I like it a lot better than the current approach.

tac0turtle avatar Aug 14 '22 13:08 tac0turtle

I really prefer the gRPC option as well as I feel like it already fits nicely into the SDK architecture and would allow for a great range of flexibility of plugin implementations.

alexanderbez avatar Aug 15 '22 01:08 alexanderbez

could we use hashicorup-plugins and replace all the current code?

tac0turtle avatar Aug 15 '22 07:08 tac0turtle

could we use hashicorup-plugins and replace all the current code?

Yes, I'll work on a PR using the hashicorp-plugin to see how well it fits with what we want to do.

egaxhaj avatar Aug 16 '22 16:08 egaxhaj

could we use hashicorup-plugins and replace all the current code?

Yes, I'll work on a PR using the hashicorp-plugin to see how well it fits with what we want to do.

Update on the timeline. I'm wrapping up some other work this week and will be out on PTO 8/25 - 9/5. I'll be free to focus my time on this when I'm back in the office.

egaxhaj avatar Aug 23 '22 15:08 egaxhaj

thank you for the update!! enjoy the time off

tac0turtle avatar Aug 29 '22 12:08 tac0turtle

@egaxhaj following up on this. do you need some help in pushing this over the finish line?

tac0turtle avatar Sep 19 '22 08:09 tac0turtle

@egaxhaj following up on this. do you need some help in pushing this over the finish line? I was wrapping up work that prevented me from looking at this any sooner. I'll be solely focusing on this this week. I'll checkin later into the week to let you know of my progress.

egaxhaj avatar Sep 19 '22 18:09 egaxhaj

replaced by #13472

egaxhaj avatar Oct 06 '22 23:10 egaxhaj