Skip to content

Draft: Add support for Lua-based TX filtering #119

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

Draft
wants to merge 1 commit into
base: 28.x-knots
Choose a base branch
from

Conversation

jasonfoura
Copy link

No description provided.

@jasonfoura jasonfoura changed the title WIP Draft: Add support for Lua-based TX filtering May 9, 2025
@DeepDoge
Copy link

DeepDoge commented May 10, 2025

Have you considered using WASM instead of Lua for TX filtering?
WASM is bytecode, supported by multiple languages (including Lua), and could offer better performance while still providing strong sandboxing.

@luke-jr
Copy link
Member

luke-jr commented May 11, 2025

But WASM requires compiling, therefore defeating the purpose as I understand it?

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

A lot of work to do here, so I'm going to convert the PR to a draft.

Performance is a potential concern, as is security.

I wonder if it's practical to just run Python or something in a super-minimal KVM environment? (But idk if KVM is usable cross-platform...)

Comment on lines +1468 to +1469
CPPFLAGS="-I/opt/homebrew/opt/lua/include/lua $CPPFLAGS"
LIBS="-L/opt/homebrew/opt/lua/lib -llua -lm $LIBS"
Copy link
Member

Choose a reason for hiding this comment

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

This certainly isn't portable.

@@ -0,0 +1 @@
return true
Copy link
Member

Choose a reason for hiding this comment

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

Two demos that do the same thing?

@@ -24,7 +24,7 @@ bool Clang_IndVarSimplify_Bug_SanityCheck() {
}
last = *it;
}
return false;
return true;
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the sanity check. If the test is failing for you, you should get a compiler that doesn't have the bug. It could very well impact other code!

@@ -0,0 +1,53 @@
// The MIT License (MIT)
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to use the system-installed sol if possible (optional at compile time). Or at least a subtree.

@luke-jr luke-jr marked this pull request as draft May 11, 2025 01:58
@chrisguida
Copy link

chrisguida commented May 11, 2025

@jasonfoura Amazing PR!

It seems security could be an issue if anyone is allowed to write arbitrary filter code that can be injected into Knots.

Perhaps we could have a distribution method that allows well-known devs to attest to particular filters?

@DeepDoge
Copy link

But WASM requires compiling, therefore defeating the purpose as I understand it?

That’s true, but using WASM still wouldn’t require recompiling the node itself, only the individual user scripts.
From what I understand, this is more like a plugin system?

I’m not sure what @jasonfoura’s exact vision is, but either way, having a script-based filtering option like this would be really useful. Just wanted to give my input.

@jasonfoura
Copy link
Author

Thanks for your time and feedback @luke-jr, @DeepDoge, and @chrisguida!

I've opened a new discussion. Let's continue there 👍

@michaelfolkson
Copy link

@jasonfoura: Cool. Perhaps add a PR description too including a link to the discussion.

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

Successfully merging this pull request may close these issues.

6 participants