search

hashgraph / hedera-json-rpc-relay

Implementation of Ethereum JSON-RPC APIs for Hedera
Apache License 2.0
65 stars 70 forks source link

Restore `v` value in Transaction model #1800

Closed Neurone closed 1 year ago

Neurone commented 1 year ago

Problem

2930 and 1559 transactions introduced the yParity parameter in the Transaction model and removed the v parameter.

These modifications still struggle to spread across the Ethereum ecosystem, so a few months ago, even the official specifications were updated to include the v parameter again in the Transaction model.

That would still be an optional, deprecated parameter so it will be removed in the future, but this update gives more time to the ecosystem to upgrade libraries and clients.

As Hedera wants to be as compliant as possible with the Ethereum libraries and tools ecosystem, I suggest following the new JSON-RPC specs and reintroducing the v parameter.

The behavior of the yParity does not change.

What if we don't apply this? Strange things can happen, and sometimes really hard to debug.

For example, I experienced problems with ethers-rs yesterday while deploying some smart contracts.

That library does not yet recognize the yParity parameter, so it considers the absence of the v parameter a non-conformity. As a result, ethers-rs keeps asking multiple times for the same information to the JSON-RPC Relay, thinking the result is corrupted, and essentially bombing the relay until the TIER_2_RATE_LIMIT is reached and the IP_RATE_LIMIT_EXCEEDED error appears.

All happens silently under the hood, so the users are not aware of what's happening, and they can 1) think it's a Hedera problem and 2) be locked out of other legitim requests because that behaviour eroded silently their requests limit.

Please note this issue can be related to ongoing activities on #1730

Solution

As Hedera wants to be as compliant as possible with the Ethereum libraries and tools ecosystem, I suggest following the new JSON-RPC specs and reintroducing the v parameter.

The behavior of the yParity does not change.

Alternatives

No response

Neurone commented 1 year ago

I have already started a PR in my local repo to introduce this change, but I stopped because I realized today that this could potentially be a topic that requires a broader discussion. Please let me know if you want me to send a corresponding PR.

Nana-EC commented 1 year ago

@konstantinabl and @georgi-l95 what do you think?

konstantinabl commented 1 year ago

@Nana-EC @Neurone Well, it is true that in JSON RPC Specs the v value is still present. Although it is listed as deprecated, it is still returned for backwards compatibility. I thought it is fully deprecated and did not account for the fact that some libraries may still use it, therefore I removed it from the response. If some libraries are still using it and do not recognise yParity maybe we should leave the v. However, we should also think how widely used are those libraries and do they plan on conforming to the JSON RPC specs and use yParity

Nana-EC commented 1 year ago

@Nana-EC @Neurone Well, it is true that in JSON RPC Specs the v value is still present. Although it is listed as deprecated, it is still returned for backwards compatibility. I thought it is fully deprecated and did not account for the fact that some libraries may still use it, therefore I removed it from the response. If some libraries are still using it and do not recognise yParity maybe we should leave the v. However, we should also think how widely used are those libraries and do they plan on conforming to the JSON RPC specs and use yParity

Valid points. For now to avoid breaking any developers let's just add it back since it's in the spec and we don't know how compliant tools are. We can always easily remove it once we're clear deprecation is being adhered to

AlfredoG87 commented 1 year ago

@konstantinabl, in order to complete ticket: #1824, I have addressed this issue. on PR: #1834