-
Notifications
You must be signed in to change notification settings - Fork 1k
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
conflict attribute stops the network #2907
Comments
a sample transaction sent is like https://testnet.neotube.io/transaction/0xfe03b9dfaa2b57735bfd2114a397941f303a295cff551fa41bdaf3b804e82295 |
the testnet n3t5 has already been paused for 90 mins because of those transactions at block https://testnet.neotube.io/block/2690040 |
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. |
Do you have a fix in mind? |
IIUC, that'd be a limit for stored signers, should be sufficient. |
It'd be nice to not break the testnet again though. |
@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. |
@roman-khimov I guess there are additional limitations elsewhere like stack item size or something else |
That is it,my friend. I tried to warn before that this was a dangerous feature for mempool. If so, lets revert the PR for now and update nodes 3.6.0 with a patch. |
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. |
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. |
But maybe there is a fix. But the way you explored looks like a crash. |
Maybe because of autoheal functions |
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. |
I'm not sure but I feel, some StackItem limitation is trigged here. |
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. |
@vang1ong7ang This is not the most responsible way to communicate these kind of issues. |
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). |
Conflicting hashes are a part of the neo/src/Neo/SmartContract/Native/LedgerContract.cs Lines 50 to 57 in 5207956
So that may be true.
And that may be true as far, because it requires intensive storage access.
This would help, it would be a nice solution, just limit the possible number of conflicting hashes in the |
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="]}' |
@shargon tell me the suggestions unless those suggestions brings additional works or troubles to us |
@vang1ong7ang You should never try any type of attack with the official testnet or mainnet, use your own private network, otherwise if it's confirmed, it could lead to a fork or worse still, that a third party abuses said attack and you are directly or indirectly responsible for its exploitation. We have private chats for sharing sensitive information (#2950). |
@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. 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. |
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.
|
I am 100% aligned with @vang1ong7ang . Thus, the focus here is to solve the issue. |
In computer security, coordinated vulnerability disclosure (CVD, formerly known as responsible disclosure) is a vulnerability disclosure model in which a vulnerability or an issue is disclosed to the public only after the responsible parties have been allowed sufficient time to patch or remedy the vulnerability or issue. https://en.wikipedia.org/wiki/Coordinated_vulnerability_disclosure <- I could look up thousands of other examples if you want. @vncoelho This is the first time I've seen denying service from a public network, as well as disclosing a security issue with the exploit so that anyone can continue to exploit it without fixing the problem, being treated as responsible. Let's exercise some judgment, please. |
IIUC, coordinated vulnerability disclosure and full disclosure are all pratical ways. coordinated disclosure is just a form of disclosure and I don't see how this it is related to whether I'm responsible. responsible disclosure is just a name and please do not regard it as a necessary and sufficient condition for responsibility The form of disclosure chosen depends on our assessment of the vulnerability's damage. we're trying to find a way to improve efficiency and reduce the damage. @shargon should always know, the version 3.6.0 is NOT in production (mainnet). it is currently in public testing stage. and DoS vulnerabilities are usually treated as medium level. |
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).
It's not possible with 3.5.0.
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. |
Feels like a Neo equivalent ERC-4337 solves this and a number of other issues we have, like #2577. It's what I was getting at in this comment. The way I see it currently, this issue, NotaryService + #2907 (comment) 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. |
@AnnaShaleva what do you think about only write the sender, not all of the signers? |
do not confuse multi-signer transactions with multi-signer conflicts. I think these two problems can be solved independently.
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. |
UTXO resource lock by bitcoin and increment nonce counter by ethereum are both simple and efficient ways. just as @shargon questioned:
Is the multi signer conflict problem really necessary to be solved? it could be easily solved by introducing an intermediate contract based multisig agent wallet. when you want to cancel / conflict / replace an existing transaction as soon as possible, a rational signer won't collect more than one signature since a single signature is enough to conflict with the existing one. on the other hand, when you want a transaction can be easily canceled / conflicted / replaced by multiple partners, introducing an intermediate proxy account that any one of you is able to control is a workable solution. it's not necessary to solve a non-existing situation that looks general. |
@AnnaShaleva is a careful, rigorous and smart person that I admire very much. but dancing with shackles is difficult after all. No matter how wonderful this implementation is, it cannot escape the situation of:
if the conflict fee is charged, the transaction replace fee grow by O(n) where n is the times of the tx being replaced. |
I know almost nothing about the E-letter chain, but some quick thoughts I had after glancing over it are:
Who is to pay for this filtering and in what manner? Otherwise it'll be abused.
Yes. #1573 relies on it (main transaction can be paid for by anyone, fallbacks are paid for by the Notary contract (out of predeposited GAS of course)).
Absolutely true and |
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. |
Looks like we're talking 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?
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 #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. |
the V-letter man cannot fully agree with those comments but i'm not going to be too far from the current topic. |
I don't understand why #1573 have to rely on this. can you explain it in more detail? on both sufficiency and necessity. |
@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. |
@EdgeDLT, I can agree with many points, but if we're to specifically concentrate on Conflicts then it's rather simple, it's an integral part of the #1573 solution. That can be used for other things, as noted in #1991. If we don't have it, we don't have #1573 solution (and #2577, and many other things, as I've said, with #1573 base real-time oracles can be built in a week), simple as that. If there is any other solution for #1573, I'm all ears. If we don't need it, OK, we'll keep it in the NeoFS sidechain (and will keep growing the extensions list) and let's just drop Conflicts in 3.6.1 (hi, #2914) The thing is, I'm pretty open about problems we're interested in. They mostly come from NeoFS needs, because it's not just Neo that has NeoFS, it's NeoFS that has Neo inside as well. And usually we're ready to provide some solutions to these problems. That's why there are #1573, #1991, #2577, #2866, neo-project/proposals#156, neo-project/proposals#160 and some others. That's why #2905. If there are any problems with these solutions, we can either fix them or replace them. Oh, there is also an option of not doing anything at all. Sorry, forgot about that. @vang1ong7ang, please check #1573 (comment). We have a single main transaction that we want to complete there and N (like 7) fallbacks that all conflict with it. The only common signer between them is the Notary contract. It MUST NOT be the sender of the main transaction, it MUST be the sender of the fallback. They usually don't have any other common signer. |
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 |
Originally posted by @roman-khimov in #1573 (comment) I think the existing solutions are good enough. the shortcomings you stated do not hold true.
|
@vang1ong7ang, we're obviously coming from different perspectives. If you can make something (Safe-alike or not) for Neo (with scopes and things), collect signatures in sub-block time --- be my guest. Just remember that NeoFS totally depends on multisignature collection and it's not that we need a service for a network, we need it for the main network, we need it for the sidechain, we need it for private networks, we need it everywhere. Keep in mind #2806 for the GAS dust as well. |
The approach I proposed includes #1573 functionality, just as it includes #2907, just as it includes (or rather, would rely on) #2866 or similar. P2P Notary role is basically the proposed bundler already, the only real difference is that it hasn't been generalized beyond the multi-sig use case! Can we recognize that there's nothing new about the ideas and issues I've presented here. Most of them are scattered across existing issues, we've tagged many of them, probably most of them Neo SPCC suggested in the first place because they were relevant for NeoFS. So by no means am I asking you to drop what you need for NeoFS to work. I'm asking you to help find a way to extend the ideas already brought to the table in order to improve all the other applications while we're at it. A unified solution is required because these problems are connected, we've just been treating them as separate. It comes down to the fact that we lack flexibility in how state updates can be pushed to the network. For NeoFS, you need practical multi-sig operations. For my application, I need to be able to let someone self-custody without requiring them to safeguard a private key or understand how to use a wallet. Any application on Neo requires an end user to have a GAS balance. These are all barriers that can disappear simultaneously if we just abstract them away thoughtfully. I don't know Neo or the NeoGo extensions inside and out. I really wish I could give you a neatly wrapped proposal that addresses all the nuances of implementation. But I can't, it's beyond my ability today. So it's incredibly frustrating when I know the solution exists (I can use it right now elsewhere), and the only people that can make this a reality are seemingly disinterested, despite showing interest in the individual components alone.
Right... and my practical choices as the lead for GasBot are:
I'm asking you (Neo core) to give me a fourth option. I can't design it. All I can offer is perspective. I'm an astronaut, you are the rocket scientists. You know the engineering better than I ever will. Think I've said all I can on this topic. Please think it over. |
We can think of extending it. At the moment it can do both multi-signature (multiple keys for a single signer) and multi-signer, given the signer flexibility of Neo that can solve a lot of things. But at the moment we don't have it in mainline Neo even in the original, transaction-related scope.
I've said that most of our proposals come from direct NeoFS needs. But to be fair, we could've simplified many things (#1573 included) if they were absolutely NeoFS-specific (tune for the particular use case and be done with it or implement in NeoGo and forget about it). That would mean a substantially different (from mainline Neo) sidechain with zero possibility for convergence (and potentially more unknown problems like here). So we try thinking of more generic solutions that cover more scenarios than just NeoFS.
Maybe we need to talk about it. To me it looks like most of your problems are #2577, but maybe I'm missing something. And in general I agree that any real feedback for any real dApp should be prioritized. |
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]>
Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. 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]>
Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. 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]>
Implement the neo-project/neo#2907 (comment) and port a part of neo-project/neo#2913. Signed-off-by: Anna Shaleva <[email protected]>
* Ledger: change conflict records storage scheme This commit is a part of #2907: it implements conflict records storage scheme discribed in #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]> * UT_MemoryPool: remove unused test 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 0c06c91. Signed-off-by: Anna Shaleva <[email protected]> * Policy: introduce fee for Conflicts attribute This commit implements Conflicts attribute policy described in #2907 (comment). Signed-off-by: Anna Shaleva <[email protected]> * *: remove remnants of ConflictsFee in native Policy ConflictsFee logic was replaced by the generic attribute fee mechanism implemented in #2916. Signed-off-by: Anna Shaleva <[email protected]> * Native: do not remove malicious conflict records during OnPersist 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 #2913 (comment). Signed-off-by: Anna Shaleva <[email protected]> * Properly rewrite previously added malicious conflict if it's in the storage `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]> * Throw proper exception if TestMaliciousOnChainConflict fails Signed-off-by: Anna Shaleva <[email protected]> * Optimize conflicts records storing Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add. Ref. #2913 (comment). Signed-off-by: Anna Shaleva <[email protected]> --------- Signed-off-by: Anna Shaleva <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: Jimmy <[email protected]>
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:
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)The text was updated successfully, but these errors were encountered: