core: check the signers of on-chained conflicting transaction during new transaction verification #3061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've checked this in neo-bench as far (https://github.com/nspcc-dev/neo-bench/tree/test-conflicts), works as expected: |
AnnaShaleva
force-pushed
the
check-onchain-conflicts
branch
2 times, most recently
from
34ec8c9
to
c65ae76
Compare
Codecov Report
@@ Coverage Diff @@
## master #3061 +/- ##
=======================================
Coverage 84.64% 84.64%
=======================================
Files 329 329
Lines 43792 43846 +54
=======================================
+ Hits 37069 37115 +46
- Misses 5220 5226 +6
- Partials 1503 1505 +2
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
AnnaShaleva
force-pushed
the
check-onchain-conflicts
branch
from
c65ae76
to
9b9baef
Compare
|
I'd like to optimize our storage scheme a bit to save a couple of bytes per conflicting record. Thus, not ready for review yet. |
AnnaShaleva
force-pushed
the
check-onchain-conflicts
branch
from
9b9baef
to
133082e
Compare
|
Optimisation is done, ready for review. |
roman-khimov
requested changes
Jul 20, 2023
carpawell
reviewed
Jul 20, 2023
`(*Blockchain).HasTransaction` is one of the oldest methods in our codebase, and currently it's completely unused. I also doubt that this method works as expected because it returns `true` if transaction in the mempool. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Witnesses are not yet created by the moment we return this error, thus, it was always 0 as an actual number of witnesses in ErrInvalidWitnessNum. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva
force-pushed
the
check-onchain-conflicts
branch
from
133082e
to
30f2def
Compare
During new transaction verification if there's an on-chain conflicting transaction, we should check the signers of this conflicting transaction. If the signers intersect with signers of the incoming transaction, then the conflict is treated as valid and verification for new incoming transaction should fail. Otherwise, the conflict is treated as the malicious attack attempt and will not be taken into account; verification for the new incoming transaction should continue. This commint implements the scheme described at neo-project/neo#2818 (comment), thanks to @shargon for digging. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva
force-pushed
the
check-onchain-conflicts
branch
from
30f2def
to
ee4b8f8
Compare
roman-khimov
approved these changes
Jul 21, 2023
AnnaShaleva
added a commit
to AnnaShaleva/neo
that referenced
this pull request
Aug 24, 2023
This patch should be a part of fd1748d, but was accidentally skipped. This patch matches exactly the NeoGo behaviour that was added in nspcc-dev/neo-go#3061 and that was discussed earlier in neo-project#2818 (comment). Fix the issue described in neo-project#2818 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
shargon
added a commit
to neo-project/neo
that referenced
this pull request
Sep 4, 2023
* Payloads: implement Conflicts attribute Closes #1991. * Update src/Neo/Ledger/MemoryPool.cs * Fix conflicting tx fees accounting Fix the comment #2818 (comment). * Reformat code and improve variables naming Fix Vitor's comments. * Make comments more clear and adjust variables naming Fix Owen's feedback. * Fix Conflicts attribute verification Consider the case when transaction B has the hash of transaction A in the Conflicts attribute and transaction B is on-chain. Let the transaction C has the hash of transaction A in the Conflicts attribute. Then transaction C still should be able to pass verification and should be accepted to the subsequent block, because transaction A isn't on chain and transaction A will never be accepted. Thanks to Owen for testing, this case is described at the #2818 (review). The expected behaviour in the described case is that TXID-D and TXID-E will be successfully accepted to the mempool and to chain, because the conflicting TXID-A is not on chain (and it's OK that the hash of TXID-A is on-chain as the conflicting hash). * Fix formatting, comments and add a testcase for conflicting mempool txs * Add one more testcase for conflicting transactions Testcase provided by Vitor in #2818 (comment). * Implement economic adjustments for Conflicts attribute Transaction that conflicts with mempooled transactions have to pay larger network fee than the sum of all conflicting transactions in the pool that have the same sender as the newcomer. Port the nspcc-dev/neo-go#3031. * Remove Trimmed value * Check signers of on-chained conflict during new tx verification Fix the problem described in #2818 (review). During new transaction verification if there's an on-chain conflicting transaction, we should check the signers of this conflicting transaction. If the signers contain payer of the incoming transaction, then the conflict is treated as valid and verification for new incoming transaction should fail. Otherwise, the conflict is treated as the malicious attack attempt and will not be taken into account; verification for the new incoming transaction should continue in this case. * Properly handle duplicating Conflicts hashes from the persisted transactions * Store signers of all conflicting on-chain transactions with the same Conflicts attribute Store not only signers of the latest conflicting on-chain transaction, but signers of all conflicting transactions with the same attribute. * MemoryPool: consider signers intersection on Conflicts check This patch should be a part of fd1748d, but was accidentally skipped. This patch matches exactly the NeoGo behaviour that was added in nspcc-dev/neo-go#3061 and that was discussed earlier in #2818 (comment). Fix the issue described in #2818 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * MemoryPool: add test for on-chain conflict Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> * Clean using * Refactor Malicious_OnChain_Conflict test 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> * Use Distinct to filter out duplicating conflicting on-chain signers Co-authored-by: Shargon <shargon@gmail.com> * Use HashSet as a container of conflicting hashes in the mempool Fix #2818 (comment). Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> --------- Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru> Co-authored-by: Shargon <shargon@gmail.com> Co-authored-by: Vitor Nazário Coelho <vncoelho@gmail.com> Co-authored-by: Jimmy <jinghui@wayne.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
On-chain conflicting transaction is treated as "good" and valid by default, which may prevent the main transaction from entering the chain in case of an attack. The problem is described by @shargon in neo-project/neo#2818 (comment).
Solution
Check the signers of conflicting transaction and treat conlflict as valid only if the list of signers intersects with signers of incoming main transaction. The check in mempool is updated as far.