feat: prehandle purecheck hip 551 #17530

kimbor · 2025-01-24T03:35:35Z

Description:

Related issue(s):

Fixes #17358

Notes for reviewer:

Checklist

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

…ransaction() Signed-off-by: Iris Simon <[email protected]>

Signed-off-by: Lev Povolotsky <[email protected]>

…ip-551 # Conflicts: # hapi/hedera-protobufs/services/basic_types.proto # hapi/hedera-protobufs/services/response_code.proto # hapi/hedera-protobufs/services/transaction.proto # hapi/src/main/java/com/hedera/hapi/util/HapiUtils.java # hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/HandleContext.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/handle/DispatchHandleContext.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/handle/dispatch/ChildDispatchFactory.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/handle/steps/UserTxnFactory.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/prehandle/PreHandleContextImpl.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/standalone/impl/StandaloneDispatchFactory.java # hedera-node/hedera-app/src/test/java/com/hedera/node/app/workflows/handle/DispatchHandleContextTest.java # hedera-node/hedera-app/src/test/java/com/hedera/node/app/workflows/handle/dispatch/ChildDispatchFactoryTest.java # hedera-node/hedera-app/src/test/java/com/hedera/node/app/workflows/handle/steps/UserTxnTest.java

Signed-off-by: Lev Povolotsky <[email protected]>

…e-purecheck-hip-551 # Conflicts: # hapi/hedera-protobufs/services/basic_types.proto # hapi/hedera-protobufs/services/response_code.proto # hapi/hedera-protobufs/services/transaction.proto # hapi/src/main/java/com/hedera/hapi/util/HapiUtils.java # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

…e-purecheck-hip-551 # Conflicts: # hapi/hedera-protobufs/services/transaction.proto # hapi/src/main/java/com/hedera/hapi/util/HapiUtils.java # hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/PureChecksContext.java # hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/purechecks/PureChecksContextImpl.java # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java # hedera-node/hedera-util-service/src/main/java/module-info.java

Signed-off-by: Lev Povolotsky <[email protected]>

Signed-off-by: Kim Rader <[email protected]>

Signed-off-by: Lev Povolotsky <[email protected]>

…dera-services into prehandle-purecheck-hip-551 # Conflicts: # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

Signed-off-by: Kim Rader <[email protected]>

…e-purecheck-hip-551 # Conflicts: # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java # hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/hip551/AtomicBatchTest.java # hedera-node/test-clients/src/main/java/com/hedera/services/bdd/suites/integration/ConcurrentIntegrationTests.java

Signed-off-by: Kim Rader <[email protected]>

Neeharika-Sompalli · 2025-02-19T18:02:59Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

+        Set<TransactionID> set = new HashSet<>();
+        for (final var transaction : transactions) {
+            final TransactionBody txBody;
+            try {
+                txBody = bodyParser.apply(transaction);
+            } catch (HandleException e) {
+                throw new PreCheckException(e.getStatus());
+            }
+
+            validateTruePreCheck(set.add(txBody.transactionID()), BATCH_LIST_CONTAINS_DUPLICATES);


Based on what Michael H said, we should not do these checks in pureChecks. It will fail in pre-handle the way its implemented currently

We need to parse the inner transactions in order to do later validations of duplicate txs, missing batch keys, etc. In my understanding, what Michael H meant was that these kinds of checks should not happen before fees are charged and to fix that we need to move where pureChecks is called in the ingest workflow. These checks should still happen in pureChecks but where pureChecks is called should move, which is outside the scope of this PR.

I mean checking transactionID is not required here. It will fail in preHandle for inner transactions @kimbor

Signed-off-by: Kim Rader <[email protected]>

netopyr · 2025-02-20T06:48:50Z

...a-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/PureChecksContext.java

    @NonNull
-    Configuration configuration();
+    void executeInnerPureCheck(@NonNull TransactionBody body) throws PreCheckException;


Javadocs missing.
The name of this method should be more generic IMO. Maybe dispatchPureCheck().

netopyr · 2025-02-20T06:54:27Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

-        for (final var transaction : op.transactions()) {
-            validateTruePreCheck(transaction.hasBody(), INVALID_TRANSACTION);
+        final TransactionBody txn = context.body();
+        requireNonNull(txn);


This check should not be needed.

netopyr · 2025-02-20T06:55:54Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

+        final AtomicBatchTransactionBody transactionBody = txn.atomicBatchOrThrow();
+
+        final List<Transaction> transactions = transactionBody.transactions();
+        requireNonNull(transactions);


This cannot be null.

netopyr · 2025-02-20T06:56:58Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

@@ -153,6 +192,15 @@ public void handle(@NonNull final HandleContext context) throws HandleException
        }
    }

