[Neo Core Fix]Fix https://github.com/neo-project/neo/issues/2862 #3364

Jim8y · 2024-06-27T18:04:00Z

Description

There is an issue in the memorypool where attacker can dos the node by sending tons of transactions with ascending transaction fees.

Fixes # #2862

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:

TestReVerifyTopUnverifiedTransactionsIfNeeded
TestReVerifyTopUnverifiedTransactionsWithSmartThrottler

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-27T18:30:38Z

@superboyiii can you help to test it? I will fix the test if this pr is truly working. @dusmart can you take a look?

roman-khimov

Not a solution to me.

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

cschuchardt88

Why are all these markdown file in with the source? Should be in docs folder in the root of the repo.

Jim8y · 2024-06-28T01:56:01Z

Why are all these markdown file in with the source? Should be in docs folder in the root of the repo.

I know what you mean, but we don't have a doc folder.

Jim8y · 2024-06-28T02:02:46Z

Not a solution to me.

I need more specific reason.

resolved

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs

clean empty lines

shargon

Partially review at Jim8y#1

shargon · 2024-06-28T08:23:41Z

src/Neo/Ledger/SmartThrottler.cs

+    /// </summary>
+    private bool IsHighPriorityTransaction(Transaction tx)
+    {
+        // High priority: fee > 3x average


committee address?

src/Neo/Ledger/MemoryPool.cs

vncoelho · 2024-06-28T09:46:29Z

src/Neo/Ledger/SmartThrottler.cs

+
+    // Fields for network load estimation
+    private readonly Queue<ulong> _recentBlockTimes = new();
+    private const int BlockTimeWindowSize = 20; // Consider last 20 blocks


Move to config

vncoelho

I reviewed the PR, @Jim8y.
I see some hope in it right now.

This is currently not in milestone for 3.8.0, however, I will try to take a look more carefully soon.

vncoelho · 2024-06-28T09:53:10Z

src/Neo/Ledger/SmartThrottler.cs

+        var memoryPoolUtilization = (double)_memoryPool.Count / _system.Settings.MemoryPoolMaxTransactions;
+        var networkLoad = EstimateNetworkLoad(block);
+
+        _maxTransactionsPerSecond = CalculateOptimalTps(memoryPoolUtilization, networkLoad, block);


_optimalMaxTransactionsPerSecond instead of _maxTransactionsPerSecond

no difference to the code.

nomenclature is bad like this, maxTransactionPerSecond for something that is variable does not looks good to me.

vncoelho · 2024-06-28T09:55:18Z

src/Neo/Ledger/SmartThrottler.cs

+    /// Estimates current network load
+    /// </summary>
+    /// <returns>An integer between 0 and 100 representing the estimated network load</returns>
+    private int EstimateNetworkLoad(Block? block)


move to double as well and base %, already divided by 100.

The only use of it is aligned with memPool Use

vncoelho · 2024-06-28T09:55:39Z

src/Neo/Ledger/SmartThrottler.cs

+        var load = 0;
+
+        // 1. Memory pool utilization (30% weight)
+        var memPoolUtilization = (double)_memoryPool.Count / _system.Settings.MemoryPoolMaxTransactions;


get this value from previous line before calling the method, it is called just once as it is right now

vncoelho · 2024-06-28T09:57:57Z

src/Neo/Ledger/SmartThrottler.cs

+        {
+            var avgBlockTime = _recentBlockTimes.Average(t => (double)t);
+            var blockTimeRatio = avgBlockTime / _system.Settings.MillisecondsPerBlock;
+            load += (int)(Math.Min(blockTimeRatio, 2) * 30); // Cap at 60 points


I am not sure.

avgBlockTime should usually not be lower than " _system.Settings.MillisecondsPerBlock".
So, usually blockTimeRatio is close to "1".

So, there is an average load from 30 - 60

its either 0 or 30 actually.

How?

(int)(Math.Min(blockTimeRatio, 2) * 30);

BlockTimeRatio will never/RARELY be less than 1.
So the range is 30 - 60 in this formula

vncoelho · 2024-06-28T09:58:27Z

src/Neo/Ledger/SmartThrottler.cs

+    {
+        var load = 0;
+
+        // 1. Memory pool utilization (30% weight)


These weights does not look like %, they are more than 100

how could it be more than 100?

var memPoolUtilization = (double)_memoryPool.Count / _system.Settings.MemoryPoolMaxTransactions * 100;

Check in the thread below

vncoelho · 2024-06-28T09:59:59Z

src/Neo/Ledger/SmartThrottler.cs

+            load += (int)(Math.Min(txGrowthRate, 1) * 40);
+        }
+
+        return Math.Min(load, 100); // Ensure load doesn't exceed 100


This is bad for any weight metric, insert some cut after weighting.

This part of the code needs enhanced.

If we decide move on with this solution I can later test and analyze.

Updated, now maximum load is 100.

how could it be more than 100?

The same reason you have this Math.Min(load, 100); here

vncoelho · 2024-06-28T15:58:32Z

src/Neo/Ledger/SmartThrottler.cs

+        if (_recentBlockTimes.Count > 0)
+        {
+            var avgBlockTime = _recentBlockTimes.Average(t => (double)t);
+            load += (avgBlockTime < _system.Settings.MillisecondsPerBlock?1:0) * 30; // Cap at 30 points


This is bad formula as well, @Jim8y.
load += (avgBlockTime < _system.Settings.MillisecondsPerBlock?1:0) * 30; // Cap at 30 points is a degree function in control theory.

Previous formula was surely better, you just need to adjust the weights.

could not understand, maybe you can provide one for me.

AnnaShaleva

Not a solution to me.

I need more specific reason.

I agree with Roman, and I don't quite like the idea of restricting the node in that way. I also agree with the #2862 (comment), it seems that this problem should be resolved on a consensus level rather than by restricting TPS at mempool level.

dusmart · 2024-07-02T11:50:25Z

It's great to have a feasible solution here.

My concern is that will this smart throttler can not defend us from other DoS vectors. A possible scene maybe the leader CN chooses a low priority tx somehow, then the other CNs may not get that low priority tx during a heavy network load. Then the consensus could not be reached. I've got to say. This is not the issue introduced by this PR. It's there already.

It would be better if this or some new PRs could prevent the above scene.

Jim8y · 2024-07-16T02:27:59Z

I agree with Roman, and I don't quite like the idea of restricting the node in that way. I also agree with the #2862 (comment), it seems that this problem should be resolved on a consensus level rather than by restricting TPS at mempool level.

@AnnaShaleva I dont care about how you agree with roman, i am sorry to say this anna, you and roman are in the same community and ofcourse you are with him all the time. What i care is what is your solution, you can always say this is not a perfect solution, I accept that, but where is your solution? leaving the network unprotected for months even years just waiting for a perfect solution is your solution? Previously i also asked @shargon not to sayI dont like or I dont think , cause it means nothing to me, as i dont care about how you feel, but I do respect and value your technical opinion. Propose your solution, work on it, that is what we need right now.

You are such a great reviewer and core contributer, I will always carefully consider your suggestions. As long as its not some feeling.

Jim8y · 2024-12-17T12:46:40Z

replaced with #3631

Jim8y added 2 commits June 28, 2024 01:59

fix 2862

a037be6

fix test setting

ebc9377

fix and add test

6c27d09

roman-khimov requested changes Jun 27, 2024

View reviewed changes

vncoelho previously requested changes Jun 27, 2024

View reviewed changes

src/Neo.CLI/config.fs.testnet.json Outdated Show resolved Hide resolved

cschuchardt88 reviewed Jun 28, 2024

View reviewed changes

remove limitation to sender

9f13605

Jim8y added the Waiting for Review label Jun 28, 2024

code format

7d1afea

Jim8y requested review from shargon, cschuchardt88, vncoelho, roman-khimov and AnnaShaleva June 28, 2024 05:00

shargon reviewed Jun 28, 2024

View reviewed changes

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs Outdated Show resolved Hide resolved

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

883bce9

clean empty lines

shargon reviewed Jun 28, 2024

View reviewed changes

shargon mentioned this pull request Jun 28, 2024

null checks for #3364 and fix some bugs Jim8y/neo#1

Closed

apply nullable

977226a

shargon reviewed Jun 28, 2024

View reviewed changes

vncoelho reviewed Jun 28, 2024

View reviewed changes

src/Neo/Ledger/MemoryPool.cs Show resolved Hide resolved

vncoelho reviewed Jun 28, 2024

View reviewed changes

Jim8y added 3 commits June 28, 2024 23:29

prioritize Committee transaction

3069a21

fix the networkload issue

8893cd7

format

e397df8

vncoelho reviewed Jun 28, 2024

View reviewed changes

Merge branch 'master' into fix-2862

004e5a1

shargon mentioned this pull request Jun 28, 2024

Fix: MemPool null checks #3367

Merged

15 tasks

AnnaShaleva reviewed Jul 1, 2024

View reviewed changes

Jim8y mentioned this pull request Dec 17, 2024

[Neo Core Memory Pool] fix flooding attack #3631

Closed

14 tasks

Jim8y closed this Dec 17, 2024

Jim8y deleted the fix-2862 branch December 17, 2024 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Neo Core Fix]Fix https://github.com/neo-project/neo/issues/2862 #3364

[Neo Core Fix]Fix https://github.com/neo-project/neo/issues/2862 #3364

Jim8y commented Jun 27, 2024 •

edited

Loading

Jim8y commented Jun 27, 2024 •

edited

Loading

roman-khimov left a comment

cschuchardt88 left a comment

Jim8y commented Jun 28, 2024

Jim8y commented Jun 28, 2024

shargon left a comment

shargon Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho left a comment

vncoelho Jun 28, 2024

Jim8y Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024

Jim8y Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024

Jim8y Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024 •

edited

Loading

Jim8y Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024

vncoelho Jun 28, 2024 •

edited

Loading

Jim8y Jun 28, 2024

AnnaShaleva left a comment

dusmart commented Jul 2, 2024 •

edited

Loading

Jim8y commented Jul 16, 2024 •

edited

Loading

Jim8y commented Dec 17, 2024

[Neo Core Fix]Fix https://github.com/neo-project/neo/issues/2862 #3364

[Neo Core Fix]Fix https://github.com/neo-project/neo/issues/2862 #3364

Conversation

Jim8y commented Jun 27, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Jim8y commented Jun 27, 2024 • edited Loading

roman-khimov left a comment

Choose a reason for hiding this comment

cschuchardt88 left a comment

Choose a reason for hiding this comment

Jim8y commented Jun 28, 2024

Jim8y commented Jun 28, 2024

shargon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vncoelho left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vncoelho Jun 28, 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

vncoelho Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaShaleva left a comment

Choose a reason for hiding this comment

dusmart commented Jul 2, 2024 • edited Loading

Jim8y commented Jul 16, 2024 • edited Loading

Jim8y commented Dec 17, 2024

Jim8y commented Jun 27, 2024 •

edited

Loading

Jim8y commented Jun 27, 2024 •

edited

Loading

vncoelho Jun 28, 2024 •

edited

Loading

vncoelho Jun 28, 2024 •

edited

Loading

dusmart commented Jul 2, 2024 •

edited

Loading

Jim8y commented Jul 16, 2024 •

edited

Loading