Neo: Neo.Runtime.GetTime tells us about the future it doesn't always know
Describe the bug
Neo.Runtime.GetTime (added in #122 for #114) returns timestamp from the block being persisted (P.T down below) or the last persisted (N down below) + Blockchain.MillisecondsPerBlock (15s down below). I don't know what was the original motivation for this syscall, but I think this behavior makes it less predictable than we probably want.
Consider a contract that uses this syscall in verification context. Transaction might be added to the block that satisfies some condition using N + 15s timestamp, but doesn't satisfy the same condition using P.T timestamp because it can be anywhere from N + 1s to N + Xm. Of course we will consider this transaction to be verified anyway because we trust the signed block (although one might argue that it might still be useful to reverify it), but it might not be in line with transaction's Sender intention.
Same thing for application context, of course in this case the value returned is gonna be strictly deterministic for a given block, but at the same time it might be different in any unpredictable way from test invocation at height N, so again the user might expect one result, but get something different in the end.
In application context we also receive some information about the future with this call because if we're to ask about the blockchain state in any other way (like System.Blockchain.GetHeight) we're only going to receive information about already persisted things and we won't get anything from PersistingBlock other than this timestamp. So here we don't have a clearly defined event sequence, on one hand the block didn't settle yet and we can't get any information about it (like its hash), but at the same time we already know its timestamp and thus consider transaction to be running after this time.
Possible solutions
A) Just drop Neo.Runtime.GetTime. I'm not sure whether it's really used in any way in NEO 2.0, but it looks like the best option to me as in every context it will give us clear picture: we're running after the last persisted block (it's easy to get a timestamp for it), but at not yet known time or block. It seems to be fair as we will only known that P happened after its processing will be completed (and we'll have full P data at this point).
B) Make this interop function unavailable for verification context, which solves a part of the problem and can be optionally be with the following change.
C) Make PersistingBlock fully available in the application context, this will make our transactions run after it in time, but I don't think it's correct because not all block persistence side-effects are visible yet (like other transactions or OnPersist).
roman-khimov
All 14 comments
For me, I prefer to drop Neo.Runtime.GetTime, block height is the blockchain time.
Tommo-L
on 9 Apr 2020
The tx was valid for this block, if you want to verify the past, you need the past rules.
This behavior could be reproduced with the height of the blockchain if you return true only if the height is less than 1m, and you try to verify in 1m+1?
I can't see the problem 馃槄
shargon
on 9 Apr 2020
This behavior could be reproduced with the height of the blockchain if you return true only if the height is less than 1m, and you try to verify in 1m+1?
No, you only see height of N (already persisted block) in any circumstances until P is fully persisted, so reverifying in-block transaction would work just fine at its height. That's the option we have in neo-go, obviously we do not reverify transactions from block P at height P or P + smth it doesn't make sense, but when you're adding a new block (P) you should be able to verify transactions it contains against height (and state) N which is P - 1 and it works just fine (well, not for 2.0 mainnet, but that's a different story) except this syscall that tells us that we're running after P.T timestamp.
Let me reiterate, when we're verifying (or running as in application context) some TX against blockchain at height N, P didn't happen yet, but we're already predicting P.T in this syscall and then when P happens (while it's being persisted) we already have actual P.T as a result of this syscall even though P technically would happen only after all of its transactions will be processed completely and we don't have any data from P available to the transaction via other syscalls until P is persisted.
roman-khimov
on 9 Apr 2020
GetTime takes the TimeStamp from the persising block
And it's signed by CN in Prepare request
So ... for the VM there is no difference between the TimeStamp and the Height, both are taken from the persisting block, its information already stored in the current block, so it's deterministic
shargon
on 9 Apr 2020
GetTime takes the TimeStamp from the persising block
Sure, if it has a real block. But in verification context (mempool scenario! different from 'verifying tx from the new block') and for test invocations it'll make a CreateDummyBlock(snapshot) which sets some arbitrary time in the future relative to the already persisted block.
there is no difference between the TimeStamp and the Height, both are taken from the persisting block
Not exactly, if you're to call System.Blockchain.GetHeight you will get already persisted height, not the height of the block being persisted.
roman-khimov
on 9 Apr 2020
Then we should not drop GetTime we should execute Verification in stateless mode
shargon
on 9 Apr 2020
But it's not only about verification. Imagine an attack scenario, if we're to know that some transaction (application context, let's say some auction contract that takes bids) is time-sensitive in that it uses GetTime for its decisions (like when the auction should end) then if we're to arrange DoS attack against CNs we can delay the next block (containing transaction we care about, like other bid) which will affect GetTime result. This transaction will then deterministically do something we (as an attacker) want it to do (like throw away bid that came after the declared auction end time and make us a winner) and not something Sender wanted it to do based on his test invocation result.
It's a bit overstretched example, but only a bit.
roman-khimov
on 9 Apr 2020
I think that this scenario is a regular vulnerability in the smart contract and the project must be aware about it. But it's not a problem of us
shargon
on 9 Apr 2020
Yes and no, this vulnerability is enabled by the API we provide to the smart contract (as in environment the contract runs in). Contract creators have this data, so they will use it and misuse it. Whether they should have access to this data or not is the real question.
My position is that as PersistingBlock hadn't yet happened, no data from it should be available to the contract (including timestamp). It's consistent with other APIs we have like System.Blockchain.GetHeight. And I don't see any particular use case for Neo.Runtime.GetTime data where it would be important to use this API instead of System.Blockchain.GetBlock(System.Blockchain.GetHeight())[5] and this API use would be safe. Which returns us to the question of original motivation behind this API.
roman-khimov
on 9 Apr 2020
Actually, both System.Runtime.GetTime and System.Blockchain.GetHeight can't be invoked in verification context.
Tommo-L
on 10 Apr 2020
Good, one problem less (still getting used to NEO 3 features).
roman-khimov
on 10 Apr 2020
BTW, now we do allow this syscall again in verification context.
roman-khimov
on 26 Jan 2021
The verification context doesn't have PersistingBlock. So it doesn't allow GetTime in verification context.
erikzhang
on 26 Jan 2021
True, it won't really work. Then the only thing left is the question of exposing some details of current block to the contract while hiding others. Maybe it's not critical though, if there are valid use cases for GetTime and everyone is aware that the real GetTime can differ from test invocations it can stay as is.
roman-khimov
on 26 Jan 2021
Related issues
vncoelho
路
3Comments
canesin
路
3Comments
erikzhang
路
4Comments
igormcoelho
路
3Comments
igormcoelho
路
4Comments