Reused PlatformTxnAccessor with handleTransaction from expandSignatures #1474

abhishek-hedera · 2021-05-25T22:35:13Z

Signed-off-by: abhishek-hedera [email protected]

Related issue(s):
Closes #1421

Summary of the change:
Modified ServicesState, ServicesStateTest, ProcessLogic, AwareProcessLogic to reuse PlatformTxnAccessor.
Removed unused imports.

External impacts:
None.

Applicable documentation

Javadoc
HAPI protobuf comments
README

codecov · 2021-05-25T22:43:14Z

Codecov Report

Merging #1474 (4ce8c8a) into master (dca836f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1474      +/-   ##
============================================
+ Coverage     91.39%   91.41%   +0.02%     
- Complexity     5899     5903       +4     
============================================
  Files           461      462       +1     
  Lines         16932    16954      +22     
  Branches       1783     1783              
============================================
+ Hits          15475    15499      +24     
+ Misses          991      990       -1     
+ Partials        466      465       -1

Impacted Files	Coverage Δ
...om/hedera/services/records/TxnIdRecentHistory.java	`96.10% <ø> (ø)`
...c/main/java/com/hedera/services/ServicesState.java	`94.70% <100.00%> (+0.36%)`	⬆️
...vices/legacy/services/state/AwareProcessLogic.java	`30.90% <100.00%> (+0.55%)`	⬆️
...a/com/hedera/services/utils/PlatformTxnRecord.java	`100.00% <100.00%> (ø)`
...a/com/hedera/services/utils/SignedTxnAccessor.java	`97.36% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca836f...4ce8c8a. Read the comment docs.

ljianghedera · 2021-05-26T00:01:40Z

hedera-node/src/main/java/com/hedera/services/ServicesState.java

+		} catch (InvalidProtocolBufferException exception) {
+			log.warn("Consensus platform txn was not gRPC!");


Need coverage for these two lines.

Added more tests.

ljianghedera · 2021-05-26T01:03:08Z

hedera-node/src/main/java/com/hedera/services/ServicesState.java

+			setPlatformTxnAccessor(platformTxn);
 			expandIn(
-					accessor,
+					txnAccessor,


The change here slightly changes original logic. Now line 333 may have txnAccessor as null if line 331setPlatformTxnAccessor(platformTxn); throws exception. So it would be nice to add a test case for this scenario.

Modified logic to use a separate component that would handle the conversion from Transaction to PlatformTxnAccessor.

anighanta · 2021-05-26T20:05:29Z

Description says that PR includes fixed failing test cases of ExpiryManagerTest. Cant find them in the changes

qnswirlds · 2021-05-26T21:17:04Z

hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java

+										PlatformTxnAccessor accessor,
+										Instant consensusTime, long submittingMember) throws InvalidProtocolBufferException{


nit: the indentation seems to be way off here

qnswirlds

It seems to me that the PR is not ready for review. Will re-review when it is ready.

abhishek-hedera · 2021-05-26T21:31:35Z

Description says that PR includes fixed failing test cases of ExpiryManagerTest. Cant find them in the changes

A few tests used observe() from TxnIdRecentHistory, which did not have the scope to be called from the ExpiryManagerTest. Modified it to be public instead.

hedera-services/hedera-node/src/test/java/com/hedera/services/state/expiry/ExpiryManagerTest.java

Line 160 in b14894b

    
           liveTxnHistories.computeIfAbsent(newTxnId, ignore -> new TxnIdRecentHistory()).observe(firstRecord, OK);

hedera-services/hedera-node/src/test/java/com/hedera/services/state/expiry/ExpiryManagerTest.java

Line 165 in b14894b

    
           liveTxnHistories.computeIfAbsent(newTxnId, ignore -> new TxnIdRecentHistory()).observe(secondRecord, OK);

hedera-services/hedera-node/src/test/java/com/hedera/services/state/expiry/ExpiryManagerTest.java

Line 187 in b14894b

    
           liveTxnHistories.computeIfAbsent(newTxnId, ignore -> new TxnIdRecentHistory()).observe(firstRecord, OK);

abhishek-hedera · 2021-05-26T21:35:45Z

It seems to me that the PR is not ready for review. Will re-review when it is ready.

After reviewing with @tinker-michaelj a few modifications needs to be added such as using Cache for PlatformTxnAccessor. Converting it to draft for now to be reopened later. Thanks for the review.

Signed-off-by: abhishek-hedera <[email protected]>

ljianghedera · 2021-06-01T15:01:54Z

hedera-node/src/main/java/com/hedera/services/utils/PlatformTxnRecord.java

+
+public class PlatformTxnRecord {
+
+    private Cache<Transaction, PlatformTxnAccessor> cache;


Can you put some comments to explain what's the purpose of the PlatformTxnRecord? and also why a map of <Transaction, PlatformTxnAccessor> is needed here?

ljianghedera · 2021-06-01T15:25:17Z

hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java

+		if (!ctx.invariants().holdFor(accessor, effectiveConsensusTime, submittingMember)) {
+			return;


Seems like something unusual happened here, should we have a log message here before return?

ljianghedera · 2021-06-01T15:32:28Z

hedera-node/src/main/java/com/hedera/services/legacy/services/state/AwareProcessLogic.java

-				effectiveConsensusTime = consensusTime.minusNanos(1);
-			}
+	public void incorporateConsensusTxn(
+			Transaction platformTxn,


Is the parameter platformTxn used anywhere in this method?

abhishek-hedera · 2021-06-01T19:11:20Z

As per the discussion in service team slack channel, this PR will be closed in order to merge the below PRs with new SDK.

https://github.com/hashgraph/hedera-services/compare/master-plus-sdk-0.15.0-alpha.1

https://github.com/hashgraph/hedera-services/tree/01461-M0150-SimplifyFeeCharging

https://github.com/hashgraph/hedera-services/tree/01421-01461-CreateExpandHandleSpan

abhishek-hedera requested review from tinker-michaelj, qnswirlds, anighanta, Neeharika-Sompalli, rbair23 and ljianghedera May 25, 2021 22:35

ljianghedera reviewed May 26, 2021

View reviewed changes

qnswirlds reviewed May 26, 2021

View reviewed changes

abhishek-hedera marked this pull request as draft May 26, 2021 21:22

abhishek-hedera force-pushed the 01421-M-ReusingSignedTxnAccessor branch from c56e488 to 2596ae3 Compare May 27, 2021 19:05

abhishek-hedera added 5 commits May 28, 2021 08:50

reused PlatformTxnAccessor with handleTransaction

1740cc8

Signed-off-by: abhishek-hedera <[email protected]>

added more test coverage

f6912c3

Signed-off-by: abhishek-hedera <[email protected]>

removed unused dependencies

9153447

Signed-off-by: abhishek-hedera <[email protected]>

added PlatformTxnRecord

4386ae6

Signed-off-by: abhishek-hedera <[email protected]>

integrating PlatformTxnRecord with ServicesState

314d040

Signed-off-by: abhishek-hedera <[email protected]>

abhishek-hedera force-pushed the 01421-M-ReusingSignedTxnAccessor branch from 467b037 to 314d040 Compare May 28, 2021 16:03

removed unused imports from ProcessLogic

fda8f19

Signed-off-by: abhishek-hedera <[email protected]>

abhishek-hedera marked this pull request as ready for review May 28, 2021 16:13

abhishek-hedera added 2 commits May 28, 2021 09:17

fixed indentation

1b9ea6f

Signed-off-by: abhishek-hedera <[email protected]>

fixed gcp failure on null accessor

4ce8c8a

Signed-off-by: abhishek-hedera <[email protected]>

ljianghedera reviewed Jun 1, 2021

View reviewed changes

abhishek-hedera closed this Jun 2, 2021

tinker-michaelj deleted the 01421-M-ReusingSignedTxnAccessor branch August 30, 2021 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reused PlatformTxnAccessor with handleTransaction from expandSignatures #1474

Reused PlatformTxnAccessor with handleTransaction from expandSignatures #1474

abhishek-hedera commented May 25, 2021 •

edited

Loading

codecov bot commented May 25, 2021 •

edited

Loading

ljianghedera May 26, 2021

abhishek-hedera May 28, 2021

ljianghedera May 26, 2021

abhishek-hedera May 28, 2021

anighanta commented May 26, 2021

qnswirlds May 26, 2021

abhishek-hedera May 28, 2021

qnswirlds left a comment

abhishek-hedera commented May 26, 2021

abhishek-hedera commented May 26, 2021

ljianghedera Jun 1, 2021

ljianghedera Jun 1, 2021

ljianghedera Jun 1, 2021

abhishek-hedera commented Jun 1, 2021

		} catch (InvalidProtocolBufferException exception) {
		log.warn("Consensus platform txn was not gRPC!");

		PlatformTxnAccessor accessor,
		Instant consensusTime, long submittingMember) throws InvalidProtocolBufferException{


		public class PlatformTxnRecord {

		private Cache<Transaction, PlatformTxnAccessor> cache;

		if (!ctx.invariants().holdFor(accessor, effectiveConsensusTime, submittingMember)) {
		return;

Reused PlatformTxnAccessor with handleTransaction from expandSignatures #1474

Reused PlatformTxnAccessor with handleTransaction from expandSignatures #1474

Conversation

abhishek-hedera commented May 25, 2021 • edited Loading

codecov bot commented May 25, 2021 • edited Loading

Codecov Report

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

anighanta commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qnswirlds left a comment

Choose a reason for hiding this comment

abhishek-hedera commented May 26, 2021

abhishek-hedera commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhishek-hedera commented Jun 1, 2021

abhishek-hedera commented May 25, 2021 •

edited

Loading

codecov bot commented May 25, 2021 •

edited

Loading