refactor: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

IvanKavaldzhiev · 2024-12-20T11:30:04Z

Description:
This PR updates StatsSigningTestingTool to work with the new Bytes format of system transactions.

Related issue(s):

Fixes #16871

Notes for reviewer:

Checklist

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

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

…r transactions Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…igningTestingTool-to-work-with-Bytes

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ransactions' into 16871-adapt-StatsSigningTestingTool-to-work-with-Bytes # Conflicts: # platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ransactions' into 16871-adapt-StatsSigningTestingTool-to-work-with-Bytes

codecov · 2024-12-20T11:48:25Z

Codecov Report

Attention: Patch coverage is 68.88889% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.60%. Comparing base (be1981d) to head (32c9803).

Files with missing lines	Patch %	Lines
...emo/stats/signing/StatsSigningTestingToolMain.java	53.33%	7 Missing ⚠️
...mo/stats/signing/StatsSigningTestingToolState.java	83.33%	3 Missing and 1 partial ⚠️
...swirlds/demo/stats/signing/SttTransactionPool.java	0.00%	2 Missing ⚠️
...m/swirlds/demo/stats/signing/TransactionCodec.java	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #17144      +/-   ##
============================================
+ Coverage     67.41%   67.60%   +0.18%     
- Complexity    22063    22125      +62     
============================================
  Files          2585     2585              
  Lines         96408    96440      +32     
  Branches      10071    10075       +4     
============================================
+ Hits          64992    65195     +203     
+ Misses        27698    27511     -187     
- Partials       3718     3734      +16

Files with missing lines	Coverage Δ
...m/swirlds/demo/stats/signing/TransactionCodec.java	`35.36% <75.00%> (+35.36%)`	⬆️
...swirlds/demo/stats/signing/SttTransactionPool.java	`1.53% <0.00%> (+1.53%)`	⬆️
...mo/stats/signing/StatsSigningTestingToolState.java	`47.12% <83.33%> (+47.12%)`	⬆️
...emo/stats/signing/StatsSigningTestingToolMain.java	`30.76% <53.33%> (+30.76%)`	⬆️

... and 6 files with indirect coverage changes

codacy-production · 2024-12-20T11:48:39Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.20% (target: -1.00%)	✅ 73.33%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`be1981d`)	96191	68558	71.27%
Head commit (`32c9803`)	96223 (+32)	68777 (+219)	71.48% (+0.20%)

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 (#17144)	45	33	73.33%

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}

poulok · 2024-12-20T13:41:57Z

...ingTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolMain.java

