Payloads: implement Conflicts attribute by AnnaShaleva · Pull Request #2818 · neo-project/neo

AnnaShaleva · 2022-09-27T05:57:41Z

Closes #1991. Partially based on #2661, but verification-related logic is refactored to match the initial behaviour design.

Depends on #2812.

Jim8y · 2023-03-04T00:10:22Z

@shargon @AnnaShaleva This pr is in 3.6 list, may you please update and review it.

roman-khimov · 2023-03-06T07:40:42Z

src/Neo/Ledger/MemoryPool.cs

+                var persisted = block.Transactions.Select(t => t.Hash);
+                var stale = new List<UInt256>();
+                foreach (var item in _sortedTransactions)
+                {
+                    if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes<Conflicts>().Select(a => a.Hash).Intersect(persisted).Count() > 0)
+                    {
+                        stale.Add(item.Tx.Hash);
+                        conflictingItems.Add(item.Tx);
+                    }
+                }
+                foreach (var h in stale)
+                {
+                    if (!TryRemoveVerified(h, out _)) TryRemoveUnVerified(h, out _);
+                }


IIRC LINQ is somewhat slower, but I'm OK either way. @shargon?

src/Neo/Ledger/MemoryPool.cs

src/Neo/Ledger/TransactionVerificationContext.cs

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

Fix the comment neo-project#2818 (comment).

Closes neo-project#1991.

Fix the comment neo-project#2818 (comment).

AnnaShaleva · 2023-03-12T17:13:20Z

Rebased onto current master, ready for review.

AnnaShaleva · 2023-04-06T07:43:43Z

@shargon, @Liaojinghui, @vncoelho, let's review this PR one more time and move on with it, the previously added conversations are fixed.

src/Neo/Ledger/TransactionVerificationContext.cs

vncoelho

I am reviewing again, can you clarify the trimmed a little bit more to me?

Fix Vitor's comments.

AnnaShaleva · 2023-04-06T14:44:42Z

can you clarify the trimmed a little bit more

Sure. Consider the situation when transaction A has the Conflicts attribute with hash of transaction B specified inside this attribute. Consider the transaction A is being accepted to the chain in some block whereas transaction B is not pooled to the network yet. Then the transaction A should be stored in the DB/Ledger contract in a common way. Along with that, we extract the B's hash from the Conflicts attribute of transaction A and do store the "dummy" transaction B inside the DB/Ledger contract. It is done to prevent transaction B from being included into one of the subsequent blocks since transaction B conflicts with transaction A. Note that the only thing we know about transaction B is its hash. But how do we store the transaction B if we know only its hash? We use Trimmed field for that. Basically, the Trimmed field means that there's only transaction's hash is being stored.

AnnaShaleva · 2023-08-24T12:29:16Z

@shargon, take a look at the latest commit, please. Turns out I've accidentally missed this patch when I was fixing our #2818 (comment) conversation. This patch should be a part of fd1748d commit actually (and I did all the things right in NeoGo in nspcc-dev/neo-go@ee4b8f8#diff-aec93d55c952fa59b011c31e50ccae3fb3303260537d58df6a70309bb94af622R551-R560).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2023-08-24T13:11:20Z

@shargon, take a look at the test, please: b6856ff. I've adjusted it, the idea is to persist conflicting tx1 with malicious signer on chain and after that to pool the original tx2 (with different signer), we expect it to be pooled successfully. But I can't properly organize the test, I've got InsufficientFunds while adding tx2 (it's not connected with Conflicts logic, I just don't know how to add enough GAS to the tx2 sender account).

@AnnaShaleva is this UT or a similar one added?

BTW, this scenario was tested by @superboyiii, IINM, it was passed OK. And we have a set of tests in NeoGo testing that on-chain conflicts work as expected, see the tests added in nspcc-dev/neo-go@ee4b8f8#diff-aec93d55c952fa59b011c31e50ccae3fb3303260537d58df6a70309bb94af622R551-R560.

src/Neo/SmartContract/Native/TransactionState.cs

cschuchardt88 · 2023-08-27T01:16:40Z

I just don't know how to add enough GAS to the tx2 sender account).

Check out for adding gas lines 46 to 51

neo/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs

Line 48 in 773b96a

var key = new KeyBuilder(NativeContract.GAS.Id, 20).Add(acc.ScriptHash);

Move it to the UT_Blockchain.cs file and refactor it a bit to make it actually pass as expected. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva · 2023-08-28T16:12:58Z

@shargon, I've managed to fix the InsuficientFunds error in Malicious_OnChain_Conflict, please, see the updated test. Now it passes as expected.

superboyiii · 2023-09-01T04:22:33Z

If this PR can't be merged before 4th Sep, we will release v3.6.0 directly. Too much time for waiting. We can add this to v3.7.0.
https://github.com/orgs/neo-project/teams/core @vncoelho @roman-khimov @AnnaShaleva @Liaojinghui @shargon @devhawk

AnnaShaleva · 2023-09-01T05:09:49Z

It’s ready for review, all issues are fixed.

src/Neo/SmartContract/Native/LedgerContract.cs

Co-authored-by: Shargon <shargon@gmail.com>

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

src/Neo/Ledger/MemoryPool.cs

Fix neo-project#2818 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

superboyiii · 2023-09-04T02:09:29Z

@shargon Need your merge.

No change need to make and need to perpare for merge

