Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Conflicts" transaction attribute type #1991

Closed
roman-khimov opened this issue Oct 7, 2020 · 8 comments · Fixed by #2818
Closed

"Conflicts" transaction attribute type #1991

roman-khimov opened this issue Oct 7, 2020 · 8 comments · Fixed by #2818
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Proposed attribute
This attribute makes the chain only accept one transaction of the two conflicting and adds an ability to give a priority to any of the two if needed. It's useful for solving such problems as:

  • replay attack mentioned in Use consecutive tx.nonce, like counter #1502
    User sending transactions like in Use consecutive tx.nonce, like counter #1502 (comment) would add "Conflicts Tx1" attribute to Tx2, "Conflicts Tx1, Conflicts Tx2" to Tx3 and "Conflicts Tx1, Conflicts Tx2, Conflicts Tx3" to Tx4, no matter which one is going to be relayed and added to the chain any of the other couldn't be reused because of the conflict
  • overriding erroneously sent transaction
    There is a short time window between transaction relaying and transaction acceptance, in some cases it could be used to cancel the transaction before it's accepted, to do that one sends new transaction that conflicts with the previous one, but has a higher network fee.
  • specifically creating pairs of transactions of which only one could be accepted
    This is going to leveraged by P2P signature collection service from Network assistance for multisignature transaction forming #1573.

The attribute has Uint256 data inside of it containing the hash of conflicting transaction. It is allowed to have multiple attributes of this type.

