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

fix: remove empty timestamp from the tx signature payload #1939

Merged
merged 7 commits into from
Apr 29, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Apr 16, 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.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the breaking change Functionality that contains breaking changes label Apr 16, 2024
@zivkovicmilos zivkovicmilos self-assigned this Apr 16, 2024
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Apr 16, 2024
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 70.14925% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 46.67%. Comparing base (7bf662a) to head (11ad828).

Files Patch % Lines
tm2/pkg/std/tx.go 0.00% 9 Missing ⚠️
tm2/pkg/std/doc.go 57.14% 2 Missing and 1 partial ⚠️
gno.land/pkg/gnoclient/signer.go 33.33% 1 Missing and 1 partial ⚠️
tm2/pkg/crypto/keys/client/sign.go 77.77% 1 Missing and 1 partial ⚠️
tm2/pkg/sdk/auth/ante.go 84.61% 1 Missing and 1 partial ⚠️
tm2/pkg/std/utils.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1939      +/-   ##
==========================================
+ Coverage   46.64%   46.67%   +0.02%     
==========================================
  Files         492      492              
  Lines       69624    69657      +33     
==========================================
+ Hits        32476    32510      +34     
+ Misses      34442    34435       -7     
- Partials     2706     2712       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jefft0
Copy link
Contributor

jefft0 commented Apr 16, 2024

Hi @zivkovicmilos . Thanks for the head's up. Gno Native Kit relies on gnoclient to sign. Is this signing code affected by this PR?

@zivkovicmilos
Copy link
Member Author

Hi @zivkovicmilos . Thanks for the head's up. Gno Native Kit relies on gnoclient to sign. Is this signing code affected by this PR?

If you're using the provided Go signing methods (from the repo) there is no action needed on your part -- a change is required only for non-Go clients (JS / TS, Rust...).

The signing code you've linked is already good to go (I've updated the gnoclient signer in this PR) 👌

@zivkovicmilos zivkovicmilos marked this pull request as ready for review April 19, 2024 09:27
@zivkovicmilos zivkovicmilos requested review from moul, jaekwon, a team and piux2 as code owners April 19, 2024 09:27
@zivkovicmilos zivkovicmilos added the don't merge Please don't merge this functionality temporarily label Apr 19, 2024
@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Apr 19, 2024

@jinoosss @dongwon8247
Let's coordinate an Adena + tm2-js-client release before merging this out for the Portal Loop

@moul
Copy link
Member

moul commented Apr 21, 2024

I believed that having this field could be beneficial. If initializing an empty time field is complex, why not make it nullable? Are there any examples of using this field where it's not empty?

@jinoosss
Copy link

@zivkovicmilos
Adena only needs to follow the tm2-js-client version. (using only the signature of tm2-js-client)

@zivkovicmilos
Copy link
Member Author

I believed that having this field could be beneficial. If initializing an empty time field is complex, why not make it nullable? Are there any examples of using this field where it's not empty?

The transaction timestamp can never be set with the current way the transactions are structured and parsed -- they don't have a timestamp field like in Ethereum (which doesn't go into the signature bytes). Ethereum transactions signatures are just signed RLP encodings (Ethereum encoding standard) of the following data points: nonce, gas price, gas limit, to, value, data, with additional things like gas fee info being tacked on as EIPs are enabled ("legacy" and "non-legacy" transactions)

Making the field nullable now introduces another logic block, where the node would need to reconstruct 2 tx signature bytes (one with an empty timestamp, the other without a timestamp) to verify that a signature is valid / invalid (and one of these will always be invalid). This signature bytes moment needs to be deterministic and simple on both the client (generating the sig) and the node (verifying the sig), to avoid overhead

@zivkovicmilos zivkovicmilos removed the don't merge Please don't merge this functionality temporarily label Apr 24, 2024
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the review meeting: good to go 👍

@zivkovicmilos zivkovicmilos merged commit 3832b13 into gnolang:master Apr 29, 2024
191 checks passed
@zivkovicmilos zivkovicmilos deleted the fix/standardize-tx-sig branch April 29, 2024 12:01
zivkovicmilos added a commit that referenced this pull request Apr 29, 2024
## Description

This PR updates the `gno` and `faucet` versions for the `gnofaucet` to
utilize the latest ones, that incorporate breaking changes recently
merged to `master`.

related: #1939 

<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
- [x] 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>
D4ryl00 added a commit to gnolang/gnonative that referenced this pull request Apr 30, 2024
This PR updates the version of Gno. Now gnonative uses the latest
Goclient version to support the new signature system:
gnolang/gno#1939

The PR also adds a new ErrCode used by the API method `SetRemote`
because the new GnoClient can now return an error.

This PR adds a GitHub Action workflow to automate the push on the Buf
registry. I also asked someone to add the repo secret `BUF_TOKEN`.

PR tested on Gnoboard with local gnodev.

---------

Signed-off-by: D4ryl00 <d4ryl00@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Transaction signature bytes always use an uninitialized timestamp
6 participants