feat: transaction with `CONTRACT_NEGATIVE_VALUE` breaks some routes #3387

natanasow · 2025-01-14T15:09:10Z

Description:

It seems like eth_getBlockByNumber fails because one of the transactions in the block was a CONTRACT_NEGATIVE_VALUE failure. The expectation is for the block to be successfully returned.

Steps to reproduce:

call eth_getBlockByNumber on block 73298898
see "Error invoking RPC: Invalid value - cannot pass negative number" response

The problematic transaction is here

Related issue(s):

Fixes #3367

Notes for reviewer:

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: nikolay <[email protected]>

github-actions · 2025-01-14T15:40:03Z

Test Results

18 files - 7 236 suites - 151 33m 2s ⏱️ - 40m 3s
612 tests - 3 608 ✅ + 18 4 💤 ±0 0 ❌ - 21
628 runs - 410 624 ✅ - 384 4 💤 - 1 0 ❌ - 25

Results for commit fa9f894. ± Comparison against base commit 67b61c6.

This pull request removes 3 tests.

"after all" hook for "@release should execute "eth_estimateGas" for contract call, using a standard websocket" ‑ RPC Server Acceptance Tests Acceptance tests @web-socket-batch-1 eth_estimateGas "after all" hook for "@release should execute "eth_estimateGas" for contract call, using a standard websocket"
"after all" hook in "RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests "after all" hook in "RPC Server Acceptance Tests"
"before each" hook for "should execute "eth_getStorageAt" request to get current state changes" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before each" hook for "should execute "eth_getStorageAt" request to get current state changes"

♻️ This comment has been updated with latest results.

Signed-off-by: nikolay <[email protected]>

quiet-node

Great work overall. Blocking as one warning needed to be addressed.

Also, is it feasible to add an e2e test to avoid regression?

packages/relay/src/formatters.ts

acuarica · 2025-01-16T02:47:11Z

packages/relay/src/formatters.ts

+
+    // we're using 64 bits integer because that the type returned by the mirror node - int64
+    const bits = 64;
+    return numberTo0x((BigInt(input.toString()) + (BigInt(1) << BigInt(bits))) % (BigInt(1) << BigInt(bits)));


question: do we expect number bigger than 53 bits? so it's not automatically coerced to double https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Yep, I don't know how often it will happen but we definitely must handle values bigger than Number.MAX_SAFE_INTEGER.

The max Int64 positive value is 0x7fffffffffffffff which has 63 "usable" bits.

0111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111

That's why we always convert the incoming value to BigInt here 👇

BigInt(input.toString())

Hope that makes sense or am I wrong?

Mmm, I'm not sure that BigInt(input.toString()) works as expected when input being a number uses more than 53 bits. Say

> const x = 0x7fffffffffffffff > typeof(x) 'number' > BigInt(x.toString()) - 0x7fffffffffffffffn 193n

but the difference should be zero, right?

You're right, the difference should be zero. Nice catch 👏 and thank you. That's something I must add more unit tests tomorrow.

Here are some more quick tests, it seems that .toString() messed things up, also using a shadow conversion like postfixing n doesn't seem to work as expected.

> const x = 0x7fffffffffffffff undefined > typeof(x) 'number' > BigInt(x.toString()) - 0x7fffffffffffffffn 193n > BigInt(x) - 0x7fffffffffffffffn 1n > BigInt(x) - BigInt(0x7fffffffffffffff) 0n > BigInt(x) - BigInt(9223372036854776000) 0n

Do you have an idea how this can be completely fixed? I think removing .toString() is enough but that will be confirmed when I push the unit tests covering wide-bit range numbers.

I think removing .toString() is enough

The problem is with number altogether.

> BigInt(0x7fffffffffffffff) - 0x7fffffffffffffffn 1n

The only way I see to proper fix it completely would be to not use number, and use bigint everywhere for values that are int64.

Makes sense. Will try to refactor it in an elegant way and once I'm done will ping you again for review.

Hey @acuarica,

Richard Moore has the same thoughts as you ethers-io/ethers.js#452 (comment). Today I managed to resolve it but what do you think about the implementation?

What I've done:

decided to use json-bigint

hooked up into axios's transformResponse method

parsed the json string before it's been parsed by the default JSON.parse where the number will be rounded and lost forever

Here are the two mainnet transactions I tested with:

negative number - https://hashscan.io/mainnet/transaction/1735241436.856862230

positive 62 bits number - https://hashscan.io/mainnet/transaction/1735223396.774976000

What would be the advantage of using json-bigint over bigint? Only parsing? doesn't bigint already comes with parsing capabilities?

json-bigint is doing JSONBigInt.parse(data) but under the hood, it transforms numeric values to bigint instead of JS Number.

If we're doing JSON.parse() and then manually transforming numeric values to bigint, JSON.parse() already rounded the number and we lost it.

json-bigint doesn't replace bigint and they are not correlated. json-bigint is used instead of JSON.parse() due to its capability of converting string json input directly to bigint instead of JS Number.

Thanks for the clarification.

Signed-off-by: nikolay <[email protected]>

natanasow · 2025-01-16T09:51:48Z

Great work overall. Blocking as one warning needed to be addressed.

Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.

I have no problem adding that test if you insist 🚀 🙏.

quiet-node · 2025-01-17T02:11:11Z

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.

I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

packages/relay/src/formatters.ts

Signed-off-by: nikolay <[email protected]>

sonarqubecloud · 2025-01-17T12:07:36Z

Quality Gate failed

Failed conditions
34.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

natanasow · 2025-01-17T13:28:11Z

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.
I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

Sending a negative amount via ContractCallTransaction will be enough while it accepts int64 (not uint64).

