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: Precision loss for gas calculation of HTS system contracts v2 #15446

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

stoyanov-st
Copy link
Contributor

Description:

NOTE: This PR is copied from PR
Due to DCO issues we had to open this one with the exact changes and linked issues.

Original description from initial PR
Main issues are two

  • Gas differs as exchange rate changes!
  • Fixed view gas cost of 100 units, does not represent accurately actual 0.0001$ cost.

Those results represent 3 real and possible price schemes for hBar:

Values for fields in ExchangeRate.java
1hbar = ~$0.005 | hbarEquiv = 30k; centEquiv = 16197
1hbar = ~$0.12 | hbarEquiv = 30k; centEquiv = 359789
1hbar = ~$0,96 | hbarEquiv = 30k; centEquiv = 2888899

Test cases for contractCalls calling system contracts using the above exchange rates.
cryptoTransferNonFungible + auto-associate -> 747724, 748112, 823803 🤯
cryptoTransferNonFungible -> 43467, 43474, 44959

Conclusion, due to precision loss the higher the price for the operation & per hbar, the higher the discrepancies.

This PR aims to fix the precision loss by the following changes :

  • Instead of dividing the denominator with FEE_SCHEDULE_UNITS_PER_TINYCENT, multiply numerator with FEE_SCHEDULE_UNITS_PER_TINYCENT, thus avoid precision loss.
  • Remove the conversion from tinyCents to tinyBars, where possible, as it’s not needed and it allows for the exchange rate to affect gas when due to precision loss.
  • For view operation remove Fixed gas cost, and instead use the one from TOKEN_INFO, which results a change from 100 fixed gas units to 2607 gas units, at the current configuration, representing $0.0001 canonical price.

After those improvements, we will also see changes in gas. However, they are not as big as the possible fluctuations, which also result in breaking the narrative that gas measures just computational effort.

The actual differences that will happen are documented in the issue itself - here

Related issue(s):

Fixes #10828
#6993

Notes for reviewer:

All of those changes are guarded behind a feature flags.

There is an open issue #15098 which is tracking the task for removing the deprecated logic and isGasPrecisionLossFixEnabled & isCanonicalViewGasEnabled.

Checklist

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

@stoyanov-st stoyanov-st added this to the v0.55 milestone Sep 12, 2024
@stoyanov-st stoyanov-st self-assigned this Sep 12, 2024
@stoyanov-st stoyanov-st requested a review from a team September 12, 2024 16:28
@stoyanov-st stoyanov-st requested review from a team and tinker-michaelj as code owners September 12, 2024 16:28
lukelee-sl
lukelee-sl previously approved these changes Sep 12, 2024
Copy link
Contributor

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM. tyvm @stoyanov-st

Copy link

codacy-production bot commented Sep 12, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.01% (target: -1.00%) 59.52%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (5c06347) 109026 67361 61.78%
Head commit (c422486) 109074 (+48) 67380 (+19) 61.77% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#15446) 84 50 59.52%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

…s-calculation

# Conflicts:
#	hedera-node/hedera-config/src/main/java/com/hedera/node/config/data/ContractsConfig.java
Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 53.57143% with 39 lines in your changes missing coverage. Please review.

Project coverage is 58.23%. Comparing base (5c06347) to head (c422486).

Files with missing lines Patch % Lines
.../service/contract/impl/exec/gas/TinybarValues.java 25.00% 17 Missing and 1 partial ⚠️
...act/impl/exec/gas/SystemContractGasCalculator.java 56.25% 11 Missing and 3 partials ⚠️
...ce/contract/impl/exec/gas/CustomGasCalculator.java 0.00% 2 Missing ⚠️
...ntract/impl/exec/scope/HandleHederaOperations.java 50.00% 1 Missing and 1 partial ⚠️
...p/service/contract/impl/utils/ConversionUtils.java 0.00% 2 Missing ⚠️
...de/app/service/contract/impl/exec/QueryModule.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #15446      +/-   ##
=============================================
- Coverage      58.24%   58.23%   -0.02%     
- Complexity     21592    21596       +4     
=============================================
  Files           2779     2779              
  Lines         109209   109257      +48     
  Branches       11177    11180       +3     
=============================================
+ Hits           63609    63623      +14     
- Misses         41738    41767      +29     
- Partials        3862     3867       +5     
Files with missing lines Coverage Δ
...a/com/hedera/node/config/data/ContractsConfig.java 100.00% <ø> (ø)
.../service/contract/impl/exec/TransactionModule.java 100.00% <100.00%> (ø)
...p/service/contract/impl/exec/gas/DispatchType.java 100.00% <100.00%> (ø)
...systemcontracts/hts/create/ClassicCreatesCall.java 57.69% <100.00%> (ø)
...emcontracts/hts/transfer/ClassicTransfersCall.java 86.13% <100.00%> (ø)
...de/app/service/contract/impl/exec/QueryModule.java 72.72% <0.00%> (ø)
...ce/contract/impl/exec/gas/CustomGasCalculator.java 43.47% <0.00%> (ø)
...ntract/impl/exec/scope/HandleHederaOperations.java 95.88% <50.00%> (-1.15%) ⬇️
...p/service/contract/impl/utils/ConversionUtils.java 89.23% <0.00%> (-0.93%) ⬇️
...act/impl/exec/gas/SystemContractGasCalculator.java 68.88% <56.25%> (-22.03%) ⬇️
... and 1 more

Impacted file tree graph

Copy link
Contributor

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM from hedera-base changes. Will defer to @lukelee-sl for smart contract changes

@stoyanov-st stoyanov-st merged commit 1a31fa6 into develop Sep 13, 2024
50 of 53 checks passed
@stoyanov-st stoyanov-st deleted the 10828-fix-precision-loss-in-gas-calculation branch September 13, 2024 19:43
netopyr added a commit that referenced this pull request Sep 19, 2024
* develop: (22 commits)
  test: New HAPI test for TokenAirdrop transaction (#15348)
  test: unit test verifySyncInvalidEd25519() is not stable (#15534)
  chore: add `TracerBinding` interface for `TransactionExecutors`. (#15480)
  chore: Add missing javadocs in Consensus Service (#15299)
  fix: Validate `CustomFees` input arrays in `UpdateTokenCustomFeesDecoder` (#15520)
  feat: migrate event serialization to protobuf (#15417)
  fix: 15494: Improve VirtualLeafRecord serialization to bytes during flushes (#15512)
  docs: tss block signing proposal (#15160)
  fix: 15438: Eliminate busy loop in HalfDiskHashMap.endWriting() (#15439)
  build: cleanup settings.gradle.kts / remove build.gradle.kts (#15470)
  test: fix CryptographyTests (#15529)
  fix: recreate block hash from state (#15444)
  docs: Proposal Process Update - Specify post-acceptance non-material update procedure (#15447)
  chore: testnet event hashing (#15432)
  fix: 15167: Remove timeout from reconnect/rehash Iterators (#15468)
  chore: remove snapshot ops (#15462)
  feat: Add TokenUpdateNFTs as a smart contract operation v2 (#15445)
  feat: introduce PbjRecordHasher and RosterUtils.hash(Roster) (#15457)
  fix: Precision loss for gas calculation of HTS system contracts v2 (#15446)
  chore: correct the variable name in roster.proto (#15465)
  ...

# Conflicts:
#	hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/scope/HandleHederaOperations.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gas calculation for precompiles incorrectly varies with HAPI exchange rate
3 participants