Codecov Report

Merging #856 into master will increase coverage by 0.12%.
The diff coverage is 83.8%.

@@            Coverage Diff             @@
##           master     #856      +/-   ##
==========================================
+ Coverage   44.93%   45.06%   +0.12%     
==========================================
  Files         177      178       +1     
  Lines       12577    12605      +28     
==========================================
+ Hits         5652     5680      +28     
  Misses       6925     6925

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 950b5ff...6361bf3. Read the comment docs.

@erikzhang congratulations for this amazing step!

1. Transaction.ValidUntilBlock is state dependent.

I thought a lot about it in the past month, and I think that this is not state-related. We can define two things: ledger structure (blockchain itself, including height); and blockchain state itself (result of operations over this chain).
So, requiring access to height is not a state operation per-se, because it only depends on unprocessed headers, and "material" stuff. The real problem is "imaterial stuff" like storage, notifications, and things that require processing, and are not directly persisted.
Item 1 is not a problem, although I agree that a tx which enter mempool, may eventually become invalid. That's why this field is of upmost importance on tx (not a simple verification script), because it allows us to remove tx over time, regardless of their state. We can use this field for prunning, as long as we don't allow tx to have high expiration values.

2. Policies may be changed every block.

This is fine, because of item 1. Tx will have limited life.

3. Check fees. (This can be solved easily.)

In fact, this would be the most problematic case, because like you said before, a NEP-5 balance is a state operation. So, this can be "easily" solved, as long as only balances for GAS and NEO are controlled. If we wanted to control every type of asset, on verification, we would need crazy ideas like the Storage.Add we were discussing. But since we won't need to do that, that's fine... just beware that we will likely have Payable contract operations inside smart contracts, right? So, I think the safest way to do this, is only allow GAS and NEO to be paid to other contracts (the other NEP-5 are regular dynamic/static invoke operations), to the limit which is transferred to the contract in the operation itself.

Example:

Tx A
verification:
Sending 100 GAS from user B to contract C
application:
invokes send 60 GAS from contract C to contract D

probably it should allow sending back automatically to user B the 40 GAS difference, which was "unused" (this depends on logic). This type of thing guarantees that we only re-manage funds available over verification, so tx will never fail when out of GAS funds inside blocks.

The only problem to be solved now is the change in policy. My solution is to clear mempool when policy changed.

Why policy change is so problematic.. pricing changes?

Nice commits, Erik, how about the GAS spent during Application on in payable contracts? We need to sum them as well during reverify.

Why policy change is so problematic.. pricing changes?

Yes.

Nice commits, Erik, how about the GAS spent during Application on in payable contracts? We need to sum them as well during reverify.

Don't understand.

Don't understand.

Let's use the following scenario, an account with 10 GAS:

• emits 10 different txs (tx_1, ..., tx_10) with 1 GAS each;
• all of them in the mempool;
• tx_1 of them enters in the current blocks and 9 still on the mempool;
• tx_1 was a payable tx and its applications transferred 7 GAS to someone else;
• 2 out of the remaining 9 txs should not be dropped from mempool during re-verify

Notes:
This is why we have been also mentioning about:

• A limit in which payable txs are willing to pay. With that we could use this limit during re-verify

I guess Vitor is worried about the payable operations which we will be able to do during invoke now... I think they shouldn't be purely based on Application, otherwise their effects may become unpredictable in the future, so as user balances. I asset transfer is done purely on application, and storage is delayed, it will be hard to verify future tx, to be sure the funds will actually exist at that time (even to pay network fees, system fees, etc). So I think that payable stuff inside contracts should be pre-paid during verification, with the possibility of "cashing back" the difference or unused gas/neo.

2 out of the remaining 9 txs should not be dropped from mempool during re-verify

It is correctly handled currently.

Erik,looks like it works, logic is fine. Native nep5 is supposed to be sent on application trigger or only system trigger?

Just to clarify how transfers would work, and make sure we are on the same page (nep5 mint operation):

Sender = myhash
NetworkFee = gas from Sender
SystemFee = gas from Sender
WITNESSES= mywitness
Script (nep5 mint)
push myhash
push icohash
push 10 (neo)
Call NativeNeo
push "mint"
push myhash (in array)
Call icoContract

Then, IcoContract would get notifications, including from NativeNeo, and know how much tokens to send, right?

My only concern is when contracts start transfering native tokens internally, we need to make sure this wont break balances. But we can discuss this later, we have options.

Still missing the policy change part I guess.

Yes. I will mark it as ready for review when it is completed.

Erik, do you think we can pass two Script on a tx, first limited to a "system trigger", and second an application trigger?
first could be used for contract create, migrate, native transfers, and would run on block proposal. This ensures balance is always correct and updated, and "sender" could be removed too, as network fee could be a regular transfer to consensus address.

@erikzhang, it has been a great PR and discussions.
Sorry for not taking dedicated time to help with fine tunes of this implementation.

Let's move to the next steps and later we double check everything.