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

refactor: system transactions #17406

Draft
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

mustafauzunn
Copy link
Collaborator

Description:

The final PR to support the new format of GossipEvent where transactions will be passed as repeated bytes

Related issue(s):

Fixes #16665

Notes for reviewer:

Checklist

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

Signed-off-by: Mustafa Uzun <[email protected]>
…5-system-transaction-refactoring

# Conflicts:
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/eventhandling/DefaultTransactionPrehandler.java
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/state/SwirldStateManager.java
#	platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/state/TransactionHandler.java
Signed-off-by: Mustafa Uzun <[email protected]>
@mustafauzunn mustafauzunn added the Platform Tickets pertaining to the platform label Jan 16, 2025
@mustafauzunn mustafauzunn added this to the v0.59 milestone Jan 16, 2025
@mustafauzunn mustafauzunn self-assigned this Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 70.88608% with 23 lines in your changes missing coverage. Please review.

Project coverage is 68.93%. Comparing base (829e18c) to head (446625a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...om/swirlds/platform/pool/TransactionPoolNexus.java 22.22% 7 Missing ⚠️
.../com/swirlds/platform/builder/PlatformBuilder.java 0.00% 4 Missing ⚠️
.../swirlds/platform/system/events/UnsignedEvent.java 50.00% 3 Missing ⚠️
...rc/main/java/com/hedera/node/app/ServicesMain.java 0.00% 2 Missing ⚠️
.../java/com/swirlds/platform/cli/DiagramCommand.java 0.00% 2 Missing ⚠️
...irlds/platform/system/transaction/Transaction.java 33.33% 1 Missing and 1 partial ⚠️
.../demo/consistency/ConsistencyTestingToolRound.java 0.00% 1 Missing ⚠️
...re/src/main/java/com/swirlds/platform/Browser.java 0.00% 1 Missing ⚠️
...rm/eventhandling/DefaultTransactionPrehandler.java 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #17406      +/-   ##
============================================
- Coverage     68.96%   68.93%   -0.03%     
+ Complexity    22763    22758       -5     
============================================
  Files          2619     2619              
  Lines         98283    98260      -23     
  Branches      10185    10183       -2     
============================================
- Hits          67777    67732      -45     
- Misses        26675    26694      +19     
- Partials       3831     3834       +3     
Files with missing lines Coverage Δ
...ows/standalone/impl/StandaloneDispatchFactory.java 98.41% <100.00%> (-0.05%) ⬇️
...swirlds/platform/builder/ApplicationCallbacks.java 100.00% <ø> (ø)
...m/components/consensus/DefaultConsensusEngine.java 87.50% <ø> (ø)
...ction/system/SystemTransactionExtractionUtils.java 93.75% <ø> (-6.25%) ⬇️
...java/com/swirlds/platform/event/PlatformEvent.java 87.61% <100.00%> (-1.77%) ⬇️
...form/event/creation/tipset/TipsetEventCreator.java 81.87% <ø> (ø)
...wirlds/platform/event/hashing/PbjStreamHasher.java 93.10% <100.00%> (+1.79%) ⬆️
...ent/resubmitter/DefaultTransactionResubmitter.java 100.00% <ø> (ø)
...vent/validation/DefaultInternalEventValidator.java 81.35% <ø> (-9.04%) ⬇️
...om/swirlds/platform/metrics/AddedEventMetrics.java 100.00% <100.00%> (+4.59%) ⬆️
... and 17 more

... and 10 files with indirect coverage changes

Impacted file tree graph

Copy link

codacy-production bot commented Jan 16, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.01% (target: -1.00%) 77.66%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (3e8fa1f) 97675 71128 72.82%
Head commit (50733eb) 97689 (+14) 71124 (-4) 72.81% (-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 (#17406) 94 73 77.66%

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

Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Copy link

github-actions bot commented Jan 20, 2025

Node: HAPI Test (Token) Results

 28 files   4 errors  24 suites   8s ⏱️
136 tests  0 ✅ 0 💤 136 ❌
226 runs   0 ✅ 0 💤 226 ❌

For more details on these parsing errors and failures, see this check.

Results for commit 1acb251.

♻️ This comment has been updated with latest results.

mustafauzunn and others added 4 commits January 20, 2025 12:42
Signed-off-by: Mustafa Uzun <[email protected]>
…ring' into 16665-system-transaction-refactoring
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
Copy link

github-actions bot commented Jan 20, 2025

Node: HAPI Test (Smart Contract) Results

  117 files      2 errors  115 suites   36m 3s ⏱️
  169 tests   169 ✅ 0 💤 0 ❌
1 072 runs  1 072 ✅ 0 💤 0 ❌

For more details on these parsing errors, see this check.

Results for commit b64fc96.

♻️ This comment has been updated with latest results.

Comment on lines 50 to 53
transactionPoolNexus.submitTransaction(
new EventTransaction(new OneOf<>(TransactionOneOfType.STATE_SIGNATURE_TRANSACTION, payload)), true);
final Bytes payloadBytes = stateLifecycles.encodeSystemTransaction(payload);
transactionPoolNexus.submitTransaction(payloadBytes, true, true);
Copy link
Member

Choose a reason for hiding this comment

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

this transformation should be done by a wiring transformer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

Comment on lines 172 to 173
public synchronized boolean submitTransaction(
@NonNull final Bytes transaction, final boolean priority, final boolean isSystem) {
Copy link
Member

Choose a reason for hiding this comment

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

priority & isSystem are always equal, can be simplified

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored

Comment on lines -154 to +149
if (TransactionUtils.getLegacyTransactionSize(transaction) > maximumTransactionSize) {
if (TransactionUtils.getLegacyTransactionSize(appTransaction) > maximumTransactionSize) {
Copy link
Member

Choose a reason for hiding this comment

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

@lpetrovic05 create a ticket for this cleanup

Comment on lines 111 to 113
default Bytes encodeSystemTransaction(@NonNull final StateSignatureTransaction transaction) {
throw new IllegalStateException("Invoke the method on the appropriate SwirldMain implementation!");
}
Copy link
Member

Choose a reason for hiding this comment

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

should be removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

Comment on lines +42 to +43
Bytes getTransactionBytes();

Copy link
Member

Choose a reason for hiding this comment

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

there should be a single method call to get the transaction

@@ -39,14 +39,18 @@ public sealed interface Transaction permits ConsensusTransaction {
@Deprecated
EventTransaction getTransaction();
Copy link
Member

Choose a reason for hiding this comment

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

seems we dont need this method anymore

Comment on lines 63 to 64
int getBytesSize();

Copy link
Member

Choose a reason for hiding this comment

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

there should be a single size method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored

/**
* Check if a transaction is a system transaction.<br>
* This is a convenience method that delegates to {@link #isSystemTransaction(OneOf)}.
*
* @param transaction the transaction to check
* @return {@code true} if the transaction is a system transaction, {@code false} otherwise
*/
public static boolean isSystemTransaction(@NonNull final EventTransaction transaction) {
return isSystemTransaction(transaction.transaction());
// TODO: Refactor this method to work properly
Copy link
Member

Choose a reason for hiding this comment

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

todo

* <p>
* If set to true, transactions will be encoded in the new format of Bytes. If set to false, the deprecated OneOf format will be used.
*/
private boolean useNewTransactionFormat = false;
Copy link
Member

Choose a reason for hiding this comment

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

this should be reverted

Copy link

Node: HAPI Test (Misc) Results

 74 files    1 errors  73 suites   15m 57s ⏱️
106 tests  85 ✅ 0 💤  21 ❌
463 runs  337 ✅ 0 💤 126 ❌

For more details on these parsing errors and failures, see this check.

Results for commit 5a0b661.

mustafauzunn and others added 4 commits January 22, 2025 15:50
Signed-off-by: Mustafa Uzun <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
…ring' into 16665-system-transaction-refactoring
Copy link

Node: HAPI Test (Crypto) Results

 30 files   2 errors  28 suites   10s ⏱️
 28 tests  0 ✅ 0 💤  28 ❌
344 runs   0 ✅ 0 💤 344 ❌

For more details on these parsing errors and failures, see this check.

Results for commit cb7022e.

IvanKavaldzhiev and others added 4 commits January 22, 2025 17:09
Signed-off-by: Ivan Kavaldzhiev <[email protected]>
Signed-off-by: Mustafa Uzun <[email protected]>
…5-system-transaction-refactoring

# Conflicts:
#	hedera-node/hedera-app/src/main/java/com/hedera/node/app/Hedera.java
Signed-off-by: Mustafa Uzun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform Tickets pertaining to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System transactions refactoring
3 participants