* DBFTPlugin: adapt Conflicts attribute verification Depends on neo-project/neo#2818, see the neo-project/neo#2818 (comment). Signed-off-by: Anna Shaleva <anna@nspcc.ru> * Fix Conflicts attribute verification Fetch the update from core: neo-project/neo#2818 (comment). * Fetch on-chain conflict signer fix from core Use new API from neo-project/neo@ee7333f. --------- Signed-off-by: Anna Shaleva <anna@nspcc.ru> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com> Co-authored-by: Owen Zhang <38493437+superboyiii@users.noreply.github.com> Co-authored-by: Jimmy <jinghui@wayne.edu>

AnnaShaleva force-pushed the conflicts branch from 30c6461 to 7d75211 Compare September 27, 2022 06:01

roman-khimov mentioned this pull request Feb 2, 2023

Neo v3.6.0 Checklist #2844

Closed

mialbu mentioned this pull request Feb 9, 2023

Neo v3.6.0 neow3j/neow3j#916

Closed

12 tasks

roman-khimov reviewed Mar 6, 2023

View reviewed changes

shargon reviewed Mar 7, 2023

View reviewed changes

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

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

shargon reviewed Mar 7, 2023

View reviewed changes

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

AnnaShaleva added a commit to AnnaShaleva/neo that referenced this pull request Mar 9, 2023

Fix conflicting tx fees accounting

e0a44e8

Fix the comment neo-project#2818 (comment).

AnnaShaleva added a commit to AnnaShaleva/neo that referenced this pull request Mar 9, 2023

Fix conflicting tx fees accounting

8a041a6

Fix the comment neo-project#2818 (comment).

AnnaShaleva force-pushed the conflicts branch from e0a44e8 to 8a041a6 Compare March 9, 2023 11:16

AnnaShaleva requested review from Jim8y, roman-khimov and shargon and removed request for Jim8y, roman-khimov and shargon March 9, 2023 11:28

AnnaShaleva and others added 3 commits March 12, 2023 20:11

Payloads: implement Conflicts attribute

77d823c

Closes neo-project#1991.

Update src/Neo/Ledger/MemoryPool.cs

e42dd05

Fix conflicting tx fees accounting

9c4bedf

Fix the comment neo-project#2818 (comment).

AnnaShaleva force-pushed the conflicts branch from 8a041a6 to 9c4bedf Compare March 12, 2023 17:12

vncoelho reviewed Apr 6, 2023

View reviewed changes

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

vncoelho reviewed Apr 6, 2023

View reviewed changes

Reformat code and improve variables naming

4d54d60

Fix Vitor's comments.

AnnaShaleva requested a review from Jim8y April 6, 2023 14:44

AnnaShaleva dismissed Jim8y’s stale review via efa8f57 August 24, 2023 12:26

MemoryPool: add test for on-chain conflict

b6856ff

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

Clean using

9460db6

shargon reviewed Aug 26, 2023

View reviewed changes

src/Neo/SmartContract/Native/TransactionState.cs Show resolved Hide resolved

Refactor Malicious_OnChain_Conflict test

0c06c91

Move it to the UT_Blockchain.cs file and refactor it a bit to make it actually pass as expected. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

AnnaShaleva requested a review from shargon August 28, 2023 16:15

Merge branch 'master' into conflicts

4df221d

shargon reviewed Sep 1, 2023

View reviewed changes

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

Use Distinct to filter out duplicating conflicting on-chain signers

f7c3acb

Co-authored-by: Shargon <shargon@gmail.com>

shargon reviewed Sep 1, 2023

View reviewed changes

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

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

Use HashSet as a container of conflicting hashes in the mempool

2efa3b2

Fix neo-project#2818 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

shargon approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'master' into conflicts

9dbc60e

Jim8y approved these changes Sep 1, 2023

View reviewed changes

shargon merged commit 6398801 into neo-project:master Sep 4, 2023

roman-khimov mentioned this pull request Sep 15, 2023

conflict attribute stops the network #2907

Closed

shargon mentioned this pull request Sep 15, 2023

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

Closed

dusmart mentioned this pull request Sep 18, 2023

checklist of 3.6.0 PRs that will cause FORK #2910

Closed

5 tasks

AnnaShaleva mentioned this pull request Sep 18, 2023

Improved scheme for Conflicts attribute storing and pricing #2913

Merged

superboyiii mentioned this pull request Oct 10, 2023

add cancel command for conflict attribute neo-project/neo-node#903

Merged

vang1ong7ang mentioned this pull request Mar 26, 2024

Implement NotaryAssisted transaction attribute #3175

Merged

12 tasks

Conversation

AnnaShaleva commented Sep 27, 2022

Uh oh!

Jim8y commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roman-khimov Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnnaShaleva commented Mar 12, 2023

Uh oh!

AnnaShaleva commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva commented Apr 6, 2023

Uh oh!

AnnaShaleva commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cschuchardt88 commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaShaleva commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superboyiii commented Sep 1, 2023

Uh oh!

AnnaShaleva commented Sep 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

superboyiii commented Sep 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Jim8y commented Mar 4, 2023 •

edited

Loading

AnnaShaleva commented Apr 6, 2023 •

edited

Loading

AnnaShaleva commented Aug 24, 2023 •

edited

Loading

AnnaShaleva commented Aug 24, 2023 •

edited

Loading

cschuchardt88 commented Aug 27, 2023 •

edited

Loading

AnnaShaleva commented Aug 28, 2023 •

edited

Loading