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

feat: Validate non-standard hedera.shard/hedera.realm configurations #17460

Merged
merged 56 commits into from
Feb 10, 2025

Conversation

vtronkov
Copy link
Contributor

@vtronkov vtronkov commented Jan 21, 2025

Description:
Changing the shard/realm from the default to the values from the configuration

Related issue(s):

Fixes #17726

Checklist

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

JivkoKelchev and others added 5 commits January 21, 2025 13:09
Copy link

codacy-production bot commented Jan 21, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.04% (target: -1.00%) 93.48%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (dff9142) 99248 72374 72.92%
Head commit (583fbe8) 99210 (-38) 72304 (-70) 72.88% (-0.04%)

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 (#17460) 46 43 93.48%

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

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 68.96%. Comparing base (dff9142) to head (583fbe8).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...in/impl/handlers/ReadableFreezeUpgradeActions.java 80.00% 3 Missing ⚠️
...ervice/token/impl/validators/StakingValidator.java 50.00% 0 Missing and 2 partials ⚠️
...sus/impl/handlers/ConsensusUpdateTopicHandler.java 0.00% 0 Missing and 1 partial ⚠️
...ice/token/impl/handlers/FinalizeRecordHandler.java 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #17460      +/-   ##
============================================
- Coverage     69.01%   68.96%   -0.05%     
+ Complexity    23009    22981      -28     
============================================
  Files          2647     2646       -1     
  Lines         99465    99427      -38     
  Branches      10289    10280       -9     
============================================
- Hits          68641    68569      -72     
- Misses        26940    26971      +31     
- Partials       3884     3887       +3     
Files with missing lines Coverage Δ
...ddressbook/impl/schemas/V053AddressBookSchema.java 94.59% <100.00%> (+0.20%) ⬆️
...om/hedera/node/app/spi/validation/Validations.java 96.00% <ø> (ø)
...ws/standalone/impl/NoopVerificationStrategies.java 100.00% <ø> (ø)
...chedule/impl/handlers/AbstractScheduleHandler.java 86.82% <100.00%> (+0.20%) ⬆️
...service/token/impl/handlers/BaseCryptoHandler.java 76.92% <100.00%> (+14.42%) ⬆️
...pl/handlers/staking/EndOfStakingPeriodUpdater.java 92.23% <100.00%> (+0.15%) ⬆️
...pl/handlers/staking/StakingRewardsHandlerImpl.java 88.10% <100.00%> (+0.12%) ⬆️
...sus/impl/handlers/ConsensusUpdateTopicHandler.java 51.16% <0.00%> (+1.16%) ⬆️
...ice/token/impl/handlers/FinalizeRecordHandler.java 86.91% <85.71%> (+0.12%) ⬆️
...ervice/token/impl/validators/StakingValidator.java 68.57% <50.00%> (ø)
... and 1 more

... and 16 files with indirect coverage changes

Impacted file tree graph

@vtronkov vtronkov changed the title Non zero shard realm feat: Validate non-standard hedera.shard/hedera.realm configurations Jan 22, 2025
@vtronkov vtronkov added this to the v0.60 milestone Jan 22, 2025
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: Valentin Tronkov <[email protected]>
# Conflicts:
#	hedera-node/hedera-token-service-impl/src/main/java/com/hedera/node/app/service/token/impl/handlers/CryptoCreateHandler.java
#	hedera-node/hedera-token-service-impl/src/test/java/com/hedera/node/app/service/token/impl/test/handlers/CryptoGetAccountBalanceHandlerTest.java
#	hedera-node/hedera-token-service/src/main/java/com/hedera/node/app/service/token/AliasUtils.java
#	hedera-node/test-clients/src/main/java/com/hedera/services/bdd/spec/HapiPropertySource.java
#	hedera-node/test-clients/src/main/java/com/hedera/services/bdd/spec/utilops/UtilVerbs.java
#	hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/contract/leaky/LeakyContractTestsSuite.java
Signed-off-by: Valentin Tronkov <[email protected]>
@vtronkov vtronkov marked this pull request as ready for review February 5, 2025 10:38
@vtronkov vtronkov requested review from a team and tinker-michaelj as code owners February 5, 2025 10:38
Copy link
Contributor

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

2 very quick comments on first look at smart contracts impl. Better look to come soon. But there's a lot of potential changes in the smart contracts due to the behavior of the "long-zero" addresses (which are now going to be even more badly named than they were before - we should really be using the preferred term "account num alias" throughout our documentation and code ...) when treated as EVM addresses.

Copy link
Contributor

@david-bakin-sl david-bakin-sl left a comment

Choose a reason for hiding this comment

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

On a second look: This important change really affects smart contracts, what with "long zero" addresses being used as EVM addresses, and our frequent need to distinguish "long zero" addresses from arbitrary EVM addresses. Yet the testing in this PR for smart contracts does not, in either unit tests or hapispec tests, test any non-zero shard/realm, or any other implications of that.

And that's totally understandable because the current tests assume shard.realm == 0.0 and to fix up even the current hapispec tests to test non-zero shard/realm is a lot of work. (Or to change the hapispec framework to do it automatically for contract tests ... or whatever the solution is.). And it's unreasonable to expect the author of this PR to do all that testing for smart contracts.

The smart contracts team has an issue on the board - #17711 - for the next sprint to look into this and other work needed to support non-zero shard/realm. I strongly suggest we wait for this analysis - which will undoubtedly include some kind of cooperative effort itself with the PR author (at least) - before this PR gets merged.

# Conflicts:
#	hedera-node/hedera-smart-contract-service-impl/src/main/java/com/hedera/node/app/service/contract/impl/exec/systemcontracts/has/isauthorizedraw/IsAuthorizedRawCall.java
Signed-off-by: Valentin Tronkov <[email protected]>
@vtronkov
Copy link
Contributor Author

vtronkov commented Feb 7, 2025

On a second look: This important change really affects smart contracts, what with "long zero" addresses being used as EVM addresses, and our frequent need to distinguish "long zero" addresses from arbitrary EVM addresses. Yet the testing in this PR for smart contracts does not, in either unit tests or hapispec tests, test any non-zero shard/realm, or any other implications of that.

And that's totally understandable because the current tests assume shard.realm == 0.0 and to fix up even the current hapispec tests to test non-zero shard/realm is a lot of work. (Or to change the hapispec framework to do it automatically for contract tests ... or whatever the solution is.). And it's unreasonable to expect the author of this PR to do all that testing for smart contracts.

The smart contracts team has an issue on the board - #17711 - for the next sprint to look into this and other work needed to support non-zero shard/realm. I strongly suggest we wait for this analysis - which will undoubtedly include some kind of cooperative effort itself with the PR author (at least) - before this PR gets merged.

I reverted all smart-contract changes 👍

Copy link
Contributor

@david-bakin-sl david-bakin-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 - while knowing that the changes to smart contracts will happen later - see analysis task #17711 which is the start of that.

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.

Since there will be more PRs for these changes, we can make any review comment updates in next PRs. LGTM! Thanks @vtronkov

@vtronkov vtronkov merged commit a1a5509 into main Feb 10, 2025
48 of 49 checks passed
@vtronkov vtronkov deleted the non-zero-shard-realm branch February 10, 2025 10:06
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.

Change shard/realm to services business code to use the configuration
6 participants