+    private @NonNull List<TransactionBody> getTransactionBodies(@NonNull List<Transaction> transactions)


This method is not needed. Inner transactions must always use Transaction.body

netopyr · 2025-02-20T06:59:22Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

+                throw new PreCheckException(INVALID_NODE_ACCOUNT_ID);
+            }
+
+            if (txBody.hasFreeze() || txBody.hasAtomicBatch())


There should be a configurable blacklist

Implemented

netopyr · 2025-02-20T07:00:48Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

-        // TODO
+        final var op = context.body();
+        final var atomicBatchTransactionBody = op.atomicBatchOrThrow();
+        requireNonNull(op);


This check makes no sense here.

netopyr · 2025-02-20T07:01:03Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

+        final var op = context.body();
+        final var atomicBatchTransactionBody = op.atomicBatchOrThrow();
+        requireNonNull(op);
+        List<Transaction> transactions = atomicBatchTransactionBody.transactions();


netopyr · 2025-02-20T07:21:49Z

...rc/test/java/com/hedera/node/app/service/util/impl/test/handlers/AtomicBatchHandlerTest.java

-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static com.hedera.hapi.node.base.ResponseCodeEnum.*;
+import static org.junit.jupiter.api.Assertions.*;


We do not use wildcard imports. Please update the settings of your IDE.

Signed-off-by: Kim Rader <[email protected]>

Signed-off-by: Lev Povolotsky <[email protected]>

Signed-off-by: Kim Rader <[email protected]>

# Conflicts: # hedera-node/hedera-app-spi/src/main/java/com/hedera/node/app/spi/workflows/PreHandleContext.java # hedera-node/hedera-app-spi/src/testFixtures/java/com/hedera/node/app/spi/fixtures/workflows/FakePreHandleContext.java # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java # hedera-node/hedera-util-service-impl/src/test/java/com/hedera/node/app/service/util/impl/test/handlers/AtomicBatchHandlerTest.java

Signed-off-by: Kim Rader <[email protected]>

netopyr

LGTM, just some minor comments - thanks @kimbor

netopyr · 2025-02-21T11:38:35Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

+
+        for (final var innerTxBody : atomicBatchTransactionBody.transactions().stream()
+                .map(Transaction::bodyOrThrow)
+                .toList()) {


Interesting approach. I would recommend though to use forEach() on the stream directly.

@iwsimon this one as well please.

netopyr · 2025-02-21T11:41:16Z

...ce-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

-        for (final var body : txnBodies) {
-            final var payerId = body.transactionIDOrThrow().accountIDOrThrow();
+        for (final var innerTxBody :
+                innerTxs.stream().map(Transaction::bodyOrThrow).toList()) {


Same here. In general, I wouldn't put a complex expression inside of a foreach-loop. Using Stream.forEach() is more efficient as we do not have to create the List.

I will make this change in my pr.

TY @iwsimon

Neeharika-Sompalli

LGTM! Thanks @kimbor @povolev15

iwsimon

LGTM, thanks @povolev15 @kimbor

iwsimon and others added 2 commits January 21, 2025 11:21

Added AtomicBatchHandler.handle(),atomicBatchDispatch() and bodyFromT…

f9d7b73

…ransaction() Signed-off-by: Iris Simon <[email protected]>

prehnbadle and purecheck atomic transaction impl

4d80e40

Signed-off-by: Lev Povolotsky <[email protected]>

kimbor added the HiP-551 label Jan 24, 2025

kimbor added this to the v0.60 milestone Jan 24, 2025

kimbor assigned kimbor and povolev15 Jan 24, 2025

kimbor changed the base branch from main to hip-551-batch-txs January 24, 2025 03:36

kimbor changed the title ~~Prehandle purecheck hip 551~~ feat: prehandle purecheck hip 551 Jan 24, 2025

kimbor changed the base branch from hip-551-batch-txs to 17373-batch-handle January 24, 2025 04:12

povolev15 added 7 commits February 3, 2025 13:12

add some fanctionality that neeed base on the design

d60fb66

Signed-off-by: Lev Povolotsky <[email protected]>

remove duplicates

c38f0cb

Signed-off-by: Lev Povolotsky <[email protected]>

fix compile iossues after main merge

567c7d9

Signed-off-by: Lev Povolotsky <[email protected]>

fix compile iossues after main merge

2dc5c8b

Signed-off-by: Lev Povolotsky <[email protected]>

integrate all purecheck and pre handle requests

2b4b614

Signed-off-by: Lev Povolotsky <[email protected]>

Base automatically changed from 17373-batch-handle to hip-551-batch-txs February 5, 2025 16:40

povolev15 and others added 13 commits February 5, 2025 14:06

all pure check and pre handle logic in place

ad26233

Signed-off-by: Lev Povolotsky <[email protected]>

all pure check and pre handle logic in place

a0f5766

Signed-off-by: Lev Povolotsky <[email protected]>

add last excpetions

beba844

Signed-off-by: Lev Povolotsky <[email protected]>

fix the exceptin

e48942a

Signed-off-by: Lev Povolotsky <[email protected]>

Spotless.

8451201

Signed-off-by: Kim Rader <[email protected]>

Fix requires directive

e8920ce

Signed-off-by: Kim Rader <[email protected]>

Spotless

e2c6d9d

Signed-off-by: Kim Rader <[email protected]>

pre handle unit tests

fddf819

Signed-off-by: Lev Povolotsky <[email protected]>

Merge branch 'prehandle-purecheck-hip-551' of github.com:hashgraph/he…

f3c6dd7

…dera-services into prehandle-purecheck-hip-551 # Conflicts: # hedera-node/hedera-util-service-impl/src/main/java/com/hedera/node/app/service/util/impl/handlers/AtomicBatchHandler.java

Add two unit tests

f134e87

Signed-off-by: Kim Rader <[email protected]>

Add four more unit tests

161fe7d

Signed-off-by: Kim Rader <[email protected]>

Comment out failing test

6775f20

Signed-off-by: Kim Rader <[email protected]>

kimbor added 5 commits February 18, 2025 18:05

spotless

4b292ef

Signed-off-by: Kim Rader <[email protected]>

Fix a test and disable a test

d85f869

Signed-off-by: Kim Rader <[email protected]>

Spotless

0ceb2f3

Signed-off-by: Kim Rader <[email protected]>

Fix merge problems, spelling typos

496f3f2

Signed-off-by: Kim Rader <[email protected]>

Neeharika-Sompalli reviewed Feb 19, 2025

View reviewed changes

Consolidate calls to parse inner transactions into a single method

8e44eff

Signed-off-by: Kim Rader <[email protected]>

kimbor requested review from ibankov, iwsimon, edward-swirldslabs, imalygin and Neeharika-Sompalli February 19, 2025 23:43

netopyr reviewed Feb 20, 2025

View reviewed changes

kimbor and others added 10 commits February 20, 2025 07:42

Merge branch 'hip-551-batch-txs' into prehandle-purecheck-hip-551

9bad70f

Respond to reviewer comments

0a2ab51

Signed-off-by: Kim Rader <[email protected]>

added the config param whitelist for atomic batch config

6f8dbcd

Signed-off-by: Lev Povolotsky <[email protected]>

added the config param blacklist

bb90525

Signed-off-by: Lev Povolotsky <[email protected]>

add the black list validation to prehandle

e268e0e

Signed-off-by: Lev Povolotsky <[email protected]>

Respond to remaining reviewer comments

7683a3b

Signed-off-by: Kim Rader <[email protected]>

Removed some unnecessary code

51f126d

Signed-off-by: Kim Rader <[email protected]>

Fix merge errors

90c5983

Signed-off-by: Kim Rader <[email protected]>

Fix failing test

836eb47

Signed-off-by: Kim Rader <[email protected]>

kimbor requested a review from netopyr February 21, 2025 04:12

netopyr approved these changes Feb 21, 2025

View reviewed changes

Neeharika-Sompalli approved these changes Feb 21, 2025

View reviewed changes

iwsimon approved these changes Feb 21, 2025

View reviewed changes

povolev15 merged commit f97cf62 into hip-551-batch-txs Feb 21, 2025
44 checks passed

povolev15 deleted the prehandle-purecheck-hip-551 branch February 21, 2025 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prehandle purecheck hip 551 #17530

feat: prehandle purecheck hip 551 #17530

kimbor commented Jan 24, 2025

Neeharika-Sompalli Feb 19, 2025

kimbor Feb 19, 2025

Neeharika-Sompalli Feb 21, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr Feb 20, 2025

kimbor Feb 21, 2025

netopyr Feb 20, 2025

kimbor Feb 21, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr Feb 20, 2025

kimbor Feb 20, 2025

netopyr left a comment

netopyr Feb 21, 2025

povolev15 Feb 21, 2025

iwsimon Feb 21, 2025

netopyr Feb 21, 2025

iwsimon Feb 21, 2025

povolev15 Feb 21, 2025

Neeharika-Sompalli left a comment

iwsimon left a comment

feat: prehandle purecheck hip 551 #17530

feat: prehandle purecheck hip 551 #17530

Conversation

kimbor commented Jan 24, 2025

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netopyr 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

Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

iwsimon left a comment

Choose a reason for hiding this comment