During transaction processing additional actions are to be done for transactions containing attributes of this type:

  • check for conflicting transaction in the ledger
    if conflicting transaction is already on-chain, the transaction is rejected as invalid
  • check for conflicting transaction in the mempool
    if conflicting transaction is present in the mempool, then two transactions are compared for their network fees, the one with a bigger network fee wins, so the other one is either rejected as invalid (if it's not in the mempool) or evicted from the mempool and replaced by the new one
  • reverification follows the same basic rules, any conflicting transaction on-chain makes mempooled one invalid
  • persistence logic should check in-block transactions for this attribute and
    store dummy conflicting transactions in the DB to prevent conflicting ones from being accepted

Since this attribute incurs some additional processing overhead it may have an additional network fee.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Ledger
  • P2P (TCP)
@roman-khimov
Copy link
Contributor Author

There is a prototype now in nspcc-dev/neo-go#1507.

The logic has been adjusted a bit to solve an attack possible with the following scenario: Alice signs transaction A and sends it to the network, then Bob sees this transaction in the mempool and immediately creates transaction B conflicting with A and paying higher fee. The original logic described here allowed to accept B in favor of A and if Bob has enough GAS he can effectively DoS Alice for as long as he wants to. So B has to be signed by Alice to prevent this, if it's not then B is invalid and A stays in the pool irrespective of B's fee.

And we also effectively reserved version 255 for dummy conflicting transactions to check for conflicts/presence in the DB with one DB access (it can be done without that, but for now it's simpler this way).

@igormcoelho
Copy link
Contributor

I think this is an interesting problem... instead of attributes, I'd always prefer to keep standard features, such as cosigners.
If you add a script on verification like "Neo.Transaction.Exists txid", and it also considers txs in mempool at that time, this may solve your problem. The issue is that txs become stateful again, as in your proposal.
A stateful tx with mempool interference may also interfere in consensus: imagine one node prefers txA and other txB... speaker proposes txA, but other nodes fail to extract and verify txA from p2p mempool because they already have txB. A solution may be some sort of "force receive tx" even if it seems invalid (on mempool), as during block verification it will be valid.
Other possibility is to only allow this interop consider block context, not mempool, so speaker may have both on mempool, and only when assigning one by one to block it will discover that txB cannot be selected because txA was already selected (and many other variants of this).
Again, I like the power of such mechanism, but we must assure it doesn't affect consensus process in a bad way.

@roman-khimov
Copy link
Contributor Author

add a script on verification like "Neo.Transaction.Exists txid", and it also considers txs in mempool at that time

This would mean that we have to reverify any non-standard script of any mempooled transaction for any transaction added to the pool.

And functionally it only allows for transaction B to be invalid in presence of A, which is not enough, we need preemption guarantees if some conditions are met (like this doesn't allow to override any already sent but not yet accepted into block transaction).

speaker proposes txA, but other nodes fail to extract and verify txA from p2p mempool because they already have txB

Backup CNs will request txA from their peers and this transaction doesn't have to go through the mempool, so if it'll manage to arrive in time it's not a problem (at least for neo-go, but IIRC C# is also fine wrt this in the master branch).

@igormcoelho
Copy link
Contributor

This would mean that we have to reverify any non-standard script of any mempooled transaction for any transaction added to the pool.

This is the same as current, will not increase number of validations: only new transactions would be validated, and previously validated are kept assumed as validated. Example: txA enters mempool (validation ok!); txB tries to enter mempool (validation fails because txA exists). From what you mentioned, some txC enters mempool, that is fine (no conflicts) with txA; but maybe txA is against existence of txC (not symmetric), is that the case? So, yes, in this case, both txA and txC would be on mempool, and only when puting to block, txA would "realize" that txC is also a candidate, thus being rejected (if txC has entered before).
That's a side effect of any stateful mechanism.. and that's why transactions are re-validated before puting on block.

@roman-khimov
Copy link
Contributor Author

some txC enters mempool, that is fine (no conflicts) with txA; but maybe txA is against existence of txC (not symmetric)

Consider txA and txB, txB does Neo.Transaction.Exists txA check, txA does no checks at all (and we can't really put Neo.Transaction.Exists txB there). Now if there is txA in the mempool and we receive txB it'd run a witness check and fail it, txB is rejected, everything is fine. But if we're in the opposite scenario where there is txB in the mempool and we receive txA, it'd be happily accepted with txB still being present in the mempool.

and only when puting to block, txA would "realize" that txC is also a candidate, thus being rejected (if txC has entered before).

Back to txA and txB from above, even if you're to do this block-level check, block B1{txA, txB} would be considered invalid, but block B2{txB, txA} is perfectly valid again, although it shouldn't be.

All of this is not a problem when using Conflicts attribute.

and that's why transactions are re-validated before puting on block.

Except they're not as we've discussed in #2017. Fees are being checked, but not witnesses.

@igormcoelho
Copy link
Contributor

Fees are being checked, but not witnesses.

I hadn't realized that... so my final argument there may not be correct. For me, all witnesses are being rechecked in batch-mode before tx invocations. Anyway, if tx witness is currently stateless, there's really no need to re-check (but this proposal makes them stateful, so recheck is needed during block proposal/acceptance).

I agree there are many challenging points here, because I'd much prefer having standard cosigners checks doing the job, than a new attribute for the same behavior. I'll try to better understand the expected behavior in your proposal, to try to sketch the same without a new attribute.

@roman-khimov
Copy link
Contributor Author

but this proposal makes them stateful, so recheck is needed during block proposal/acceptance

We just need to check for conflicts (kinda similar to UTXO double spend check) and it's feasible, already implemented in neo-go.

I'd much prefer having standard cosigners checks doing the job, than a new attribute for the same behavior.

That was one of the main conflicts to resolve for #1573, whether to leverage some witness script powers or to add attributes. Given the constraints we have I don't think it's possible to do that via witness scripts (and especially given that we don't want to parse scripts for some data we need), thus we've resorted to attributes and they're working fine for now. Though if you manage to do that it'd be interesting, of course.

@igormcoelho
Copy link
Contributor

So, I think we may have a general strategy / pattern to follow in these situations... since transaction validation is intended to be stateless, we may resort to attributes in order to execute these stateful computation, but in quite constrained scenario.
It's interesting, because we could build the dependency graph and perform these checks on verification, but doing that "manually" via attributes look much easier (since they are done on C#/Go/...), at the cost of increasing software complexity (one more attribute).
The same situation happened with ValidUntil field, since it's also easily implementable as cosigner, but in the end, it was preferred as a transaction field.

So, with this strategy in mind, I agree with you, it's better to have it as attribute then as cosigner script.

AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Sep 27, 2022
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Sep 27, 2022
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Sep 27, 2022
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Sep 27, 2022
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Sep 27, 2022
AnnaShaleva added a commit to AnnaShaleva/neo that referenced this issue Mar 12, 2023
shargon added a commit that referenced this issue 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 <[email protected]>

* MemoryPool: add test for on-chain conflict

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

* 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 <[email protected]>

* Use Distinct to filter out duplicating conflicting on-chain signers

Co-authored-by: Shargon <[email protected]>

* Use HashSet as a container of conflicting hashes in the mempool

Fix #2818 (comment).

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

---------

Signed-off-by: Anna Shaleva <[email protected]>
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Vitor Nazário Coelho <[email protected]>
Co-authored-by: Jimmy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants