[Neo Core] Part 1. Isolate Plugins Exceptions from the Node. #3309

Jim8y · 2024-06-08T16:39:59Z

Description

Currently plugins and the core runs in the same process, and we lack the measure of handling the plugin exceptions. This makes any plugin can have the ability to block the whole node, turning plugin an attack surface for neo core. Issue #3296 is a typical example.

Fixes # #3296 (comment)

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Jim8y · 2024-06-08T16:56:01Z

@shargon this should be a bug right?

shargon · 2024-06-08T16:57:16Z

@shargon this should be a bug right?

Bad design

src/Neo/Ledger/Blockchain.cs

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

roman-khimov · 2024-06-09T13:47:44Z

I'm not sure it improves things for users. The node will continue to work, sure, but if users have plugins they likely depend on them anyway (RPC/ApplicationLogs), so if node continues to process blocks, but users can't get data from it --- it's still broken from the user's perspective. It will also invalidate plugin state and will require subsequent resynchronization (right now updated nodes can just continue processing).

Jim8y · 2024-06-09T15:28:40Z

I'm not sure it improves things for users. The node will continue to work, sure, but if users have plugins they likely depend on them anyway (RPC/ApplicationLogs), so if node continues to process blocks, but users can't get data from it --- it's still broken from the user's perspective. It will also invalidate plugin state and will require subsequent resynchronization (right now updated nodes can just continue processing).

One service down does not mean the whole node including other services should also down. You can't say our brain should stop working if our hand is numb. And one pr only fix one specific problem. This pr isolate the exception from plugin to the core, which i believe is definately a good design, otherwise any plugin can be an attack surface of the whole node. As to your concern please check #3311.

But this is a good one, will update 3311 to make user able to config as stop the whole node.

cschuchardt88 · 2024-06-09T21:18:01Z

I think I will create a continue sync of state when missing data for ApplicationLog. To pickup the missing state.

shargon · 2024-06-10T08:14:56Z

src/Neo/Plugins/Plugin.cs

