Thank you for your PR that resolved the testnet halt issue caused by the attack. We definitely support merging it.

I’d like to propose a consideration that might lead to a new PR or issue: could we allow all possible outcomes after a successful simulation and block packaging, especially when the transaction simulation results in VMState.HALT? Instead of throwing exceptions, this could cover scenarios like those we encountered during Storage.Commit, or even within OnPersist and PostPersist, helping to mitigate similar attacks in the future.

While the likelihood of an issue like today’s happening again is quite small, logging a high-level error could serve as an early warning. Take it into today’s case: the blockchain won't be halted by the attack, the affected contract might become unreadable due to deserialization failure — which, arguably, is a slightly better outcome.

What do you think?

Additionally, I’m wondering if there might be other potential risks beyond the contract checks — could other IInteroperable instances also cause similar issues during Storage.Commit? @Jim8y, could you and your teammates help take a closer look?

Shargon has also found the Oracle's problem. Great job!

Thank you for your PR that resolved the testnet halt issue caused by the attack. We definitely support merging it.

I’d like to propose a consideration that might lead to a new PR or issue: could we allow all possible outcomes after a successful simulation and block packaging, especially when the transaction simulation results in VMState.HALT? Instead of throwing exceptions, this could cover scenarios like those we encountered during Storage.Commit, or even within OnPersist and PostPersist, helping to mitigate similar attacks in the future.

While the likelihood of an issue like today’s happening again is quite small, logging a high-level error could serve as an early warning. Take it into today’s case: the blockchain won't be halted by the attack, the affected contract might become unreadable due to deserialization failure — which, arguably, is a slightly better outcome.

What do you think?

Yes, we will deep into it

the testnet halt issue caused by the attack

It's certainly not an attack, just an accident. People write contracts, people try them on testnet.

those we encountered during Storage.Commit

Each state change is bound to some execution context by design. The core of the problem is that this assumption is broken, transaction does something and fully succeeds, but then it turns out that the state change is incorrect and should've never happened.

within OnPersist and PostPersist

I don't think you can do this. It's pretty similar to the problem of insufficient GAS balance during OnPersist processing. If this happens we're toast, there is no correct behavior when we have a correctly signed block that does something wrong. This can be recovered only via a fork, throwing away a block and creating a new one.

those we encountered during Storage.Commit

Each state change is bound to some execution context by design. The core of the problem is that this assumption is broken, transaction does something and fully succeeds, but then it turns out that the state change is incorrect and should've never happened.

Perhaps, at the very least, we should have a reminder for others developers — ensuring that every new IInteroperable includes a check to confirm it can be properly serialized if it will be committed to the storage during transaction simulation. Otherwise, wouldn’t this kind of issue be something only experienced developers like you and Shargon would think to catch?

Or perhaps we should consider simulating the commit process during transaction simulation as well for all kinds of txs. If any errors occur during commit, the simulation shouldn't return a HALT state.

Or perhaps we should consider simulating the commit process during transaction simulation as well for all kinds of txs. If any errors occur during commit, the simulation shouldn't return a HALT state.

This can be done in consensus, although the transaction may give a different result (not care because it's a commit test).

We need HF flag here. @shargon

But why? You can't do

if old_node
    do_something()
else
    do_something_else()

here. The buggy node just stops processing and can't persist this block, nothing happened. Proper ones are perfectly aligned with this PR in behavior, they fail the transaction.

Agree with @roman-khimov , old node doesn't work, so no hf needed

It just occured to me that we can destroy contract as well in the same transaction. Can this be a problem?

Frankly I'd rather roll out the fix quickly for mainnet and be done with that.

@neo-project/core please review

But why? You can't do

if old_node
    do_something()
else
    do_something_else()

here. The buggy node just stops processing and can't persist this block, nothing happened. Proper ones are perfectly aligned with this PR in behavior, they fail the transaction.

It's a different behaviour and different native contract, you don't know what may happen before next release. Even stateroot is also different when resync. No doubt, it's a hardfork already.

IIUC you don't have any stateroot for testnet block 5557998 with the old node because to get a state root you need a state first and you don't have a state because there is an unserializable thing in the cache. Then if we're to tie this to Echidna the question is how to process testnet block 5557998, it's certainly pre-Echidna (unless you're to set it earlier and require reprocessing), but you can't have the old behavior because it just stops the node completely. Then also mainnet, I won't talk about it much here, but you understand the current state of it perfectly with the unpatched node.

IIUC you don't have any stateroot for testnet block 5557998 with the old node because to get a state root you need a state first and you don't have a state because there is an unserializable thing in the cache. Then if we're to tie this to Echidna the question is how to process testnet block 5557998, it's certainly pre-Echidna (unless you're to set it earlier and require reprocessing), but you can't have the old behavior because it just stops the node completely. Then also mainnet, I won't talk about it much here, but you understand the current state of it perfectly with the unpatched node.

So nothing breaks on mainnet, but a different storage from block 0 in v3.8.0. You think it works as expected?

@Jim8y Merge?