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: adapt StatsSigningTestingTool to work with Bytes wrapper for transactions #17144

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
63edb17
refactor: consumer for system transactions
mustafauzunn Dec 19, 2024
50669f7
refactor: adapt StatsSigningTestingTool to work with Bytes wrapper fo…
IvanKavaldzhiev Dec 20, 2024
fd7222c
Merge remote-tracking branch 'origin/develop' into 16871-adapt-StatsS…
IvanKavaldzhiev Dec 20, 2024
a7d6e22
style: restore headers
IvanKavaldzhiev Dec 20, 2024
b4dcec7
Merge remote-tracking branch 'origin/16703-refactor-consumer-system-t…
IvanKavaldzhiev Dec 20, 2024
8478fd9
Merge branch 'develop' into 16703-refactor-consumer-system-transactions
mustafauzunn Dec 20, 2024
2c6d151
Merge remote-tracking branch 'origin/16703-refactor-consumer-system-t…
IvanKavaldzhiev Dec 20, 2024
b67ca58
Merge remote-tracking branch 'origin/develop' into 16871-adapt-StatsS…
IvanKavaldzhiev Dec 20, 2024
2145caa
nit: resolve PR comments
IvanKavaldzhiev Dec 20, 2024
9315b1c
nit: resolve PR comments
IvanKavaldzhiev Dec 20, 2024
c26e80d
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Dec 26, 2024
a3d2c8b
nit: resolve PR comment and change system transaction differentiation
IvanKavaldzhiev Dec 27, 2024
7520540
nit: fix build
IvanKavaldzhiev Dec 27, 2024
c82d849
nit: resolve PR comment
IvanKavaldzhiev Dec 27, 2024
dcb186e
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Dec 27, 2024
3d56d76
Merge branch 'main' into 16871-adapt-StatsSigningTestingTool-to-work-…
rbarkerSL Jan 2, 2025
4e9ef8b
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 6, 2025
0cc6405
Merge remote-tracking branch 'origin/16871-adapt-StatsSigningTestingT…
IvanKavaldzhiev Jan 6, 2025
f3fd9b8
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 7, 2025
09bba4d
refactor: consume converted system transaction in callback instead of…
IvanKavaldzhiev Jan 7, 2025
734ed9a
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 7, 2025
90f12bc
style: spotless apply and remove unnecessary check
IvanKavaldzhiev Jan 7, 2025
c91aa49
style: spotless apply
IvanKavaldzhiev Jan 7, 2025
9877ca8
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 8, 2025
e8e5eae
nit: add final declarations and comments for elaborating business logic
IvanKavaldzhiev Jan 8, 2025
24c549a
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 9, 2025
8a809cf
refactor: simplify logic for distinguishing between system and applic…
IvanKavaldzhiev Jan 10, 2025
fd85b62
Merge remote-tracking branch 'origin/main' into 16871-adapt-StatsSign…
IvanKavaldzhiev Jan 10, 2025
2ab240e
nit: resolve PR comment
IvanKavaldzhiev Jan 10, 2025
289fe7c
nit: fix PR comment
IvanKavaldzhiev Jan 10, 2025
32c9803
nit: resolve PR comment
IvanKavaldzhiev Jan 10, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ private void applyTransactionToState(final @NonNull ConsensusTransaction transac
@Override
public void preHandle(
@NonNull final Event event,
@NonNull final Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransaction) {
@NonNull final Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransactions) {
mustafauzunn marked this conversation as resolved.
Show resolved Hide resolved
event.forEachTransaction(transaction -> {
if (transaction.isSystem()) {
return;
Expand All @@ -269,7 +269,7 @@ public void preHandle(
public void handleConsensusRound(
final @NonNull Round round,
final @NonNull PlatformStateModifier platformState,
@NonNull final Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransaction) {
@NonNull final Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransactions) {
mustafauzunn marked this conversation as resolved.
Show resolved Hide resolved
Objects.requireNonNull(round);
Objects.requireNonNull(platformState);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,30 @@
// SPDX-License-Identifier: Apache-2.0
/*
* Copyright (C) 2024 Hedera Hashgraph, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

plugins { id("org.hiero.gradle.module.application") }

// Remove the following line to enable all 'javac' lint checks that we have turned on by default
// and then fix the reported issues.
tasks.withType<JavaCompile>().configureEach { options.compilerArgs.add("-Xlint:-cast") }

application.mainClass = "com.swirlds.demo.stats.signing.StatsSigningTestingToolMain"

testModuleInfo {
requires("org.assertj.core")
requires("org.junit.jupiter.api")
requires("org.mockito")
requires("org.junit.jupiter.params")
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2022-2024 Hedera Hashgraph, LLC
* Copyright (C) 2024 Hedera Hashgraph, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -34,6 +34,8 @@
import static com.swirlds.platform.test.fixtures.state.FakeMerkleStateLifecycles.FAKE_MERKLE_STATE_LIFECYCLES;
import static com.swirlds.platform.test.fixtures.state.FakeMerkleStateLifecycles.registerMerkleStateRootClassIds;

import com.hedera.hapi.platform.event.StateSignatureTransaction;
import com.hedera.pbj.runtime.io.buffer.Bytes;
import com.swirlds.common.constructable.ClassConstructorPair;
import com.swirlds.common.constructable.ConstructableRegistry;
import com.swirlds.common.constructable.ConstructableRegistryException;
Expand Down Expand Up @@ -313,4 +315,9 @@
public BasicSoftwareVersion getSoftwareVersion() {
return softwareVersion;
}

@Override
public Bytes encodeSystemTransaction(@NonNull final StateSignatureTransaction transaction) {
return StateSignatureTransaction.PROTOBUF.toBytes(transaction);

Check warning on line 321 in platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolMain.java

View check run for this annotation

Codecov / codecov/patch

platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/StatsSigningTestingToolMain.java#L321

Added line #L321 was not covered by tests
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2022-2024 Hedera Hashgraph, LLC
* Copyright (C) 2024 Hedera Hashgraph, LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -46,6 +46,7 @@
import com.swirlds.platform.system.transaction.ConsensusTransaction;
import com.swirlds.platform.system.transaction.Transaction;
import edu.umd.cs.findbugs.annotations.NonNull;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -122,6 +123,13 @@ public void preHandle(
if (transaction.isSystem()) {
return;
}

if (areTransactionBytesSystemOnes(transaction)) {
stateSignatureTransaction.accept(
new ScopedSystemTransaction(event.getCreatorId(), event.getSoftwareVersion(), transaction));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Fixed

return;
}

final TransactionSignature transactionSignature =
sttTransactionPool.expandSignatures(transaction.getApplicationTransaction());
if (transactionSignature != null) {
Expand All @@ -141,13 +149,22 @@ public void handleConsensusRound(
@NonNull final PlatformStateModifier platformState,
@NonNull final Consumer<ScopedSystemTransaction<StateSignatureTransaction>> stateSignatureTransaction) {
throwIfImmutable();
round.forEachTransaction(this::handleTransaction);

round.forEachEventTransaction((event, transaction) -> {
if (areTransactionBytesSystemOnes(transaction)) {
stateSignatureTransaction.accept(
new ScopedSystemTransaction(event.getCreatorId(), event.getSoftwareVersion(), transaction));
} else {
handleTransaction(transaction);
}
});
}

private void handleTransaction(final ConsensusTransaction trans) {
if (trans.isSystem()) {
return;
}

final TransactionSignature s = trans.getMetadata();

if (s != null && validateSignature(s, trans) && s.getSignatureStatus() != VerificationStatus.VALID) {
Expand Down Expand Up @@ -175,6 +192,36 @@ private void handleTransaction(final ConsensusTransaction trans) {
maybeDelay();
}

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

Choose a reason for hiding this comment

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

add nullity annotation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

final var transactionBytes = transaction.getApplicationTransaction();

if (transactionBytes.length() == 0) {
return false;
}

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();
}
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a simpler approach for filtering system transactions.

private void maybeDelay() {
if (SYNTHETIC_HANDLE_TIME) {
final long start = System.nanoTime();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,4 +212,8 @@
}
}
}

public int getTransactionSize() {
return transactionSize;

Check warning on line 217 in platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/SttTransactionPool.java

View check run for this annotation

Codecov / codecov/patch

platform-sdk/platform-apps/tests/StatsSigningTestingTool/src/main/java/com/swirlds/demo/stats/signing/SttTransactionPool.java#L217

Added line #L217 was not covered by tests
}
}
Loading