Here is the link to the doc https://docs.hedera.com/hedera/sdks-and-apis/hedera-api/readme-1-1/contractcall#contractcalltransactionbody.

codecov · 2025-01-17T15:26:55Z

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Project coverage is 64.10%. Comparing base (67b61c6) to head (fa9f894).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/relay/src/formatters.ts	41.66%	5 Missing and 2 partials ⚠️
packages/relay/src/lib/clients/mirrorNodeClient.ts	57.14%	2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (67b61c6) and HEAD (fa9f894). Click for more details.

HEAD has 4 uploads less than BASE

Flag BASE (67b61c6) HEAD (fa9f894)

config-service 1 0

relay 1 0

server 1 0

ws-server 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3387       +/-   ##
===========================================
- Coverage   84.87%   64.10%   -20.77%     
===========================================
  Files          69       65        -4     
  Lines        4742     4550      -192     
  Branches     1067     1043       -24     
===========================================
- Hits         4025     2917     -1108     
- Misses        400     1106      +706     
- Partials      317      527      +210

Flag	Coverage Δ
config-service	`?`
relay	`?`
server	`?`
ws-server	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
packages/relay/src/lib/clients/mirrorNodeClient.ts	`62.62% <57.14%> (-26.13%)`	⬇️
packages/relay/src/formatters.ts	`63.97% <41.66%> (-2.04%)`	⬇️

... and 49 files with indirect coverage changes

quiet-node · 2025-01-17T18:54:59Z

Great work overall. Blocking as one warning needed to be addressed.
Also, is it feasible to add an e2e test to avoid regression?

Adding an e2e test is not a straightforward process because that kind of revert can't be reproduced with an EthereumTransaction. In order to re-create it we have to execute ContractCall SDK call which is out of the relay scope IMO.
I have no problem adding that test if you insist 🚀 🙏.

Ah I see. Just curious how would you reproduice the problem?

Sending a negative amount via ContractCallTransaction will be enough while it accepts int64 (not uint64).

Here is the link to the doc https://docs.hedera.com/hedera/sdks-and-apis/hedera-api/readme-1-1/contractcall#contractcalltransactionbody.

Ah I see. So if it's feasible and do not take crazy amount of time, I would encourage we have an e2e just to make sure no regression later. Thanks much Nikki!

chore: handle contract negative value calls

bde9a91

Signed-off-by: nikolay <[email protected]>

natanasow self-assigned this Jan 14, 2025

natanasow added the bug Something isn't working label Jan 14, 2025

natanasow added this to the 0.65.0 milestone Jan 14, 2025

natanasow added 2 commits January 14, 2025 17:44

chore: remove .only

ec6dc59

Signed-off-by: nikolay <[email protected]>

chore: fix comments

48ce0a3

Signed-off-by: nikolay <[email protected]>

natanasow marked this pull request as ready for review January 14, 2025 15:55

natanasow requested review from a team as code owners January 14, 2025 15:55

natanasow requested a review from stoyanov-st January 14, 2025 15:55

chore: edit imports

c17f02a

Signed-off-by: nikolay <[email protected]>

quiet-node requested changes Jan 15, 2025

View reviewed changes

packages/relay/src/formatters.ts Outdated Show resolved Hide resolved

packages/relay/src/formatters.ts Outdated Show resolved Hide resolved

acuarica reviewed Jan 16, 2025

View reviewed changes

chore: resolving comments

5e9e1b1

Signed-off-by: nikolay <[email protected]>

natanasow requested review from acuarica and quiet-node January 16, 2025 14:56

quiet-node reviewed Jan 17, 2025

View reviewed changes

packages/relay/src/formatters.ts Outdated Show resolved Hide resolved

chore: fix json parsing

fa9f894

Signed-off-by: nikolay <[email protected]>

natanasow requested a review from quiet-node January 17, 2025 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transaction with `CONTRACT_NEGATIVE_VALUE` breaks some routes #3387

feat: transaction with `CONTRACT_NEGATIVE_VALUE` breaks some routes #3387

natanasow commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

quiet-node left a comment

acuarica Jan 16, 2025

natanasow Jan 16, 2025

acuarica Jan 16, 2025

natanasow Jan 16, 2025

acuarica Jan 16, 2025

natanasow Jan 16, 2025

natanasow Jan 17, 2025

acuarica Jan 17, 2025

natanasow Jan 17, 2025 •

edited

Loading

acuarica Jan 17, 2025

natanasow commented Jan 16, 2025

quiet-node commented Jan 17, 2025

sonarqubecloud bot commented Jan 17, 2025

natanasow commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025

quiet-node commented Jan 17, 2025

feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes #3387

Are you sure you want to change the base?

feat: transaction with CONTRACT_NEGATIVE_VALUE breaks some routes #3387

Conversation

natanasow commented Jan 14, 2025 • edited Loading

github-actions bot commented Jan 14, 2025 • edited Loading

Test Results

quiet-node left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natanasow Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natanasow commented Jan 16, 2025

quiet-node commented Jan 17, 2025

sonarqubecloud bot commented Jan 17, 2025

Quality Gate failed

natanasow commented Jan 17, 2025 • edited Loading

codecov bot commented Jan 17, 2025

Codecov Report

quiet-node commented Jan 17, 2025

feat: transaction with `CONTRACT_NEGATIVE_VALUE` breaks some routes #3387

feat: transaction with `CONTRACT_NEGATIVE_VALUE` breaks some routes #3387

natanasow commented Jan 14, 2025 •

edited

Loading

github-actions bot commented Jan 14, 2025 •

edited

Loading

natanasow Jan 17, 2025 •

edited

Loading

natanasow commented Jan 17, 2025 •

edited

Loading