neo-project
SmartContract: restrict the number of allowed notifications
Description
The problem is described in https://github.com/nspcc-dev/neo-go/issues/3490. In short words, an arbitrary number of smart contract notifications is allowed (up to VM's GasLimit and MaxBlockSystemFee, but it's huge), which results in a possibility to DoS the node. 10K of large notifications emitted in a single transaction result in block acceptance delays for both Go and C# nodes. 50K of large notifications may result in the node failure. I did these tests on privnet, you may find the results in https://github.com/nspcc-dev/neo-go/issues/3490#issuecomment-2432050884. Please, don't try to DoS mainnet/testnet using this vulnerability.
Port the solution from https://github.com/nspcc-dev/neo-go/pull/3640, but an alternative solution is described in https://github.com/nspcc-dev/neo-go/pull/3640#issue-2608501310 and in https://github.com/nspcc-dev/neo-go/issues/3490#issuecomment-2432050884. MaxNotificationsCount constraint is chosen based on the Mainnet statistics of the number of notifications per every transaction, ref. https://github.com/nspcc-dev/neo-go/issues/3490#issuecomment-2427153253.
This PR is a subject of discussion, hence please, share your thoughts on the described problem and proposed solution. Once we agree on the solution, I'll add unit-tests to this PR.
Type of change
- [X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
How Has This Been Tested?
- [ ] Unit-tests should be added once we agree on solution.
Checklist:
- [X] My code follows the style guidelines of this project
- [X] I have performed a self-review of my code
- [X] I have commented my code, particularly in hard-to-understand areas
- [X] I have made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [X] I have added tests that prove my fix is effective or that my feature works
- [X] New and existing unit tests pass locally with my changes
- [X] Any dependent changes have been merged and published in downstream modules
This needs to be in VM limit class.
Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.
This needs to be in VM limit class.
Strictly speaking, it's not a VM limit. Notifications are not a part of VM, they are a part of execution engine.
All the same thing. Class is called ExecutionEngineLimits in Neo.VM namespace. How to use engine.Limit.MaxNotification
instead of limiting number of notifactions, i prefer to make the price related to the notifaction size. From @AnnaShaleva 's excellent benchmark, i think size * number is the real reason. Cause i think its a normal practice contract may need to send a lof of notifactions, but definately abnormal to send large notifactions.
i think size * number is the real reason
Exactly, as described in https://github.com/nspcc-dev/neo-go/issues/3490#issuecomment-2432050884.
i prefer to make the price related to the notifaction size.
Yes, it's an alternative solution proposed in https://github.com/nspcc-dev/neo-go/issues/3490#issuecomment-2429477277. But we need to discuss it because even with extremely expensive notifications there's a chance that CNs will be killed by a single transaction. Yes, the user will have to pay for it, but CNs may not be able to process this transaction. Also, large number of notifications is harmful for both C# and Go RPC servers.
So to me, the restriction of the overall notifications number is required, but at the same time it can be combined with dynamic price solution.
Economics.
Currently System.Runtime.Notify is priced at 32768×ExecutionFeeFactor, for mainnet that's roughly 98K datoshis.
Storage fee price is 10K datoshis per byte for mainnet.
Typical NEP-17 Transfer is ~50 bytes. So storing it on-chain is 500K datoshis. Emitting as an event is 98K. Seems to be fair enough, roughly 2K per byte or five times cheaper. If we're to extend this multiplier to 1024-byte event that'd be 2M datoshis or 0.02048 GAS. Multiply it over 50K events and you get a 1024 GAS cost. Which is 4-5K USD at the moment and is allowed by MaxBlockSystemFee (#3552). So currently this economics doesn't really work to solve the problem.
Based on https://github.com/nspcc-dev/neo-go/issues/3490 even 10K are problematic (204.8 GAS with 2K per byte), so we better target at 2-5K as being practically impossible. Which doesn't really work even with 10K per byte since 10K events would cost the same 1024 GAS then. (side note: @steven1227, these calculations are also relevant for any fee reduction talks)
Not sure we can balance this economically.
Which doesn't really work even with 10K per byte since 10K events wo
More than X notifications (100?), can be priced exponentially by the count.
priced exponentially by the count.
Yeah, I had this thought as well, but you can easily avoid it with some number of transactions. Maybe they'd be a little less problematic than a single one, but still.
Any feedback or merge?
As is discussed in our previous meeting, this should wait for https://github.com/neo-project/neo/issues/3644
Previously we’ve agreed that this solution is less preferable than https://github.com/neo-project/neo/issues/3600. @roman-khimov do you think we need to merge it?
Previously we’ve agreed that this solution is less preferable than #3600. @roman-khimov do you think we need to merge it?
Why not complementary?
The theory is that if we're to have #3552 done with MaxBlockSystemFee of like 20 and then have ExecutionFeeFactor of 30 again then 20 GAS is ~2034 notifications and we're fine as is. The problem is that we neither have a low MaxBlockSystemFee value nor we have a high ExecutionFeeFactor (and can't have it till #3600, which will happen no one knows when). Given the severity of the issue at the same time, I'm OK with some hard limit for 3.8/Echidna. At least it's simple and effective.
Good, updated. Then let's consider merging this PR in 3.8.
I'm also voting for the merge since I agree that this solution is better than nothing. Later, when we have lower MaxBlockSystemFee and flexible opcode prices, we may revert notifications number constraint (in some next hardfork).