Scale time units to block timestamp resolution when using Raft

[lundmikkel]

Small disclaimer: not sure if this is actually Quorum or Solidity related. But I'll start posting it here.

Normally, timestamps in Ethereum are returned in seconds. When using Raft, the block timestamp suddenly gets a nanosecond resolution (for justifiably reasons). However, this can result in many unforeseen problems if the user isn't aware of the resolution (s/ms/ns) of the timestamps being returned, and starts using it as if it was still in seconds.

Many of these problems could be fixed by scaling the time unit keywords found in Solidity to ns when needed. This would allow users to correctly return values in seconds regardless of the resolution of the blockchain's timestamps. Consider the following code:

pragma solidity >=0.5.0;

contract Time {
    function get1Second() public pure returns (uint) {
        return 1 seconds; // Would normally return 1, but 1000000000 when using Raft
    }

    function getBlockTimeInSeconds() public view returns (uint) {
        return block.timestamp / 1 seconds;
    }

    function getDifferenceInSeconds(uint timestamp) public view returns (uint) {
        uint difference = now - timestamp;
        return difference / 1 seconds;
    }
}

The user of the contract now doesn't have to take the block timestamp resolution into account and changing the consensus would have no effect on the returned values.

[benediamond]

@lundmikkel thanks for raising this, it's in important issue.

You approach is interesting. Here are a few comments:

• It could be a bit of a drag to have to retroactively insert / 1 seconds into all Solidity code and recompile in order to make contracts consensus-agnostic. It would be nicer if block.timestamp itself returned consistent values in all cases.
• Relatedly, your approach wouldn't fix incompatibility on the web3 side, so long as calls like web3.eth.getBlock('latest').then((block) => console.log(block.timestamp)) still returned nanosecond values under Raft.
• As a side point, it would appear that your approach would require modifying the Solidity compiler, as opposed to the geth client / EVM. That's not really our line of work.

I wonder if the best approach here would be to change the 0x42 TIMESTAMP EVM instruction so as to perform the division by 1000000000 internally. Likewise, we will change the getBlock API so as to perform the division before passing the result along to the caller. This would leave the entire internal architecture intact, but make it indistinguishable to the user whether Raft or IBFT is being used.

@SatpalSandhu61 thoughts? I'm happy to write this up into a PR and see how everything works.

[benediamond]

The other option would be to simply use seconds as standard ethereum and IBFT already do, and then disable the requirement that new blocks' timestamps be strictly greater than their parents'.

[lundmikkel]

@benediamond Glad you wanna have a look at it.

I'm not sure what the best solution is, and what can be done on your part. All I know, is that it causes problems that are somewhat difficult to handle.

We normally expect seconds in our production code. But in order to make tests faster we use Raft. Having to know the time resolution makes it super difficult to write nice code, that doesn't need to know about magic values.

Especially parsing event timestamps suddenly requires knowledge about the resolution.

[SatpalSandhu61]

@benediamond I agree, your approach of modifying the EVM to adjust the returned block timestamp makes sense, and is better than the approach I was considering (to add a precompiled contract which could be used to return the adjusted timestamp). It would also be a good idea to modify getBlock to match.

I think that would be safer than attempting to modify Raft to use 1 second resolution.

[SatpalSandhu61]

BTW I recently added handler.go::getConsensusAlgorithm() which returns the consensus mechanism in use via the public ProtocolManager.NodeInfo() call. We should use that.

[SatpalSandhu61]

Just adding some notes, following on from offline conversations. There are, I believe, the following different ways to implement this change:

1. Add a precompiled contract that returns block.timestamp in seconds, adjusted for Raft.
 Advantages: It doesn’t impact existing users in any way.
 Disadvantages: It would require users to explicitly call the precompiled contract to get the time. Also, it doesn’t fix the issue for users who access the timestamp via any of the API’s.
2. Modify the EVM TIMESTAMP instruction so that it returns an adjusted timestamp. This may have to be done using a genesis block switch to avoid impacting any existing contracts which could rely on the timestamp being in nanoseconds.
 Advantages: All new contracts would automatically get the correct, adjusted timestamp.
 Disadvantages: It doesn’t fix the issue for users who access the timestamp via any of the API’s.
3. Modify Raft so that the block.timestamp is in seconds, as for other consensus implementations. Raft can use another field (or the vanity space) to store the nanosecond time.
 Advantages: All new contracts would automatically get the correct, adjusted timestamp. It will also fix the issue for users/apps that access the timestamp via any of the API’s
. Disadvantages: If any users/contracts/apps currently expect the timestamp to be in nanoseconds, then they may be adversely impacted. This can be mitigated by using a genesis block switch.   For items 1 and 2, there is also the additional complication that we need to somehow obtain the details of the consensus implementation in the EVM. This has been problematic so far.

[benediamond]

hey @lundmikkel, just following up on this.

I've written a new branch of quorum, called seconds, which solves this issue. Just add "raftSeconds": true within the "config": { ... } block of your genesis.json file and run raft-init.sh freshly. Everything should be denominated in seconds at that point.

This is still undergoing final review, but we should raise a PR which explains everything further within the coming days. thanks for your patience and for raising the issue!