SmartContract: restrict the number of allowed notifications #3548

AnnaShaleva · 2024-10-23T16:10:17Z

Description

The problem is described in nspcc-dev/neo-go#3490. In short words, an arbitrary number of smart contract notifications is allowed (up to VM's GasLimit and MaxBlockSystemFee, but it's huge), which results in a possibility to DoS the node. 10K of large notifications emitted in a single transaction result in block acceptance delays for both Go and C# nodes. 50K of large notifications may result in the node failure. I did these tests on privnet, you may find the results in nspcc-dev/neo-go#3490 (comment). Please, don't try to DoS mainnet/testnet using this vulnerability.

Port the solution from nspcc-dev/neo-go#3640, but an alternative solution is described in nspcc-dev/neo-go#3640 (comment) and in nspcc-dev/neo-go#3490 (comment). MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. nspcc-dev/neo-go#3490 (comment).

This PR is a subject of discussion, hence please, share your thoughts on the described problem and proposed solution. Once we agree on the solution, I'll add unit-tests to this PR.

Type of change

Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Unit-tests should be added once we agree on solution.

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

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Fix the problem described in nspcc-dev/neo-go#3490. Port the solution from nspcc-dev/neo-go#3640. MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. nspcc-dev/neo-go#3490 (comment). Signed-off-by: Anna Shaleva <[email protected]>

Signed-off-by: Anna Shaleva <[email protected]>

cschuchardt88 · 2024-10-23T16:20:24Z

This needs to be in VM limit class.

vncoelho · 2024-10-23T16:21:53Z

src/Neo/SmartContract/Native/RoleManagement.cs

@@ -78,7 +78,7 @@ private void DesignateAsRole(ApplicationEngine engine, Role role, ECPoint[] node
            list.AddRange(nodes);
            list.Sort();
            engine.SnapshotCache.Add(key, new StorageItem(list));
-


Formatting job is failing without this change.

AnnaShaleva · 2024-10-23T16:22:11Z

This needs to be in VM limit class.

Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.

vncoelho · 2024-10-23T16:22:57Z

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

+            // Restrict the number of notifications for Application executions. Do not check
+            // persisting triggers to avoid native persist failure. Do not check verification
+            // trigger since verification context is loaded with ReadOnly flag.
+            if (IsHardforkEnabled(Hardfork.HF_Echidna) && Trigger == TriggerType.Application && notifications.Count == MaxNotificationCount)


Looks good to me, we just need to ensure .Count is correct calculated at this step.

Is trigger really needed? Limits shouldn't have a trigger.

Yes, it's needed, because we can't allow OnPersist and PostPersist failures (imagine that in future MaxNotificationCount may be reduced to some lower value). And for Verification trigger this check is useless by design, no notifications are possible with this trigger.

Ref. nspcc-dev/neo-go#3490 (comment).

cschuchardt88 · 2024-10-23T17:12:27Z

This needs to be in VM limit class.

Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.

All the same thing. Class is called ExecutionEngineLimits in Neo.VM namespace. How to use engine.Limit.MaxNotification

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

Jim8y · 2024-10-24T09:31:27Z

instead of limiting number of notifactions, i prefer to make the price related to the notifaction size. From @AnnaShaleva 's excellent benchmark, i think size * number is the real reason. Cause i think its a normal practice contract may need to send a lof of notifactions, but definately abnormal to send large notifactions.

AnnaShaleva · 2024-10-24T11:00:30Z

i think size * number is the real reason

Exactly, as described in nspcc-dev/neo-go#3490 (comment).

i prefer to make the price related to the notifaction size.

Yes, it's an alternative solution proposed in nspcc-dev/neo-go#3490 (comment). But we need to discuss it because even with extremely expensive notifications there's a chance that CNs will be killed by a single transaction. Yes, the user will have to pay for it, but CNs may not be able to process this transaction. Also, large number of notifications is harmful for both C# and Go RPC servers.

So to me, the restriction of the overall notifications number is required, but at the same time it can be combined with dynamic price solution.

src/Neo/SmartContract/ApplicationEngine.Runtime.cs

shargon · 2024-12-20T10:28:37Z

Any feedback or merge?

Jim8y · 2024-12-31T07:59:21Z

Any feedback or merge?

As is discussed in our previous meeting, this should wait for #3644

The base branch was changed.

shargon · 2025-01-24T08:22:39Z

src/Neo.CLI/config.fs.mainnet.json

@@ -40,7 +40,8 @@
      "HF_Aspidochelone": 3000000,
      "HF_Basilisk": 4500000,
      "HF_Cockatrice": 5800000,
-      "HF_Domovoi": 5800000
+      "HF_Domovoi": 5800000,
+      "HF_Echidna": 5800001


@AnnaShaleva Should be removed from config

It comes from the branch change

Ping @AnnaShaleva

Will update.

src/Neo/SmartContract/Native/RoleManagement.cs

shargon · 2025-03-03T10:24:45Z

This should be included in 3.8

AnnaShaleva · 2025-03-03T11:15:50Z

Previously we’ve agreed that this solution is less preferable than #3600. @roman-khimov do you think we need to merge it?

shargon · 2025-03-03T12:03:17Z

Previously we’ve agreed that this solution is less preferable than #3600. @roman-khimov do you think we need to merge it?

Why not complementary?

roman-khimov · 2025-03-03T12:32:26Z

The theory is that if we're to have #3552 done with MaxBlockSystemFee of like 20 and then have ExecutionFeeFactor of 30 again then 20 GAS is ~2034 notifications and we're fine as is. The problem is that we neither have a low MaxBlockSystemFee value nor we have a high ExecutionFeeFactor (and can't have it till #3600, which will happen no one knows when). Given the severity of the issue at the same time, I'm OK with some hard limit for 3.8/Echidna. At least it's simple and effective.

Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2025-03-03T13:46:52Z

Good, updated. Then let's consider merging this PR in 3.8.

I'm also voting for the merge since I agree that this solution is better than nothing. Later, when we have lower MaxBlockSystemFee and flexible opcode prices, we may revert notifications number constraint (in some next hardfork).

Jim8y and others added 8 commits August 8, 2024 00:24

add hardofork HF_Echidna

bcc5fd0

Add entries to Designation event (#3397)

e63f10b

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

[Neo Core StdLib] Add Base64url (#3453)

9d16c53

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

add hardofork HF_Echidna

f746f8d

Add entries to Designation event (#3397)

8d7f9e8

* Add entries to Designation event * Change to HF_Echidna * Add UT * Add count

[Neo Core StdLib] Add Base64url (#3453)

3fc8077

* add base64url * active in * update placehold hf height * fix hf issue and move methods to proper place. * fix test * use identifymodel instead.

AnnaShaleva added Discussion Initial issue state - proposed but not yet accepted Waiting for Review Hardfork labels Oct 23, 2024

AnnaShaleva requested review from shargon, cschuchardt88, Jim8y, vncoelho and roman-khimov October 23, 2024 16:10

AnnaShaleva mentioned this pull request Oct 23, 2024

【Oct.24】Core Dev Regular Meeting Proposal Discussion #3547

Closed

SmartContract: fix format

0c72e57

Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva mentioned this pull request Oct 23, 2024

core: restrict the number of allowed SC notifications nspcc-dev/neo-go#3640

Open

vncoelho reviewed Oct 23, 2024

View reviewed changes

shargon reviewed Oct 23, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Outdated Show resolved Hide resolved

Update src/Neo/SmartContract/ApplicationEngine.Runtime.cs

ee21427

shargon reviewed Oct 23, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Show resolved Hide resolved

Avoid notification creation

4a6f36a

shargon reviewed Nov 20, 2024

View reviewed changes

src/Neo/SmartContract/ApplicationEngine.Runtime.cs Show resolved Hide resolved

AnnaShaleva mentioned this pull request Dec 18, 2024

[Dec.19] Core Dev Regular Meeting Proposal Discussion #3632

Closed

shargon added Ready to Merge and removed Waiting for Review labels Dec 20, 2024

nan01ab previously approved these changes Dec 24, 2024

View reviewed changes

Jim8y removed the Ready to Merge label Jan 7, 2025

Base automatically changed from HF_Echidna to master January 23, 2025 09:52

shargon reviewed Jan 24, 2025

View reviewed changes

Merge branch 'master' into restrict-notifications-count

e5e3b08

shargon reviewed Jan 24, 2025

View reviewed changes

src/Neo/SmartContract/Native/RoleManagement.cs Outdated Show resolved Hide resolved

Update src/Neo/SmartContract/Native/RoleManagement.cs

f43f296

shargon added the Bug Used to tag confirmed bugs label Mar 3, 2025

AnnaShaleva force-pushed the restrict-notifications-count branch 2 times, most recently from 8dd5b25 to 7345ac4 Compare March 3, 2025 13:32

Neo.CLI: revert configuration changes

c309f63

Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva force-pushed the restrict-notifications-count branch from 7345ac4 to c309f63 Compare March 3, 2025 13:32

Merge branch 'master' into restrict-notifications-count

f5a0482

shargon approved these changes Mar 3, 2025

View reviewed changes

AnnaShaleva requested review from nan01ab, vncoelho and cschuchardt88 March 3, 2025 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SmartContract: restrict the number of allowed notifications #3548

SmartContract: restrict the number of allowed notifications #3548

AnnaShaleva commented Oct 23, 2024

cschuchardt88 commented Oct 23, 2024

vncoelho Oct 23, 2024

AnnaShaleva Oct 23, 2024 •

edited

Loading

AnnaShaleva commented Oct 23, 2024

vncoelho Oct 23, 2024

cschuchardt88 Oct 23, 2024 •

edited

Loading

AnnaShaleva Oct 24, 2024

AnnaShaleva Oct 25, 2024

cschuchardt88 commented Oct 23, 2024 •

edited

Loading

Jim8y commented Oct 24, 2024 •

edited

Loading

AnnaShaleva commented Oct 24, 2024 •

edited

Loading

shargon commented Dec 20, 2024

Jim8y commented Dec 31, 2024 •

edited

Loading

shargon Jan 24, 2025

shargon Jan 24, 2025

shargon Mar 3, 2025

AnnaShaleva Mar 3, 2025

AnnaShaleva Mar 3, 2025

shargon commented Mar 3, 2025

AnnaShaleva commented Mar 3, 2025

shargon commented Mar 3, 2025

roman-khimov commented Mar 3, 2025

AnnaShaleva commented Mar 3, 2025

SmartContract: restrict the number of allowed notifications #3548

Are you sure you want to change the base?

SmartContract: restrict the number of allowed notifications #3548

Conversation

AnnaShaleva commented Oct 23, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

cschuchardt88 commented Oct 23, 2024

Choose a reason for hiding this comment

AnnaShaleva Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

AnnaShaleva commented Oct 23, 2024

Choose a reason for hiding this comment

cschuchardt88 Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 commented Oct 23, 2024 • edited Loading

Jim8y commented Oct 24, 2024 • edited Loading

AnnaShaleva commented Oct 24, 2024 • edited Loading

shargon commented Dec 20, 2024

Jim8y commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon commented Mar 3, 2025

AnnaShaleva commented Mar 3, 2025

shargon commented Mar 3, 2025

roman-khimov commented Mar 3, 2025

AnnaShaleva commented Mar 3, 2025

AnnaShaleva Oct 23, 2024 •

edited

Loading

cschuchardt88 Oct 23, 2024 •

edited

Loading

cschuchardt88 commented Oct 23, 2024 •

edited

Loading

Jim8y commented Oct 24, 2024 •

edited

Loading

AnnaShaleva commented Oct 24, 2024 •

edited

Loading

Jim8y commented Dec 31, 2024 •

edited

Loading