Improved scheme for Conflicts attribute storing and pricing #2913

AnnaShaleva · 2023-09-18T19:14:14Z

Close #2907. An alternative to #2908. This PR implements the #2907 (comment) and prevents all kind of attacks that were described earlier in #2907 and in #2818.

Two basic things are implemented in the current PR:

Improved storage scheme for native Ledger's conflicts records storing.
Customizable fee for Conflicts attributes.

As it was said in #2907 (comment), this scheme doesn't have the problems that #2908 has and doesn't have the #2907 (comment) issue as far.

Any related questions and comments are welcomed; we may accidentally miss something in design or implementation. We believe that this scheme is definitely more preferable than #2908.

Notes:

This PR changes the current T5 states (genesis state of native Policy and native Ledger state for those blocks that include conflicting transactions), thus requires full node resync.
This PR requires neo-modules upgrade, I have it locally and will create the corresponding PR a bit later.
This PR needs to be tested against the conflict attribute stops the network #2907 (comment).
It is recommended for Neo committee to set Conflicts attribute fee in native Policy contract via setConflictsFee to 0.02 GAS value (as suggested in conflict attribute stops the network #2907 (comment)) after new release is being rolled on T5 and mainnet.

This commit is a part of neo-project#2907: it implements conflict records storage scheme discribed in neo-project#2907 (comment). The short scheme description: Do not store the list of conflicting signers in the Ledger's conflict record value. Instead, put each conflicting signer in the conflict record key so that the reculting key is: {Prefix_Transaction, <conflict hash>, <signer>}, and the value is {<block index>}. As an optimisation, for each conflict record store the dummy stub where the key is {Prefix_Transaction, <conflict hash>} and the value is {<block index>}. This optimisation allows to reduce DB read requests during newly-pooled transaction verification for those transactions that do not have any on-chained conflicts. Also, IsTraceableBlock check is added for on-chain conflicts verification. It is needed to avoid situations when untraceable on-chained conflict affects the newly-pooled transaction verification. Signed-off-by: Anna Shaleva <[email protected]>

Malicious_OnChain_Conflict was constructed incorrectly (see the comment in the end of the test) and was replaced by the proper TestMaliciousOnChainConflict test in UT_Blockchain.cs way back ago in neo-project@0c06c91. Signed-off-by: Anna Shaleva <[email protected]>

This commit implements Conflicts attribute policy described in neo-project#2907 (comment). Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-09-18T19:16:23Z

src/Neo/SmartContract/Native/LedgerContract.cs

+                // Remove possible previously saved malicious conflict records for the transaction (if any).
+                foreach (var (key, _) in engine.Snapshot.Find(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash).ToArray()))
+                    engine.Snapshot.Delete(key);


The discussion started by @roman-khimov and ported from AnnaShaleva@deb1470#r127677374:

Not sure about this. It's garbage collection effectively. Can slow down tx processing.

That's true and everything will work without this line. This garbage collection may be safely run in a separate routine (or as a separate task), we don't need to include it into OnPersist, but I'm not sure what's the right place for it.

I'd say that's a job for RemoveUntraceableBlocks option (C# node doesn't have it, but it can be implemented). This storage space is paid for at the same time with the new setting, so keeping these entries is OK.

I think that we should have a maximum allowed signers too

Having maximum allowed signers will bring us to the same problem that was described in #2907 (comment). So any kind of such restriction leads to this possible attack when malicious conflicts prevent some real (and valid) conflict from entering the chain.

That's why we build this new solution in such way so that all conflicts data are properly paid. And it's even harder to spam the DB with Conflicts-related data than via contract storage.

it's not necessary to care about those "dust" records and they don't really hurt.

correct attr fee introduced by @shargon is able to avoid the abuse and restrict the storage cost.

an uncontrollable O(n) procedure here can be avoided

an uncontrollable O(n) procedure here can be avoided

I agree, let's then keep these garbage records. Implemented in a separate commit, see the 024a82a.

Upgrade neo-modules to use neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

dusmart · 2023-09-19T09:18:49Z

We believe that this scheme is definitely more preferable than #2908.

We genuinely appreciate @shargon 's enthusiasm for NEO and the dedicated efforts he has put forth. He swiftly opened the #2908 and consistently enhanced the solution based on everyone's input. Occasionally, focusing on surface-level issues without delving into their root causes and relying on habitual thinking for quick solutions can impede genuine problem-solving. I admire this PR for avoiding the use of limitations to resolve the issue.

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs

src/Neo/SmartContract/Native/PolicyContract.cs

src/Neo/Wallets/Wallet.cs

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913.

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-10-10T13:53:51Z

I'll rebase this branch onto current master in a few hours.

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

ConflictsFee logic was replaced by the generic attribute fee mechanism implemented in neo-project#2916. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-10-10T14:09:21Z

Current master is fetched, corresponding adjustments are done wrt #2916, ready for review.

It's OK to keep them and save O(n) operations during OnPersist. The storage taken by these malicious conflict records is properly paid anyway. See the discussion under neo-project#2913 (comment). Signed-off-by: Anna Shaleva <[email protected]>

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva · 2023-11-02T12:58:04Z

@superboyiii, do we need to test it? I've wrote a unit-test (included in the PR) and sure it works correctly, but maybe you also want to check.

@Liaojinghui, @vang1ong7ang, @dusmart, let's move on with this PR, we need to roll the 3.6.2 and this fix is needed to avoid the possible Conflicts attribute misuse.

superboyiii · 2023-11-03T05:33:20Z

@superboyiii, do we need to test it? I've wrote a unit-test (included in the PR) and sure it works correctly, but maybe you also want to check.

Sure, let me double check.

shargon · 2023-11-05T18:42:11Z

Wait for @superboyiii tests

superboyiii · 2023-11-06T09:05:42Z

src/Neo/SmartContract/Native/LedgerContract.cs

-                    var conflictRecord = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash),
-                        () => new StorageItem(new TransactionState { ConflictingSigners = Array.Empty<UInt160>() })).GetInteroperable<TransactionState>();
-                    conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).Distinct().ToArray();
+                    engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index }));


I tried to hack a private net by the code from #2907, when multi transactions want to cancel the same tx(the same conflict attribute), it will throw InvalidOperationException when they all change the same key's vale in the same block but it passed consensus. So finally it will be like:

It's my poor knowledge of NeoC# codebase, the intention here was to replace the existing conflict record (if it's in the DB), but engine.Snapshot.Add doesn't allow to do it. I'll fix it, thank you for testing!

@superboyiii, could you, please, run the same test? I've modified the code so that now it's possible to rewrite existing malicious conflict record with the proper transaction.

…torage `engine.Snapshot.Add` doesn't allow to rewrite storage entity if it's already exist. Thus, we firstly need to remove it and afterwards to add the updated entity. Signed-off-by: Anna Shaleva <[email protected]>

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

src/Neo/SmartContract/Native/LedgerContract.cs

Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add. Ref. neo-project#2913 (comment). Signed-off-by: Anna Shaleva <[email protected]>

Jim8y · 2023-11-07T13:22:25Z

@superboyiii mind to test again?

superboyiii · 2023-11-08T03:00:27Z

@superboyiii mind to test again?

Never mind :)

superboyiii

Now it works well! Pass two-hours malicious pressure test.

Jim8y · 2023-11-08T07:20:46Z

@shargon Good to go

* DBFTPlugin: adapt new Conflicts attribute storage scheme Upgrade neo-modules to use neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]> * Update neo * Fix version * Fix NativeHistory --------- Signed-off-by: Anna Shaleva <[email protected]> Co-authored-by: Jimmy <[email protected]> Co-authored-by: Shargon <[email protected]>

AnnaShaleva added 3 commits September 18, 2023 22:14

Policy: introduce fee for Conflicts attribute

cba97f1

This commit implements Conflicts attribute policy described in neo-project#2907 (comment). Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva force-pushed the fix-conflicts branch from d9e90c6 to cba97f1 Compare September 18, 2023 19:15

AnnaShaleva commented Sep 18, 2023

View reviewed changes

roman-khimov mentioned this pull request Sep 18, 2023

Neo v3.6.2 plan #2914

Closed

AnnaShaleva mentioned this pull request Sep 18, 2023

conflict attribute stops the network #2907

Closed

AnnaShaleva added a commit to AnnaShaleva/neo-modules that referenced this pull request Sep 19, 2023

DBFTPlugin: adopt new Conflicts attribute storage scheme

9257cee

Upgrade neo-modules to use neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva added a commit to AnnaShaleva/neo-modules that referenced this pull request Sep 19, 2023

DBFTPlugin: adapt new Conflicts attribute storage scheme

1efec59

Upgrade neo-modules to use neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva mentioned this pull request Sep 19, 2023

DBFTPlugin: adapt new Conflicts attribute storage scheme neo-project/neo-modules#828

Merged

shargon mentioned this pull request Sep 19, 2023

Set max persisted conflicts signers #2908

Closed

shargon reviewed Sep 19, 2023

View reviewed changes

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

shargon reviewed Sep 19, 2023

View reviewed changes

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

shargon mentioned this pull request Sep 19, 2023

Set attribute fee #2916

Merged

shargon reviewed Sep 19, 2023

View reviewed changes

src/Neo/Wallets/Wallet.cs Outdated Show resolved Hide resolved

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 21, 2023

core: improve conflict records storage scheme

ee09eb8

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 21, 2023

core: improve conflict records storage scheme

56e183d

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023

core: improve conflict records storage scheme

e208e10

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023

core: improve conflict records storage scheme

150bc86

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Sep 22, 2023

core: improve conflict records storage scheme

6aa3a67

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva mentioned this pull request Sep 25, 2023

*: refactor Notary conflict records storage scheme nspcc-dev/neo-go#3138

Merged

Merge branch 'master' into fix-conflicts

ffa7e8d

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 10, 2023

core: improve conflict records storage scheme

88ff18f

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva added 2 commits October 10, 2023 16:59

Merge branch 'master' of https://github.com/neo-project/neo into fix-…

fff3129

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

*: remove remnants of ConflictsFee in native Policy

330cbea

ConflictsFee logic was replaced by the generic attribute fee mechanism implemented in neo-project#2916. Signed-off-by: Anna Shaleva <[email protected]>

AnnaShaleva force-pushed the fix-conflicts branch from 87e6d92 to 024a82a Compare October 10, 2023 14:17

AnnaShaleva requested review from shargon, roman-khimov and vang1ong7ang October 10, 2023 14:19

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Oct 10, 2023

core: improve conflict records storage scheme

32bfed4

Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>

shargon previously approved these changes Oct 20, 2023

View reviewed changes

Merge branch 'master' into fix-conflicts

51a51a1

Merge branch 'master' into fix-conflicts

be19957

Jim8y previously approved these changes Nov 5, 2023

View reviewed changes

Merge branch 'master' into fix-conflicts

25b7d1b

superboyiii reviewed Nov 6, 2023

View reviewed changes

Jim8y and others added 2 commits November 7, 2023 18:33

Merge branch 'master' into fix-conflicts

2350393

AnnaShaleva dismissed stale reviews from Jim8y and shargon via 88aab31 November 7, 2023 10:58

Throw proper exception if TestMaliciousOnChainConflict fails

32d1341

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

AnnaShaleva force-pushed the fix-conflicts branch from 88aab31 to 32d1341 Compare November 7, 2023 11:00

AnnaShaleva commented Nov 7, 2023

View reviewed changes

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

Optimize conflicts records storing

9845609

Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add. Ref. neo-project#2913 (comment). Signed-off-by: Anna Shaleva <[email protected]>

superboyiii approved these changes Nov 8, 2023

View reviewed changes

Jim8y approved these changes Nov 8, 2023

View reviewed changes

Merge branch 'master' into fix-conflicts

fc455f4

shargon merged commit 8d270e6 into neo-project:master Nov 8, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved scheme for Conflicts attribute storing and pricing #2913

Improved scheme for Conflicts attribute storing and pricing #2913

AnnaShaleva commented Sep 18, 2023

AnnaShaleva Sep 18, 2023

AnnaShaleva Sep 18, 2023

roman-khimov Sep 19, 2023

shargon Sep 19, 2023

AnnaShaleva Sep 19, 2023

vang1ong7ang Sep 22, 2023

AnnaShaleva Oct 10, 2023

dusmart commented Sep 19, 2023

AnnaShaleva commented Oct 10, 2023

AnnaShaleva commented Oct 10, 2023

AnnaShaleva commented Nov 2, 2023

superboyiii commented Nov 3, 2023

shargon commented Nov 5, 2023

superboyiii Nov 6, 2023 •

edited

Loading

AnnaShaleva Nov 7, 2023

AnnaShaleva Nov 7, 2023

Jim8y commented Nov 7, 2023

superboyiii commented Nov 8, 2023

superboyiii left a comment •

edited

Loading

Jim8y commented Nov 8, 2023

Improved scheme for Conflicts attribute storing and pricing #2913

Improved scheme for Conflicts attribute storing and pricing #2913

Conversation

AnnaShaleva commented Sep 18, 2023

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

dusmart commented Sep 19, 2023

AnnaShaleva commented Oct 10, 2023

AnnaShaleva commented Oct 10, 2023

AnnaShaleva commented Nov 2, 2023

superboyiii commented Nov 3, 2023

shargon commented Nov 5, 2023

superboyiii Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Nov 7, 2023

superboyiii commented Nov 8, 2023

superboyiii left a comment • edited Loading

Choose a reason for hiding this comment

Jim8y commented Nov 8, 2023

superboyiii Nov 6, 2023 •

edited

Loading

superboyiii left a comment •

edited

Loading