+            }
+            catch (Exception ex)
+            {
+                Utility.Log(nameof(Plugin), LogLevel.Error, ex);


The problem I see is that if you have a plugin that uses states, and it fails, you can have a corrupted chain and still work

I see, #3311 solve that problem

shargon · 2024-06-10T08:21:53Z

src/Neo/Ledger/Blockchain.cs

+                {
+                    handlerAction(handler);
+                }
+                catch (Exception ex) when (handler.Target is Plugin)


we need this when? it's only used for plugins

yes, need to check plugin exceptions seperately.

vncoelho

What about create a branch and merge the other related PRs into this branch we review one by one ?

shargon

Good if #3311 is merged

* [Neo Core] Part 1. Isolate Plugins Exceptions from the Node. (#3309) * catch plugin exceptions. * add UT test * udpate format * make the test more complete * complete the ut test * format * complete UT tests with NonPlugin case * async invoke * Update src/Neo/Ledger/Blockchain.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> --------- Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * [Neo Plugin New feature] UnhandledExceptionPolicy on Plugin Unhandled Exception (#3311) * catch plugin exceptions. * add UT test * udpate format * make the test more complete * complete the ut test * format * complete UT tests with NonPlugin case * async invoke * stop plugin on exception * remove watcher from blockchain if uint test is done to avoid cross test data pollution. * add missing file * 3 different policy on handling plugin exception * add missing file * fix null warning * format * Apply suggestions from code review Clean * Update src/Neo/Plugins/PluginSettings.cs Co-authored-by: Shargon <shargon@gmail.com> * Update src/Neo/Plugins/PluginSettings.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update src/Plugins/TokensTracker/TokensTracker.cs Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * Update src/Plugins/TokensTracker/TokensTracker.json --------- Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> * make the exception message clear --------- Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: NGD Admin <154295625+NGDAdmin@users.noreply.github.com>

Jim8y added 2 commits June 9, 2024 00:35

catch plugin exceptions.

3c02309

add UT test

e426ed6

Jim8y requested review from shargon, cschuchardt88, vncoelho, roman-khimov, AnnaShaleva, superboyiii and NGDAdmin June 8, 2024 16:50

Jim8y added the Waiting for Review label Jun 8, 2024

udpate format

5d6fa28

Jim8y mentioned this pull request Jun 8, 2024

some nodes on testnet stopped syncing maybe caused by some modules #3296

Closed

Jim8y changed the title ~~[Neo Core Bug] Catch plugin exceptions.~~ [Neo Core Bug] Isolate Plugins Exceptions from the Core. Jun 8, 2024

Jim8y changed the title ~~[Neo Core Bug] Isolate Plugins Exceptions from the Core.~~ [Neo Core Bug] Isolate Plugins Exceptions from the Node. Jun 8, 2024

Jim8y added the Bug Used to tag confirmed bugs label Jun 8, 2024

Jim8y added 2 commits June 9, 2024 00:58

make the test more complete

439f504

complete the ut test

e569fed

Jim8y removed the Bug Used to tag confirmed bugs label Jun 8, 2024

Jim8y added 2 commits June 9, 2024 01:01

format

d701dd3

complete UT tests with NonPlugin case

db1c7f0

shargon changed the title ~~[Neo Core Bug] Isolate Plugins Exceptions from the Node.~~ [Neo Core] Isolate Plugins Exceptions from the Node. Jun 8, 2024

async invoke

17f74ef

cschuchardt88 reviewed Jun 9, 2024

View reviewed changes

src/Neo/Ledger/Blockchain.cs Outdated Show resolved Hide resolved

Update src/Neo/Ledger/Blockchain.cs

6d06a53

Co-authored-by: Christopher Schuchardt <cschuchardt88@gmail.com>

Jim8y mentioned this pull request Jun 9, 2024

[Neo Plugin New feature] UnhandledExceptionPolicy on Plugin Unhandled Exception #3311

Merged

17 tasks

cschuchardt88 approved these changes Jun 9, 2024

View reviewed changes

shargon reviewed Jun 10, 2024

View reviewed changes

vncoelho reviewed Jun 10, 2024

View reviewed changes

Jim8y changed the base branch from master to plugin-unhandled-exception June 10, 2024 14:10

Jim8y changed the title ~~[Neo Core] Isolate Plugins Exceptions from the Node.~~ [Neo Core] Part 1. Isolate Plugins Exceptions from the Node. Jun 10, 2024

Jim8y added the Ready to Merge label Jun 11, 2024

Jim8y merged commit 89d6ea3 into neo-project:plugin-unhandled-exception Jun 16, 2024
7 checks passed

Jim8y deleted the isolate-plugins branch June 16, 2024 11:24

shargon reviewed Jun 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Core] Part 1. Isolate Plugins Exceptions from the Node. #3309

[Neo Core] Part 1. Isolate Plugins Exceptions from the Node. #3309

Jim8y commented Jun 8, 2024 •

edited

Loading

Jim8y commented Jun 8, 2024

shargon commented Jun 8, 2024

roman-khimov commented Jun 9, 2024

Jim8y commented Jun 9, 2024 •

edited

Loading

cschuchardt88 commented Jun 9, 2024 •

edited

Loading

shargon Jun 10, 2024

shargon Jun 10, 2024

shargon Jun 10, 2024

Jim8y Jun 10, 2024

vncoelho left a comment

shargon left a comment

[Neo Core] Part 1. Isolate Plugins Exceptions from the Node. #3309

[Neo Core] Part 1. Isolate Plugins Exceptions from the Node. #3309

Conversation

Jim8y commented Jun 8, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Jim8y commented Jun 8, 2024

shargon commented Jun 8, 2024

roman-khimov commented Jun 9, 2024

Jim8y commented Jun 9, 2024 • edited Loading

cschuchardt88 commented Jun 9, 2024 • edited Loading

shargon Jun 10, 2024

Choose a reason for hiding this comment

shargon Jun 10, 2024

Choose a reason for hiding this comment

shargon Jun 10, 2024

Choose a reason for hiding this comment

Jim8y Jun 10, 2024

Choose a reason for hiding this comment

vncoelho left a comment

Choose a reason for hiding this comment

shargon left a comment

Choose a reason for hiding this comment

Jim8y commented Jun 8, 2024 •

edited

Loading

Jim8y commented Jun 9, 2024 •

edited

Loading

cschuchardt88 commented Jun 9, 2024 •

edited

Loading