feat: transactions in the same block have different timestamps #1777
Conversation
github-actions
bot
added
🧾 package/realm
deelawn
changed the title
deelawn
requested review from
jaekwon,
piux2,
thehowl,
moul,
a team and
zivkovicmilos
as code owners
|
|
||
| // This is an arbitrary value that is increased after each transaction so that each transaction in the same block | ||
| // retrieve a unique time when calling `time.Now()`. | ||
| app.txCurrentTimeOffset += 100 |
There was a problem hiding this comment.
If we change this arbitrary value of 100 in production, it will cause backward compatibility issues for contracts that depend on the timestamp value. The offset should come from the block itself, otherwise it could leave potential breaking logics in the contract when the timestamp value is stored in realm states and compared with stored value in the same block, and later we change arbitrary value != 100. Also, what about multiple messages per transaction?
There was a problem hiding this comment.
what about multiple messages per transaction?
Our use case in the social app is calling a realm function like PostMessage which adds the message with its timestamp from time.Now(). I think it's reasonable that time.Now() returns the same value for each call in the same transaction. It think it is OK to explain to devs that a single call to a realm function runs "instantaneously". The only thing we're asking for in our use case is that a transaction which is submitted at a different time has a different timestamp. (Maybe the transactions are collected into one block but this is a processing detail that the devs shouldn't have to worry about.)
There was a problem hiding this comment.
Yes, the intention here is to keep the same timestamp per transaction so each message has the same and each transaction timestamp in the block is different. We could store the offset in the block; I need to think about this more.
There was a problem hiding this comment.
I agree with @piux2 -- I'm heavily leaning towards having this be a block-level thing, rather than something that's injected through the app. We can most probably keep this kind of logic in the execution context (we as the callers of the VM control the execution context).
This way we eliminate the need for bloating the app with an offset, and propagating it out to all transactions (even ones that aren't GnoVM transactions)
|
From the review meeting: I kind of dislike this approach, as it merges in what is actually the "transaction index" (within the block) together with the block time, creating a The proposed/agreed solution is that of exposing instead |
|
Will pick this up again today to make the suggested changes. |
|
can you please merge in the latest |
zivkovicmilos
added
the
breaking change
| @@ -9,8 +9,8 @@ import ( | |||
| type ExecContext struct { | |||
| ChainID string | |||
| Height int64 | |||
| Timestamp int64 // seconds | |||
| TimestampNano int64 // nanoseconds, only used for testing. | |||
| Timestamp int64 | |||
There was a problem hiding this comment.
Do we need to keep Timestamp if we have the nano version?
Can't we just make the Timestamp be the nano version?
| @@ -27,6 +27,21 @@ type InMemoryNodeConfig struct { | |||
| GenesisMaxVMCycles int64 | |||
| } | |||
|
|
|||
| func (cfg *InMemoryNodeConfig) AddGenesisBalances(balances ...Balance) error { | |||
There was a problem hiding this comment.
Not sure why this was added?
| "github.com/gnolang/gno/tm2/pkg/crypto" | ||
| "github.com/gnolang/gno/tm2/pkg/crypto/keys" | ||
| "github.com/gnolang/gno/tm2/pkg/log" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| const testChainID string = "tendermint_test" |
There was a problem hiding this comment.
We should move this into the test, even if we redeclare it
Not a huge fan of having package-level test globals
| @@ -65,7 +72,7 @@ func TestCallMultiple_Integration(t *testing.T) { | |||
| defer node.Stop() | |||
|
|
|||
| // Init Signer & RPCClient | |||
| signer := newInMemorySigner(t, "tendermint_test") | |||
| signer := defaultInMemorySigner(t, "tendermint_test") | |||
There was a problem hiding this comment.
I'm laughing that we defined a var but never used it here :)
| t.Helper() | ||
|
|
||
| mnemonic := integration.DefaultAccount_Seed | ||
| name := integration.DefaultAccount_Name | ||
| keybase := keys.NewInMemory() |
There was a problem hiding this comment.
Nitpick: this can be inlined
| if res.err != nil { | ||
| t.Errorf("unexpected error %v", res.err) | ||
| } |
There was a problem hiding this comment.
Nitpick:
| if res.err != nil { | |
| t.Errorf("unexpected error %v", res.err) | |
| } | |
| require.NoError(t, res.err) |
| if len(results) != 2 { | ||
| t.Errorf("expected 2 results, got %d", len(results)) | ||
| } |
There was a problem hiding this comment.
Nitpick:
| if len(results) != 2 { | |
| t.Errorf("expected 2 results, got %d", len(results)) | |
| } | |
| require.Len(t, results, 2) |
| if i < maxTries { | ||
| continue | ||
| } | ||
| t.Errorf("expected same height, got %d and %d", results[0].Height, results[1].Height) |
There was a problem hiding this comment.
Nitpick:
| if i < maxTries { | |
| continue | |
| } | |
| t.Errorf("expected same height, got %d and %d", results[0].Height, results[1].Height) | |
| require.Less(t, i, maxTries) |
| extractInt := func(data []byte) int64 { | ||
| parts := strings.Split(string(data), " ") | ||
| numStr := parts[0][1:] | ||
| num, err := strconv.ParseInt(numStr, 10, 64) | ||
| if err != nil { | ||
| t.Errorf("unable to parse number from string %s", string(data)) | ||
| } | ||
|
|
||
| return num | ||
| } |
There was a problem hiding this comment.
Why not declare this outside the loop?
|
|
||
| // This is an arbitrary value that is increased after each transaction so that each transaction in the same block | ||
| // retrieve a unique time when calling `time.Now()`. | ||
| app.txCurrentTimeOffset += 100 |
There was a problem hiding this comment.
I agree with @piux2 -- I'm heavily leaning towards having this be a block-level thing, rather than something that's injected through the app. We can most probably keep this kind of logic in the execution context (we as the callers of the VM control the execution context).
This way we eliminate the need for bloating the app with an offset, and propagating it out to all transactions (even ones that aren't GnoVM transactions)
|
I don't think we want this any more |
Implements #1709. This is a BREAKING CHANGE that will break code that assumes transactions in the same block will have the same timestamp. The strategy here is to add 100 nanoseconds on to the timestamp after each transaction in a block. The counter is reset to zero at the end of each block.
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description