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

Enable HollowAccountFinalizationSuite #9535

Merged
merged 47 commits into from Nov 6, 2023

Conversation

Neeharika-Sompalli
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli commented Oct 26, 2023

Fixes #9362

  • Enabled all tests except 2 tests in HollowAccountFinalizationSuite

Child Records Issues:

  • Fixes the way preceding child records are recognized. Since they don't have parentConsensusTimestamp set, we need to be able to add to the history of current user transaction based on the nonce > 0
  • Sorted the child records to be stored as per increasing value of nonce, so they are returned same way for getTxnRecord query.
  • Since in mono-service preceding child transaction are interpreted only by the way records are arranged, the check to see if state has changed causes all preceding transactions to fail. So commented them out ( Open for discussion on this change)

Hollow Account Finalization:

  • When any account is required to sign, check if the account is hollow and add them to PreHandlContext.requiredHollowAccounts which will be available in preHandleResult. Same for the payer
  • In the Handle Workflow finalize all required hollow accounts that were required to sign for the transaction. Gets the key corresponding to the alias from key verifier and dispatches a synthetic preceding CryptoUpdate to update the key.
  • In CryptoTransfer logic when there is a debit and if the debited account is hollow, it is possible that the account has signed the transaction. If so add it to the requiredHollowAccounts and finalize in HandleWorkflow

Misc:

  • Found that we fail everytime we run any transaction on genesis start with MAX_CHILD_RECORDS_EXCEEDED because we are adding ~700 child records for system account creation. Added an enum PrecedingTransactionCategory to add exception for the case. This will be an exception for genesis records and staking update synthetic transaction that happen every midnight.
  • Parsing an evmAlias using Key.PROTOBUF.parse() causing BufferUnderFlowException. Fixed it by catching it and also not parsing evmAliases

NOTE:
Two tests are not enabled hollowAccountCompletionIsPersistedEvenIfTxnFails, txnWith2CompletionsAndAnother2PrecedingChildRecords because in mono-service the hollow account finalizations are committed even if the transaction fails. Do we want to retain that in mod-code ?

iwsimon and others added 22 commits October 23, 2023 17:04
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
Signed-off-by: Iris Simon <122310714+iwsimon@users.noreply.github.com>
Signed-off-by: Iris Simon <122310714+iwsimon@users.noreply.github.com>
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
…ite' into 9362-HollowAccountFinalizationSuite
Signed-off-by: Iris Simon <iris.simon@swirldslabs.com>
Signed-off-by: Iris Simon <122310714+iwsimon@users.noreply.github.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
…-account-finalization

Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Node: E2E Test Results

    1 files      1 suites   21m 38s ⏱️
310 tests 310 ✔️ 0 💤 0
332 runs  332 ✔️ 0 💤 0

Results for commit 6c84f20.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Node: Unit Test Results

    2 271 files      2 271 suites   1h 25m 29s ⏱️
118 338 tests 118 304 ✔️ 34 💤 0
126 599 runs  126 565 ✔️ 34 💤 0

Results for commit 6c84f20.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (824d007) 0.00% compared to head (8a1e878) 64.73%.

❗ Current head 8a1e878 differs from pull request most recent head 6c84f20. Consider uploading reports for the commit 6c84f20 to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##             develop    #9535       +/-   ##
==============================================
+ Coverage           0   64.73%   +64.73%     
- Complexity         0    29552    +29552     
==============================================
  Files              0     3261     +3261     
  Lines              0   124682   +124682     
  Branches           0    12935    +12935     
==============================================
+ Hits               0    80716    +80716     
- Misses             0    40925    +40925     
- Partials           0     3041     +3041     
Files Coverage Δ
...m/hedera/node/app/spi/workflows/HandleContext.java 63.15% <ø> (ø)
...ra/node/app/state/recordcache/RecordCacheImpl.java 77.77% <100.00%> (ø)
...ows/handle/record/GenesisRecordsConsensusHook.java 100.00% <100.00%> (ø)
.../node/app/workflows/prehandle/PreHandleResult.java 100.00% <ø> (ø)
...app/workflows/prehandle/PreHandleWorkflowImpl.java 96.82% <100.00%> (ø)
...a/com/hedera/node/config/data/ConsensusConfig.java 100.00% <ø> (ø)
...no/txns/crypto/HollowAccountFinalizationLogic.java 100.00% <ø> (ø)
...l/handlers/NetworkTransactionGetRecordHandler.java 58.46% <100.00%> (ø)
...ervice/token/impl/handlers/TokenCreateHandler.java 84.43% <100.00%> (ø)
...pl/handlers/staking/EndOfStakingPeriodUpdater.java 91.17% <100.00%> (ø)
... and 10 more

... and 3241 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Node: Integration Test Results

280 tests   280 ✔️  32m 43s ⏱️
    5 suites      0 💤
    5 files        0

Results for commit 6c84f20.

♻️ This comment has been updated with latest results.

@Neeharika-Sompalli Neeharika-Sompalli self-assigned this Oct 27, 2023
@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Node: HAPI Test Results

1 242 tests   814 ✔️  1h 24m 20s ⏱️
   165 suites  428 💤
   165 files        0

Results for commit 6c84f20.

♻️ This comment has been updated with latest results.

Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
iwsimon
iwsimon previously approved these changes Nov 3, 2023
Copy link
Contributor

@iwsimon iwsimon left a comment

Choose a reason for hiding this comment

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

LGTM

iwsimon
iwsimon previously approved these changes Nov 6, 2023
Copy link
Contributor

@iwsimon iwsimon 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
Collaborator

@tinker-michaelj tinker-michaelj 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

@iwsimon iwsimon left a comment

Choose a reason for hiding this comment

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

LGTM

@Neeharika-Sompalli Neeharika-Sompalli merged commit 20bdd4a into develop Nov 6, 2023
10 of 11 checks passed
@Neeharika-Sompalli Neeharika-Sompalli deleted the 9362-D-hollow-account-finalization branch November 6, 2023 16:05
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.

Start testing HollowAccountFinalizationSuite
5 participants