@@ -313,4 +315,9 @@ public PlatformMerkleStateRoot newMerkleStateRoot() {
    public BasicSoftwareVersion getSoftwareVersion() {
        return softwareVersion;
    }
+
+    @Override
+    public Bytes encodeSystemTransaction(@NonNull StateSignatureTransaction transaction) {


add final

poulok · 2024-12-20T13:48:13Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

+
+                if (areTransactionBytesSystemOnes((ConsensusTransaction) transaction)) {
+                    stateSignatureTransactions.accept(
+                            new ScopedSystemTransaction(event.getCreatorId(), event.getSoftwareVersion(), transaction));


You can return after this, since the application should not do anything with the system transactions other than invoke the callback.

Good idea. Fixed

poulok · 2024-12-20T13:48:52Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

-        round.forEachTransaction(this::handleTransaction);
+
+        round.forEachEventTransaction((event, transaction) -> {
+            final var transactionWithSystemBytes = handleTransaction(transaction);


Why not check if it is a system transaction here in the lambda before invoking handleTransaction?

…igningTestingTool-to-work-with-Bytes # Conflicts: # platform-sdk/platform-apps/demos/CryptocurrencyDemo/src/main/java/com/swirlds/demo/crypto/CryptocurrencyDemoState.java # platform-sdk/platform-apps/demos/HelloSwirldDemo/src/main/java/com/swirlds/demo/hello/HelloSwirldDemoState.java # platform-sdk/platform-apps/demos/StatsDemo/src/main/java/com/swirlds/demo/stats/StatsDemoState.java # platform-sdk/platform-apps/tests/AddressBookTestingTool/src/main/java/com/swirlds/demo/addressbook/AddressBookTestingToolState.java # platform-sdk/platform-apps/tests/ConsistencyTestingTool/src/main/java/com/swirlds/demo/consistency/ConsistencyTestingToolState.java # platform-sdk/platform-apps/tests/ISSTestingTool/src/main/java/com/swirlds/demo/iss/ISSTestingToolState.java # platform-sdk/platform-apps/tests/MigrationTestingTool/src/main/java/com/swirlds/demo/migration/MigrationTestingToolState.java # platform-sdk/platform-apps/tests/PlatformTestingTool/src/main/java/com/swirlds/demo/platform/PlatformTestingToolState.java # platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java # platform-sdk/platform-apps/tests/StressTestingTool/src/main/java/com/swirlds/demo/stress/StressTestingToolState.java # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/state/PlatformMerkleStateRoot.java # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/system/SwirldState.java # platform-sdk/swirlds-platform-core/src/test/java/com/swirlds/platform/turtle/runner/TurtleTestingToolState.java # platform-sdk/swirlds-platform-core/src/testFixtures/java/com/swirlds/platform/test/fixtures/state/BlockingSwirldState.java Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

lpetrovic05 · 2024-12-23T11:22:51Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

+    private boolean areTransactionBytesSystemOnes(final ConsensusTransaction transaction) {
+        // We have maximum allocation of 100 bytes for the transaction + 10 bytes for the preamble size in
+        // TransactionCodec
+        // + 64 bytes for the maximum public key size + 64 bytes for the maximum signature size + 12 bytes for 3
+        // integers
+        final var maximumSignedEncodedTransactionSize = 100 + 10 + 64 + 64 + 12;
+        return transaction.getApplicationTransaction().length() > maximumSignedEncodedTransactionSize;
+    }
+


this app has a configurable transaction size, so size cannot be used to differentiate between system and app

Agreed. This approach wasn't future proof. I changed the logic for differentiation. It can be re-reviewd.

andrewb1269hg

Reviewed file platform-sdk/platform-apps/tests/StatsSigningTestingTool/build.gradle.kts and approved.

…ingTestingTool-to-work-with-Bytes

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

lpetrovic05 · 2024-12-27T10:31:24Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

+        final var transactionPool = transactionPoolSupplier.get();
+
+        // Check the data length size is the expected size based on the transactionSize property.
+        // If it is not, then the transaction is a system one.
+        if (TransactionCodec.txIsSigned(transactionBytes)) {
+            // Data length can be read directly from the extracted signature
+            final var signature = TransactionCodec.extractSignature(transactionBytes);
+            return signature.getMessageLength() != transactionPool.getTransactionSize();
+        } else {
+            // Data length value can be read from the transaction bytes using an offset
+            final ByteBuffer wrapper =
+                    ByteBuffer.wrap(transactionBytes.toByteArray()).position(21);
+            final byte dataLength = wrapper.get();
+            return dataLength != transactionPool.getTransactionSize();
+        }
+    }
+


this does not work if they happen to be the same size, so we need a definitive way of differentiating them

Added additional logic that follows the transaction bytes structure from TransactionCodec. Now the algorithm checks if we have the correct size for the different segments on the correct positions and whether we have any bytes left after we load the data segment (which is the last one), using the data length value before it. I believe it will be very unlikely to have a system transaction that will cover all the additional checks I've added.

Another approach, which will be more harming for the performance, is trying to parse the transactions bytes to a StateSignatureTransaction pbj type for each incoming transaction.

while this makes it very unlikely to confuse the two, it is still theoretically possible. so why not make it simpler and be 100% sure?

I added a simpler approach for filtering system transactions.

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ingTestingTool-to-work-with-Bytes

…with-Bytes

rbarkerSL

Review applies only to platform-sdk/platform-apps/tests/StatsSigningTestingTool/build.gradle.kts. Approved.

…ingTestingTool-to-work-with-Bytes

…ool-to-work-with-Bytes' into 16871-adapt-StatsSigningTestingTool-to-work-with-Bytes

…ingTestingTool-to-work-with-Bytes # Conflicts: # platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolMain.java # platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java Signed-off-by: Ivan Kavaldzhiev <[email protected]>

… the transaction itself Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ingTestingTool-to-work-with-Bytes

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ingTestingTool-to-work-with-Bytes

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

poulok · 2025-01-09T13:37:52Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

+     * @param transaction the transaction to check
+     * @return true if the transaction bytes are system ones, false otherwise
+     */
+    private boolean areTransactionBytesSystemOnes(final Transaction transaction) {


add nullity annotation

poulok · 2025-01-09T13:38:20Z

...ngTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolState.java

+    }
+
+    private void consumeSystemTransaction(
+            final Transaction transaction,


nullity for all parameters

…ingTestingTool-to-work-with-Bytes

...tencyTestingTool/src/main/java/com/swirlds/demo/consistency/ConsistencyTestingToolState.java

…ation transactions Signed-off-by: Ivan Kavaldzhiev <[email protected]>

…ingTestingTool-to-work-with-Bytes # Conflicts: # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/system/SwirldMain.java Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

lpetrovic05 · 2025-01-10T13:01:43Z

...ingTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolMain.java

+    public Bytes encodeSystemTransaction(@NonNull final StateSignatureTransaction transaction) {
+        final String stateSignatureMarker = STATE_SIGNATURE_MARKER;
+        final int markerSize = stateSignatureMarker.length();
+        final byte[] parsedStateSignatureTransaction =
+                StateSignatureTransaction.PROTOBUF.toBytes(transaction).toByteArray();
+        final int bufferCapacity = Integer.BYTES + markerSize + parsedStateSignatureTransaction.length;
+
+        final ByteBuffer buffer = ByteBuffer.allocate(bufferCapacity);
+        buffer.putInt(markerSize);
+        buffer.put(stateSignatureMarker.getBytes());
+        buffer.put(parsedStateSignatureTransaction);
+
+        return Bytes.wrap(buffer.array());
+    }


why put a string as a marker? a byte would be enough.
also, you can use var out = new WritableStreamingData(new ByteArrayOutputStream)) to write the marker first and then the PBJ record with StateSignatureTransaction.PROTOBUF.write(transaction, out)

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

mustafauzunn and others added 7 commits December 19, 2024 20:52

refactor: consumer for system transactions

63edb17

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

refactor: adapt StatsSigningTestingTool to work with Bytes wrapper fo…

50669f7

…r transactions Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge remote-tracking branch 'origin/develop' into 16871-adapt-StatsS…

fd7222c

…igningTestingTool-to-work-with-Bytes

style: restore headers

a7d6e22

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge branch 'develop' into 16703-refactor-consumer-system-transactions

8478fd9

Merge remote-tracking branch 'origin/16703-refactor-consumer-system-t…

2c6d151

…ransactions' into 16871-adapt-StatsSigningTestingTool-to-work-with-Bytes

IvanKavaldzhiev added the Platform Tickets pertaining to the platform label Dec 20, 2024

IvanKavaldzhiev added this to the v0.58 milestone Dec 20, 2024

IvanKavaldzhiev requested review from lpetrovic05, poulok and mustafauzunn December 20, 2024 11:30

IvanKavaldzhiev self-assigned this Dec 20, 2024

IvanKavaldzhiev requested review from a team as code owners December 20, 2024 11:30

IvanKavaldzhiev requested a review from rbarkerSL December 20, 2024 11:30

Base automatically changed from 16703-refactor-consumer-system-transactions to develop December 20, 2024 12:58

poulok reviewed Dec 20, 2024

View reviewed changes

IvanKavaldzhiev added 3 commits December 20, 2024 16:40

nit: resolve PR comments

2145caa

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

nit: resolve PR comments

9315b1c

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

lpetrovic05 reviewed Dec 23, 2024

View reviewed changes

andrewb1269hg previously approved these changes Dec 26, 2024

View reviewed changes

IvanKavaldzhiev added 2 commits December 26, 2024 18:24

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

c26e80d

…ingTestingTool-to-work-with-Bytes

nit: resolve PR comment and change system transaction differentiation

a3d2c8b

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

nit: fix build

7520540

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

lpetrovic05 reviewed Dec 27, 2024

View reviewed changes

IvanKavaldzhiev added 2 commits December 27, 2024 15:41

nit: resolve PR comment

c82d849

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

dcb186e

…ingTestingTool-to-work-with-Bytes

IvanKavaldzhiev requested a review from lpetrovic05 December 27, 2024 13:47

Merge branch 'main' into 16871-adapt-StatsSigningTestingTool-to-work-…

3d56d76

…with-Bytes

rbarkerSL previously approved these changes Jan 2, 2025

View reviewed changes

IvanKavaldzhiev added 3 commits January 6, 2025 14:24

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

4e9ef8b

…ingTestingTool-to-work-with-Bytes

Merge remote-tracking branch 'origin/16871-adapt-StatsSigningTestingT…

0cc6405

…ool-to-work-with-Bytes' into 16871-adapt-StatsSigningTestingTool-to-work-with-Bytes

IvanKavaldzhiev dismissed rbarkerSL’s stale review via f3fd9b8 January 7, 2025 09:01

IvanKavaldzhiev added 6 commits January 7, 2025 16:26

refactor: consume converted system transaction in callback instead of…

09bba4d

… the transaction itself Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

734ed9a

…ingTestingTool-to-work-with-Bytes

style: spotless apply and remove unnecessary check

90f12bc

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

style: spotless apply

c91aa49

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

9877ca8

…ingTestingTool-to-work-with-Bytes

nit: add final declarations and comments for elaborating business logic

e8e5eae

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

poulok reviewed Jan 9, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

24c549a

…ingTestingTool-to-work-with-Bytes

mustafauzunn reviewed Jan 10, 2025

View reviewed changes

...tencyTestingTool/src/main/java/com/swirlds/demo/consistency/ConsistencyTestingToolState.java Outdated Show resolved Hide resolved

...tencyTestingTool/src/main/java/com/swirlds/demo/consistency/ConsistencyTestingToolState.java Outdated Show resolved Hide resolved

IvanKavaldzhiev added 4 commits January 10, 2025 14:24

refactor: simplify logic for distinguishing between system and applic…

8a809cf

…ation transactions Signed-off-by: Ivan Kavaldzhiev <[email protected]>

Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…

fd85b62

…ingTestingTool-to-work-with-Bytes # Conflicts: # platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/system/SwirldMain.java Signed-off-by: Ivan Kavaldzhiev <[email protected]>

nit: resolve PR comment

2ab240e

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

nit: fix PR comment

289fe7c

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

IvanKavaldzhiev requested review from mustafauzunn and poulok January 10, 2025 12:34

lpetrovic05 reviewed Jan 10, 2025

View reviewed changes

nit: resolve PR comment

32c9803

Signed-off-by: Ivan Kavaldzhiev <[email protected]>

IvanKavaldzhiev requested a review from lpetrovic05 January 10, 2025 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

refactor: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

IvanKavaldzhiev commented Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

codacy-production bot commented Dec 20, 2024 •

edited

Loading

poulok Dec 20, 2024

IvanKavaldzhiev Dec 20, 2024

poulok Dec 20, 2024

IvanKavaldzhiev Dec 20, 2024

poulok Dec 20, 2024

IvanKavaldzhiev Dec 20, 2024

lpetrovic05 Dec 23, 2024

IvanKavaldzhiev Dec 27, 2024

andrewb1269hg left a comment

lpetrovic05 Dec 27, 2024

IvanKavaldzhiev Dec 27, 2024

lpetrovic05 Jan 9, 2025

IvanKavaldzhiev Jan 10, 2025

rbarkerSL left a comment

poulok Jan 9, 2025

IvanKavaldzhiev Jan 10, 2025

poulok Jan 9, 2025

IvanKavaldzhiev Jan 10, 2025

lpetrovic05 Jan 10, 2025

IvanKavaldzhiev Jan 10, 2025

refactor: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

Are you sure you want to change the base?

refactor: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

Conversation

IvanKavaldzhiev commented Dec 20, 2024

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

codacy-production bot commented Dec 20, 2024 • edited Loading

Coverage summary from Codacy

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

andrewb1269hg 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

rbarkerSL 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

codecov bot commented Dec 20, 2024 •

edited

Loading

codacy-production bot commented Dec 20, 2024 •

edited

Loading