Set max persisted conflicts signers #2908

shargon · 2023-09-15T13:11:32Z

Partially fix of #2907

First step, ledger fix
Second step, memory pool fix ( @AnnaShaleva thanks for it)

src/Neo/Network/P2P/Payloads/Conflicts.cs

src/Neo/Network/P2P/Payloads/Transaction.cs

src/Neo/Network/P2P/Payloads/Conflicts.cs

AnnaShaleva · 2023-09-15T13:39:34Z

src/Neo/Network/P2P/Payloads/Conflicts.cs

+            // Check already stored signers
+
+            var conflictingSigners = tx.Signers.Select(s => s.Account);
+
+            foreach (var attr in tx.GetAttributes<Conflicts>())
+            {
+                var conflictRecord = snapshot.TryGet(
+                    new KeyBuilder(NativeContract.Ledger.Id, LedgerContract.Prefix_Transaction).Add(attr.Hash))?
+                    .GetInteroperable<TransactionState>();
+                if (conflictRecord == null) continue;
+
+                if (conflictRecord.ConflictingSigners.Concat(conflictingSigners).Distinct().Count() > 100)
+                {
+                    return false;
+                }
+            }
+


The direction is right, this will fix the problem (if, of course, the true source of the problem is the conflicting signers list overflow). We need to test this patch against the nice scenario provided by @vang1ong7ang in #2907 (comment).

src/Neo/Network/P2P/Payloads/Conflicts.cs

dusmart · 2023-09-15T13:51:44Z

By the way, I think this PR's idea will solve this golang-C# inconsistency indeed if golang follows. #2907 (comment)

While I'm not sure if it can prevent the entire issue of #2907 if no mempool change is made.

vang1ong7ang · 2023-09-15T13:55:15Z

I think @AnnaShaleva knows what happened and works on the right direction

vang1ong7ang · 2023-09-15T13:57:35Z

Adding over-tight restrictions doesn't completely solve the problem, it just makes the functionality worse and worse.

AnnaShaleva · 2023-09-15T14:01:40Z

By the way, I think this PR will solve this golang-C# inconsistency indeed if golang follows. #2907 (comment)

First of all, I'll port this MaxConflictingSigners constraint to our node ASAP.

And actually, this problem with getrawtransaction is a bit deeper than it seems, because getrawtransaction itself must not be bothered by any conflicting staff. These conflict records just presented as "dummy" transactions inside the native Ledger and expected to be unretrievable by getrawtransaction. And we have this code:

neo/src/Neo/SmartContract/Native/LedgerContract.cs

Lines 263 to 268 in 5207956

    
           private Transaction GetTransactionForContract(ApplicationEngine engine, UInt256 hash) 
        
           { 
        
               TransactionState state = GetTransactionState(engine.Snapshot, hash); 
        
               if (state is null || !IsTraceableBlock(engine.Snapshot, state.BlockIndex, engine.ProtocolSettings.MaxTraceableBlocks)) return null; 
        
               return state.Transaction; 
        
           }

Which uses GetTransactionState that retrieves all (both proper and dummy) transactions:

neo/src/Neo/SmartContract/Native/LedgerContract.cs

Lines 244 to 249 in 5207956

    
           public TransactionState GetTransactionState(DataCache snapshot, UInt256 hash) 
        
           { 
        
               var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable<TransactionState>(); 
        
               if (state?.Transaction is null) return null; 
        
               return state; 
        
           }

And likely throws the exception on deserialization (right before the Transaction field check).

Co-authored-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-09-15T14:03:10Z

Adding tighter restrictions doesn't completely solve the problem, it just makes the functionality worse and worse.

@vang1ong7ang, why do you think so?

vang1ong7ang · 2023-09-15T14:30:30Z

Adding tighter restrictions doesn't completely solve the problem, it just makes the functionality worse and worse.

@vang1ong7ang, why do you think so?

max attr per tx / max signers per tx / max conflict per tx / ... are not something critical, reducing them only increase a little bit the cost of the hacker.

shargon · 2023-09-15T16:04:40Z

Ready to merge @roman-khimov @Liaojinghui could you review it?
We will open a new one for fix the memory pool issue

vncoelho

Let's wait for the other PR, test both together and merge both of them solving the issue.

On Monday I can try to prepare a temporary network similar to testnet that we can all test.

dusmart · 2023-09-15T16:39:49Z

Ready to merge @roman-khimov @Liaojinghui could you review it?
We will open a new one for fix the memory pool issue

To be responsible, only collecting approvals is not enough for the merge. Build your 7-node private testnet and follow the instructions at #2907, only if you can prove to others that it can not break the testnet then is the merge deemed good.

I think that’s why Vang provides the details. It’s for good purposes like this.

Ignore me if you developers have your own ways. These words comes because I feel unfair for Vang when you want him to build a private environment while you won’t.

…ict record Instead of restricting the maximum number of conflicting signers per conflict record we can restrict the number of transactions that make their contribution to this conflict record. The constraint nature is the same, it doesn't allow to spam the chain with a large set of transactions with the same Conflicts attribute. However, this approach has several benefits over restricting the number of signers for these conflicting transactions: 1. It significantly simplifies and accelerates a newly-pooled transactions verification. We don't need to perform this `Concat(signers).Distinct().Count()` operation anymore on new transaction addition. 2. It has a space for improvement which is not presented in this commit, but have to be implemented if the current solution will be accepted by the core developers. A space for improvement is the following: during Conflicts attribute verification we don't actually need to retrieve the whole list of conflicting signers for the conflict record. What we need instead is to check the `ConflictingTransactionsCount` field only. Thus, we may safely omit the `ConflictingSigners` deserialisation from stack item. 3. It allows to easily port the same constraint to the mempool (see the further commit) which, in turn, roughly restrict the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. At the same time, the overhead the current commit brings in the `TransactionState` is quite insignificant (it's only a single extra Integer field). Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on upgrade. States will differs from the current 3.6.0 T5. Note 2: this PR (as far as neo-project#2908) doesn't help with those transactions that already were included into blocks 2690038-2690040 of the current T5 due to the neo-project#2907 (comment). We need to have a separate discussion on these "malicious" blocks and decide how to handle them. However, this PR doesn't ever prevent the node from the proper processing of these blocks on resync, the blocks will be processed successfully (with HALT state, the same as they were processed by the 3.6.0 release). Signed-off-by: Anna Shaleva <[email protected]>

Instead of restricting the maximum number of conflicting signers per conflict record we can restrict the number of transactions that make their contribution to this conflict record. The constraint nature is the same, it doesn't allow to spam the chain with a large set of transactions with the same Conflicts attribute. However, this approach has several benefits over restricting the number of signers for these conflicting transactions: 1. It significantly simplifies and accelerates a newly-pooled transactions verification. We don't need to perform this `Concat(signers).Distinct().Count()` operation anymore on new transaction addition. 2. It has a space for improvement which is not presented in this commit, but have to be implemented if the current solution will be accepted by the core developers. A space for improvement is the following: during Conflicts attribute verification we don't actually need to retrieve the whole list of conflicting signers for the conflict record. What we need instead is to check the `ConflictingTransactionsCount` field only. Thus, we may safely omit the `ConflictingSigners` deserialisation from stack item. 3. It allows to easily port the same constraint to the mempool (see the further commit) which, in turn, roughly restrict the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. At the same time, the overhead the current commit brings in the `TransactionState` is quite insignificant (it's only a single extra Integer field). Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on upgrade. States will differ from the current 3.6.0 T5. Note 2: this PR (as far as neo-project#2908) doesn't help with those transactions that already were included into blocks 2690038-2690040 of the current T5 due to the neo-project#2907 (comment). We need to have a separate discussion on these "malicious" blocks and decide how to handle them. However, this PR doesn't ever prevent the node from the proper processing of these blocks on resync, the blocks will be processed successfully (with HALT state, the same as they were processed by the 3.6.0 release). Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-09-17T16:02:59Z

I've created the second part of this PR, please, check out the #2911. See also #2907 (comment) comment.

src/Neo/SmartContract/Native/LedgerContract.cs

…lict record (#2911) * Revert "Sort and limit size" This reverts commit 2a69047. As it was said in #2909 (comment), we can't save only a part of on-chained conflicting signers. Instead, we need to construct new blocks in such way so that there were no conflicting signers limit overflow. It's not hard to implement, see the further commits. Signed-off-by: Anna Shaleva <[email protected]> * Ledger: restrict max number of conflicting txs per conflict record Instead of restricting the maximum number of conflicting signers per conflict record we can restrict the number of transactions that make their contribution to this conflict record. The constraint nature is the same, it doesn't allow to spam the chain with a large set of transactions with the same Conflicts attribute. However, this approach has several benefits over restricting the number of signers for these conflicting transactions: 1. It significantly simplifies and accelerates a newly-pooled transactions verification. We don't need to perform this `Concat(signers).Distinct().Count()` operation anymore on new transaction addition. 2. It has a space for improvement which is not presented in this commit, but have to be implemented if the current solution will be accepted by the core developers. A space for improvement is the following: during Conflicts attribute verification we don't actually need to retrieve the whole list of conflicting signers for the conflict record. What we need instead is to check the `ConflictingTransactionsCount` field only. Thus, we may safely omit the `ConflictingSigners` deserialisation from stack item. 3. It allows to easily port the same constraint to the mempool (see the further commit) which, in turn, roughly restrict the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. At the same time, the overhead the current commit brings in the `TransactionState` is quite insignificant (it's only a single extra Integer field). Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on upgrade. States will differ from the current 3.6.0 T5. Note 2: this PR (as far as #2908) doesn't help with those transactions that already were included into blocks 2690038-2690040 of the current T5 due to the #2907 (comment). We need to have a separate discussion on these "malicious" blocks and decide how to handle them. However, this PR doesn't ever prevent the node from the proper processing of these blocks on resync, the blocks will be processed successfully (with HALT state, the same as they were processed by the 3.6.0 release). Signed-off-by: Anna Shaleva <[email protected]> * MemoryPool: restrict max number of conflicting txs per conflict record Port the Ledger conflicting tx restriction logic to the MemoryPool: the maximum number of mempooled transactions that has the same Conflicts attribute (i.e. those that conflict with the same transaction) is restricted by the LedgerContract.MaxAllowedConflictingTransactions constant. This restriction, combined with the existing Ledger's storage restriction, roughly restricts the overall possible number of transactions per conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number of signers per conflict record in the worst case equals to 2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480. Signed-off-by: Anna Shaleva <[email protected]> --------- Signed-off-by: Anna Shaleva <[email protected]>

src/Neo/Ledger/MemoryPool.cs

shargon · 2023-09-19T10:30:09Z

Closed in favor of #2913

Set max conflicts attributes

53e77d4

shargon requested review from superboyiii and Jim8y September 15, 2023 13:12

comment

7180b68

roman-khimov reviewed Sep 15, 2023

View reviewed changes

src/Neo/Network/P2P/Payloads/Conflicts.cs Outdated Show resolved Hide resolved

src/Neo/Network/P2P/Payloads/Transaction.cs Outdated Show resolved Hide resolved

AnnaShaleva reviewed Sep 15, 2023

View reviewed changes

src/Neo/Network/P2P/Payloads/Conflicts.cs Outdated Show resolved Hide resolved

shargon added 4 commits September 15, 2023 15:25

Check already stored signers

be2e694

Optimize check

46bb8c4

change visibility

604f01a

Fix UT

b4d401b

dusmart reviewed Sep 15, 2023

View reviewed changes

src/Neo/Network/P2P/Payloads/Conflicts.cs Outdated Show resolved Hide resolved

src/Neo/Network/P2P/Payloads/Conflicts.cs Show resolved Hide resolved

src/Neo/Network/P2P/Payloads/Conflicts.cs Outdated Show resolved Hide resolved

shargon changed the title ~~Set max conflicts attributes~~ Set max conflicts signers Sep 15, 2023

shargon requested review from AnnaShaleva, dusmart and roman-khimov September 15, 2023 13:39

Define MaxConflictSigners

56ad6c9

AnnaShaleva reviewed Sep 15, 2023

View reviewed changes

Update src/Neo/Network/P2P/Payloads/Conflicts.cs

c210775

Co-authored-by: Anna Shaleva <[email protected]>

shargon added 4 commits September 15, 2023 16:08

AnnaShaleva feedback

3663150

Mover to Ledger

acca094

Revert AllowedCount

ae44f91

Revert comment

8e71c9c

shargon changed the title ~~Set max conflicts signers~~ Set max persisted conflicts signers Sep 15, 2023

Just to be more precise.

0940160

shargon requested a review from AnnaShaleva September 15, 2023 16:04

shargon added 3 commits September 15, 2023 18:08

Ensure that Hash is not repeated in the same transaction

73d8e6b

Clean code

4485fc8

Change comment

41171b3

vncoelho reviewed Sep 15, 2023

View reviewed changes

AnnaShaleva mentioned this pull request Sep 15, 2023

Set max persisted conflicts signers (2/2) #2909

Closed

Sort and limit size

2a69047

This was referenced Sep 17, 2023

Restrict maximum number of conflicting transactions per Ledger's conflict record #2911

Merged

conflict attribute stops the network #2907

Closed

vang1ong7ang reviewed Sep 18, 2023

View reviewed changes

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

shargon commented Sep 18, 2023

View reviewed changes

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

AnnaShaleva mentioned this pull request Sep 18, 2023

Improved scheme for Conflicts attribute storing and pricing #2913

Merged

roman-khimov mentioned this pull request Sep 18, 2023

Neo v3.6.2 plan #2914

Closed

shargon closed this Sep 19, 2023

shargon deleted the max-conflicts branch September 19, 2023 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set max persisted conflicts signers #2908

Set max persisted conflicts signers #2908

shargon commented Sep 15, 2023 •

edited

Loading

AnnaShaleva Sep 15, 2023

dusmart commented Sep 15, 2023 •

edited

Loading

vang1ong7ang commented Sep 15, 2023

vang1ong7ang commented Sep 15, 2023 •

edited

Loading

AnnaShaleva commented Sep 15, 2023

AnnaShaleva commented Sep 15, 2023

vang1ong7ang commented Sep 15, 2023

shargon commented Sep 15, 2023

vncoelho left a comment •

edited

Loading

dusmart commented Sep 15, 2023 •

edited

Loading

AnnaShaleva commented Sep 17, 2023

shargon commented Sep 19, 2023

Set max persisted conflicts signers #2908

Set max persisted conflicts signers #2908

Conversation

shargon commented Sep 15, 2023 • edited Loading

AnnaShaleva Sep 15, 2023

Choose a reason for hiding this comment

dusmart commented Sep 15, 2023 • edited Loading

vang1ong7ang commented Sep 15, 2023

vang1ong7ang commented Sep 15, 2023 • edited Loading

AnnaShaleva commented Sep 15, 2023

AnnaShaleva commented Sep 15, 2023

vang1ong7ang commented Sep 15, 2023

shargon commented Sep 15, 2023

vncoelho left a comment • edited Loading

Choose a reason for hiding this comment

dusmart commented Sep 15, 2023 • edited Loading

AnnaShaleva commented Sep 17, 2023

shargon commented Sep 19, 2023

shargon commented Sep 15, 2023 •

edited

Loading

dusmart commented Sep 15, 2023 •

edited

Loading

vang1ong7ang commented Sep 15, 2023 •

edited

Loading

vncoelho left a comment •

edited

Loading

dusmart commented Sep 15, 2023 •

edited

Loading