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

Remove v of non-legacy tx signature from RPC response #5927

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

rubo
Copy link
Contributor

@rubo rubo commented Jul 12, 2023

Changes

Removed the v component of the non-legacy transaction signatures from the RPC response.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Also, needs manual testing.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

This fix should address the Nethermind's part of the issue with ethers v6.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Looks good, but can you explain the reasoning?

@rubo
Copy link
Contributor Author

rubo commented Jul 13, 2023

Looks good, but can you explain the reasoning?

@LukaszRozmej Please see the remarks

@benaadams
Copy link
Member

This fix should address the Nethermind's part of ethers-io/ethers.js#3835 with ethers v6.

Since causes issue in major 3rd party lib should this be a candidate for pulling into 1.20.0?

@MarekM25 @kamilchodola

@rubo
Copy link
Contributor Author

rubo commented Jul 13, 2023

should this be a candidate for pulling into 1.20.0?

@benaadams Yes

@rubo
Copy link
Contributor Author

rubo commented Jul 13, 2023

Note that this PR alone won't address ethers' issue as it doesn't handle yParity in hex.

@rubo rubo marked this pull request as ready for review July 13, 2023 13:36
@rubo rubo merged commit 691bf12 into master Jul 13, 2023
65 checks passed
@rubo rubo deleted the hotfix/rpc-tx-signature branch July 13, 2023 13:42
kamilchodola added a commit that referenced this pull request Aug 1, 2023
* Add missing peer types to limits (#5838)

* Add missing peer types to limits

* Add Reth

* Flaky test

* Use better processing timer for logging (#5843)

* Use different processing timer

* Use single queue

* Revert to simpler

* Add Gnosis Shanghai hard-fork timestamp (#5848)

* Don't use DarkGray (#5849)

* Bump to 1.20.0-rc

* MaxDegreeOfParallelism defaults for full pruning (#5662)

* other defaults for full pruning?

* degreeOfParalleism

* small refactor

* fix build

* Fixing BatchedTrieVistior

* 25% of cores

* Update Pruning config

* add logger

* fix

* Perf/smoother peer discovery (#5846)

* Continuously connect

* Added another delay due to disconnect

* Increase useless peer timeout

* Simple rate limiter

* Integrate with peer manager

* Adjust some stats

* Check all peer, and uses pending variable

* Make logic clearer

* Minor cleanup

* Missed cancellation token

* Cancel setup outgoing peer connection if throttled

* Lockless ratelimiter

* Addressing comment

* Minor adjustments

* Whitespace

* Having trouble getting candidate at higher speed

* Make test more consistent

* Even more lenient

(cherry picked from commit 6ab0a3a)

* Update RocksDB package (#5883)

(cherry picked from commit 4e2bf0f)

* Update DotNetty feed

(cherry picked from commit 3e330ad)

* Update DotNetty package

(cherry picked from commit 47a92ee)

* add more gnosis bootnodes (#5910)

(cherry picked from commit b7086a9)

* Remove `v` of non-legacy tx signature from RPC response (#5927)

* minor fixes

* Update version to 1.20.0

* Bump to 1.20.1

* Restore `v` in tx signature for Geth compatibility (#5937)

* Restore DotNetty feed (#5976)

* Revert `nuget.config`

* Update DotNetty package

---------

Co-authored-by: Ben Adams <[email protected]>
Co-authored-by: Ruben Buniatyan <[email protected]>
Co-authored-by: Kamil Chodoła <[email protected]>
Co-authored-by: Marek Moraczyński <[email protected]>
Co-authored-by: Amirul Ashraf <[email protected]>
Co-authored-by: Marcin Sobczak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants