Skip to content
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

Transaction signature bytes always use an uninitialized timestamp #810

Closed
zivkovicmilos opened this issue May 8, 2023 · 1 comment · Fixed by #1939
Closed

Transaction signature bytes always use an uninitialized timestamp #810

zivkovicmilos opened this issue May 8, 2023 · 1 comment · Fixed by #1939
Assignees
Labels
🐞 bug Something isn't working 📦 🌐 tendermint v2 Issues or PRs tm2 related

Comments

@zivkovicmilos
Copy link
Member

zivkovicmilos commented May 8, 2023

Description

The signature base for transactions is generated from the SignDoc structure, which has a field called Timestamp (ISO string).

This Timestamp field goes into the transaction signature bytes uninitialized (as the default value of time.Time: 0001-01-01T00:00:00Z).

gno/tm2/pkg/std/doc.go

Lines 19 to 38 in 0aaee73

type SignDoc struct {
ChainID string `json:"chain_id" yaml:"chain_id"`
AccountNumber uint64 `json:"account_number" yaml:"account_number"`
Time time.Time `json:"time" yaml:"time"`
Sequence uint64 `json:"sequence" yaml:"sequence"`
Fee Fee `json:"fee" yaml:"fee"`
Msgs []Msg `json:"msgs" yaml:"msgs"`
Memo string `json:"memo" yaml:"memo"`
}
// SignBytes returns the bytes to sign for a transaction.
func SignBytes(chainID string, accountNumber uint64, sequence uint64, fee Fee, msgs []Msg, memo string) []byte {
bz, err := amino.MarshalJSON(SignDoc{
ChainID: chainID,
AccountNumber: accountNumber,
Sequence: sequence,
Fee: fee,
Msgs: msgs,
Memo: memo,
})

This means that the Timestamp for any transaction signature needs to be 0001-01-01T00:00:00Z, as the actual transaction timestamp is not read at all (doesn't go into SignDoc).

Of course, this means that any transaction signature that has a valid timestamp (different from 0001-01-01T00:00:00Z) will be considered invalid, because the gno verifier code generates the sign bytes assuming it's the default value

@moul
I'm not sure if we'd be breaking backwards compatibility by making sure the transaction sign bytes take into account the valid transaction signature timestamp instead of the default value

@zivkovicmilos zivkovicmilos changed the title Transaction signature bytes require an uninitialized timestamp Transaction signature bytes always use an uninitialized timestamp May 8, 2023
@zivkovicmilos zivkovicmilos self-assigned this May 8, 2023
@ajnavarro ajnavarro added 🐞 bug Something isn't working 📦 🌐 tendermint v2 Issues or PRs tm2 related and removed bug labels May 15, 2023
@moul moul moved this to 🏆 Needed for Launch in 🚀 The Launch [DEPRECATED] Sep 5, 2023
@moul moul added this to the 🚀 main.gno.land milestone Sep 6, 2023
@moul
Copy link
Member

moul commented Mar 19, 2024

Related with #1511

@zivkovicmilos zivkovicmilos moved this from Backlog to Todo in 🧙‍♂️gno.land core team Apr 3, 2024
@zivkovicmilos zivkovicmilos moved this from Todo to In Progress in 🧙‍♂️gno.land core team Apr 18, 2024
zivkovicmilos added a commit that referenced this issue Apr 29, 2024
## Description

This PR closes #810 

I've dropped the unused `Timestamp` field from the `SignDoc`, which
forced all clients that generate tx signatures outside of the `gno` repo
to initialize it to an empty UTC timestamp value (ex. Adena,
`tm2-js-client`...)

**BREAKING CHANGE**: all clients that generate transaction signatures
without using the provided Go methods in this repository need to update
their source code to remove the `Timestamp` field. (@dongwon8247
@jefft0)

I've also provided error handling for generating these signature bytes,
which weren't present before.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🧙‍♂️gno.land core team Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: 🚀 Needed for Launch
Development

Successfully merging a pull request may close this issue.

3 participants