-
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
Question: Verification before Invocation #2017
Comments
Currently yes, if the transaction was chosen for the block, we doesn't check again if the verification it's valid or not. If your verification depends to the state, and two transactions enter, the next one can be wrong verified and enter as success. I think that this behavior is logic because the CN doesn't execute the TX, otherwise you will pay the fee for unverified transaction. |
Thanks a lot for the explanation @shargon , but really, we need to fix that. It is not acceptable that some invalid witness is considered for valid execution, I guess you can imagine why. The fix is simple though, and it doesn't affect pricing strategy:
This allows for one transaction to take into account the side effects of other transactions. This fix is valid for both Neo 2 and Neo 3, and it is an important one, as storage reads seem to be quite useful in verification, and Neo 2 strongly relies on them.
I understand your perspective, in which "logical" means, the way it is makes some sense... but paying for unverified transaction, in worst case scenario, is better than having some CheckWitness returning inconsistent result. User tx will have opportunity to be "pruned" by mempool, or just before speaker puts in block, but it may happen to be paid and unused (otherwise we drop precious space from blocks). The good thing is that Invocation itself will not be paid (and could even be refunded, but only doable in Neo3). |
I think we can trust our own mempool and not do the second verification, but even if we're to eliminate this step, reverifying before execution would seriously affect TPS metrics. We can skip standard (state-independent) verification scripts, though.
But overall all of that reminds me of #1285. |
This a very good point @roman-khimov . I'll take s look at the issue you mentioned. Anyway, even if we skip state independent witnesses, we will pay some cost to detect them. And from a present / future perspective, maybe @shargon will also agree that we want nearly all, or maybe all, transactions to include balance state verification. |
After some thoughts and discussions, I may have changed my mind, so this is how I see : Neo2:
Neo3
If thats the case, I guess we can close this, thanks fir all clarifications |
@roman-khimov just put some doubt in my mind in a discussion here: neo-project/proposals#97 (comment)
It seems to me that the logical operation of Verification is to occur before Invocation, since we may use these witnesses inside the Invocation. And, it seems logical to me that, if we are using CheckWitness, we assume this witness is correct (returns True), right?
Ok then. Maybe, if we are verifying all transactions in a pack, and only then, executing all the invocations (I don't remember exactly if this is the procedure), this can cause inconsistencies and even break witness assumptions.
Suppose some witness depends on checking the signature of some owned contract (written in storage), and if we change this owner, we have to suppose that the next transactions would fail this verification, regardless of being in same block or not. Or are we only taking into account the states in the block BEFORE execution...
If verification fails, then tx must FAULT invocation. And that's even more important to have Post Verification flag stored on the chain, as recently proposed, otherwise we risk breaking witness and verification invariant, which are sacred.
If this is the case, I think we should definitely change this behavior, by re-executing Verification before every execution of Invocation (including side-effects of previous tx in same block), issuing FAULT if witnesses not satisfied, for security reasons.
Can you clarify @shargon?
The text was updated successfully, but these errors were encountered: