Number overflow halting parser on crypto/file/contract update (#409)

* Fix #386 number overflow converting from (long sec, int nanos) to long nanos_timestamp affecting entity (crypto account, file, contract) updates. Signed-off-by: Mike Burrage <mike.burrage@hedera.com> * Address review feedback Signed-off-by: Steven Sheehy <steven.sheehy@hedera.com> * Add test for create entity with expiration flow Signed-off-by: Steven Sheehy <steven.sheehy@hedera.com> * Added rest api integration test to verify min and max set for account expiration time Signed-off-by: Nana-EC <56320167+Nana-EC@users.noreply.github.com> * Removed unnecessary tests Signed-off-by: Nana-EC <56320167+Nana-EC@users.noreply.github.com>
hashgraph · Nov 27, 2019 · 9ba5d23 · 9ba5d23
1 parent 5d9f74a
commit 9ba5d23
Show file tree

Hide file tree

Showing 9 changed files with 155 additions and 37 deletions.
diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md
@@ -63,6 +63,7 @@ issues are resolvable by updating the application configuration and restarting t
 These should be investigated by Hedera, but require no immediate escalation unless they are occurring frequently (more than 1/min).
 
 -   `Transaction type .* not known to mirror node`
+-   `Long overflow when converting time to nanos timestamp`
 
 ## Known Issues
 

diff --git a/rest-api/__tests__/integration.test.js b/rest-api/__tests__/integration.test.js
@@ -160,14 +160,14 @@ const flywayMigrate = function() {
 const shard = 0;
 const realm = 15;
 const accountEntityIds = {};
-const addAccount = async function(accountId) {
+const addAccount = async function(accountId, exp_tm_nanosecs = 4223372036854775807) {
   let e = accountEntityIds[accountId];
   if (e) {
     return e;
   }
   let res = await sqlConnection.query(
-    'insert into t_entities (fk_entity_type_id, entity_shard, entity_realm, entity_num) values ($1, $2, $3, $4) returning id;',
-    [1, shard, realm, accountId]
+    'insert into t_entities (fk_entity_type_id, entity_shard, entity_realm, entity_num, exp_time_ns) values ($1, $2, $3, $4, $5) returning id;',
+    [1, shard, realm, accountId, exp_tm_nanosecs]
   );
   e = res.rows[0]['id'];
   accountEntityIds[accountId] = e;
@@ -242,16 +242,26 @@ const addCryptoTransferTransaction = async function(
 /**
  * Setup test data in the postgres instance.
  */
+const accountCount = 10;
+const minExpiryAccountId = accountCount - 1;
+const maxExpiryAccountId = accountCount;
 const setupData = async function() {
   let res = await sqlConnection.query('insert into t_record_files (name) values ($1) returning id;', ['test']);
   let fileId = res.rows[0]['id'];
   console.log(`Record file id is ${fileId}`);
 
-  const accountCount = 10;
   const balancePerAccountCount = 3;
   console.log(`Adding ${accountCount} accounts with ${balancePerAccountCount} balances per account`);
   for (var i = 1; i <= accountCount; ++i) {
-    await addAccount(i);
+    if (i == maxExpiryAccountId) {
+    // set max expiry time for account accountCount
+      await addAccount(i, '9223372036854775807');
+    } else if (i == minExpiryAccountId) {
+    // set min expiry time for account accountCount - 1
+      await addAccount(i, '-9223372036854775808');
+    } else {
+      await addAccount(i);
+    }
     // Add 3 balances for each account.
     for (var ts = 0; ts < balancePerAccountCount; ++ts) {
       await sqlConnection.query(

diff --git a/rest-api/__tests__/specs/accounts-01-no-args.spec.json b/rest-api/__tests__/specs/accounts-01-no-args.spec.json
@@ -9,7 +9,7 @@
           "balance": 10
         },
         "account": "0.15.1",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -20,7 +20,7 @@
           "balance": 20
         },
         "account": "0.15.2",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -31,7 +31,7 @@
           "balance": 30
         },
         "account": "0.15.3",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -42,7 +42,7 @@
           "balance": 40
         },
         "account": "0.15.4",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -53,7 +53,7 @@
           "balance": 50
         },
         "account": "0.15.5",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -64,7 +64,7 @@
           "balance": 60
         },
         "account": "0.15.6",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -75,7 +75,7 @@
           "balance": 70
         },
         "account": "0.15.7",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -86,7 +86,7 @@
           "balance": 80
         },
         "account": "0.15.8",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "4223372036.854776000",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -97,7 +97,7 @@
           "balance": 90
         },
         "account": "0.15.9",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "-9223372036.854775808",
         "auto_renew_period": null,
         "key": null,
         "deleted": false
@@ -108,7 +108,7 @@
           "balance": 100
         },
         "account": "0.15.10",
-        "expiry_timestamp": null,
+        "expiry_timestamp": "9223372036.854775807",
         "auto_renew_period": null,
         "key": null,
         "deleted": false

diff --git a/rest-api/__tests__/utils.test.js b/rest-api/__tests__/utils.test.js
@@ -20,7 +20,6 @@
 'use strict';
 
 const request = require('supertest');
-const math = require('mathjs');
 const utils = require('../utils.js');
 
 describe('Utils getNullableNumber tests', () => {

diff --git a/src/main/java/com/hedera/mirror/parser/record/Entities.java b/src/main/java/com/hedera/mirror/parser/record/Entities.java
@@ -20,9 +20,6 @@
  * ‍
  */
 
-import static java.sql.Types.VARBINARY;
-import static java.sql.Types.VARCHAR;
-
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.hederahashgraph.api.proto.java.AccountID;
 import com.hederahashgraph.api.proto.java.ContractID;
@@ -35,12 +32,15 @@
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.Types;
+import java.time.DateTimeException;
 import java.util.HashMap;
 
 import lombok.extern.log4j.Log4j2;
 
 import com.hedera.mirror.util.Utility;
 
+import static java.sql.Types.*;
+
 @Log4j2
 public class Entities {
     private static int FK_ACCOUNT = 0;
@@ -139,7 +139,7 @@ private long updateEntity(int fk_entity_type, long shard, long realm, long num,
             if ((exp_time_seconds != 0) || (exp_time_nanos != 0)) {
                 updateEntity.setLong(1, exp_time_seconds);
                 updateEntity.setLong(2, exp_time_nanos);
-                updateEntity.setLong(3, Utility.convertToNanos(exp_time_seconds, exp_time_nanos));
+                updateEntity.setLong(3, Utility.convertToNanosMax(exp_time_seconds, exp_time_nanos));
                 fieldCount = 3;
             }
 
@@ -332,8 +332,7 @@ private long createEntity(long shard, long realm, long num, long exp_time_second
             entityCreate.setInt(5, fk_entity_type);
             entityCreate.setLong(6, exp_time_seconds);
             entityCreate.setLong(7, exp_time_nanos);
-
-            entityCreate.setLong(8, Utility.convertToNanos(exp_time_seconds, exp_time_nanos));
+            entityCreate.setLong(8, Utility.convertToNanosMax(exp_time_seconds, exp_time_nanos));
             entityCreate.setLong(9, auto_renew_period);
 
             if (key == null) {

diff --git a/src/main/java/com/hedera/mirror/util/Utility.java b/src/main/java/com/hedera/mirror/util/Utility.java
@@ -591,19 +591,32 @@ public static boolean greaterThanSuperMajorityNum(long n, long N) {
      * Convert an Instant to a Long type timestampInNanos
      */
     public static Long convertInstantToNanos(Instant instant) {
+        return convertToNanos(instant.getEpochSecond(), instant.getNano());
+    }
+
+    /**
+     * Converts time in (second, nanos) to time in only nanos.
+     */
+    public static Long convertToNanos(long second, long nanos) {
         try {
-            return Math.addExact(Math.multiplyExact(instant.getEpochSecond(), SCALAR), instant.getNano());
+            return Math.addExact(Math.multiplyExact(second, SCALAR), nanos);
         } catch (ArithmeticException e) {
-            log.error("Long overflow when converting Instant to nanos timestamp : {}", instant, e);
+            log.error("Long overflow when converting time to nanos timestamp : {}s {}ns", second, nanos);
             throw e;
         }
     }
 
     /**
-     * Converts time in (second, nanos) to time in only nanos.
+     * Converts time in (second, nanos) to time in only nanos, with a fallback if overflow:
+     * If positive overflow, return the max time in the future (Long.MAX_VALUE).
+     * If negative overflow, return the max time in the past (Long.MIN_VALUE).
      */
-    public static Long convertToNanos(long second, long nanos) {
-        return convertInstantToNanos(Instant.ofEpochSecond(second, nanos));
+    public static Long convertToNanosMax(long second, long nanos) {
+        try {
+            return convertToNanos(second, nanos);
+        } catch (ArithmeticException ex) {
+            return second >= 0 ? Long.MAX_VALUE : Long.MIN_VALUE;
+        }
     }
 
     /**

diff --git a/src/test/java/com/hedera/mirror/parser/record/RecordFileLoggerCryptoTest.java b/src/test/java/com/hedera/mirror/parser/record/RecordFileLoggerCryptoTest.java
@@ -51,7 +51,10 @@
 import org.apache.commons.codec.binary.Hex;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.springframework.test.context.jdbc.Sql;
 
 import com.hedera.mirror.domain.Entities;
@@ -470,6 +473,51 @@ void cryptoUpdateSuccessfulTransaction() throws Exception {
         );
     }
 
+    @DisplayName("update account such that expiration timestamp overflows nanos_timestamp")
+    @ParameterizedTest(name = "with seconds {0} and expectedNanosTimestamp {1}")
+    @CsvSource({
+            "9223372036854775807, 9223372036854775807",
+            "31556889864403199, 9223372036854775807",
+            "-9223372036854775808, -9223372036854775808",
+            "-1000000000000000000, -9223372036854775808"
+    })
+    void cryptoUpdateExpirationOverflow(long seconds, long expectedNanosTimestamp) throws Exception {
+
+        // first create the account
+        var createTransaction = cryptoCreateTransaction();
+        var createTransactionBody = TransactionBody.parseFrom(createTransaction.getBodyBytes());
+        var createRecord = transactionRecordSuccess(createTransactionBody);
+
+        RecordFileLogger.storeRecord(createTransaction, createRecord);
+
+        // now update
+        var updateTransaction = Transaction.newBuilder();
+        var updateTransactionBody = CryptoUpdateTransactionBody.newBuilder();
+        updateTransactionBody.setAccountIDToUpdate(accountId);
+
+        // *** THIS IS THE OVERFLOW WE WANT TO TEST ***
+        // This should result in the entity having a NULL expiration
+        updateTransactionBody.setExpirationTime(Timestamp.newBuilder().setSeconds(seconds));
+
+        var transactionBody = defaultTransactionBodyBuilder(memo);
+        transactionBody.setCryptoUpdateAccount(updateTransactionBody.build());
+        updateTransaction.setBodyBytes(transactionBody.build().toByteString());
+        var rec = transactionRecordSuccess(transactionBody.build());
+
+        RecordFileLogger.storeRecord(updateTransaction.build(), rec);
+        RecordFileLogger.completeFile("", "");
+
+        var dbTransaction = transactionRepository
+                .findById(Utility.timeStampInNanos(rec.getConsensusTimestamp())).get();
+        var dbAccountEntity = entityRepository.findById(dbTransaction.getEntityId()).get();
+
+        assertAll(
+                () -> assertEquals(1, recordFileRepository.count()),
+                () -> assertEquals(2, transactionRepository.count()),
+                () -> assertEquals(expectedNanosTimestamp, dbAccountEntity.getExpiryTimeNs())
+        );
+    }
+
     @Test
     void cryptoUpdateFailedTransaction() throws Exception {
 

diff --git a/src/test/java/com/hedera/mirror/parser/record/RecordFileLoggerFileTest.java b/src/test/java/com/hedera/mirror/parser/record/RecordFileLoggerFileTest.java
@@ -20,12 +20,14 @@
  * ‍
  */
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.*;
 
 import com.google.protobuf.ByteString;
 
 import com.hederahashgraph.api.proto.java.AccountAmount;
 import com.hederahashgraph.api.proto.java.AccountID;
+import com.hederahashgraph.api.proto.java.CryptoUpdateTransactionBody;
 import com.hederahashgraph.api.proto.java.FileAppendTransactionBody;
 import com.hederahashgraph.api.proto.java.FileCreateTransactionBody;
 import com.hederahashgraph.api.proto.java.FileDeleteTransactionBody;
@@ -53,8 +55,11 @@
 import org.apache.commons.io.FileUtils;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 import org.springframework.test.context.jdbc.Sql;
 
 import com.hedera.mirror.MirrorProperties;
@@ -320,6 +325,26 @@ void fileCreatePersistSystemNegative() throws Exception {
         );
     }
 
+    @ParameterizedTest(name = "with {0} s and expected {1} ns")
+    @CsvSource({
+            "9223372036854775807, 9223372036854775807",
+            "31556889864403199, 9223372036854775807",
+            "-9223372036854775808, -9223372036854775808",
+            "-1000000000000000000, -9223372036854775808"
+    })
+    void fileCreateExpirationTimeOverflow(long seconds, long expectedNanosTimestamp) throws Exception {
+        final Transaction transaction = fileCreateTransaction(Timestamp.newBuilder().setSeconds(seconds).build());
+        final TransactionBody transactionBody = TransactionBody.parseFrom(transaction.getBodyBytes());
+        final TransactionRecord record = transactionRecord(transactionBody);
+
+        RecordFileLogger.storeRecord(transaction, record);
+        RecordFileLogger.completeFile("", "");
+
+        var dbTransaction = transactionRepository.findById(Utility.timeStampInNanos(record.getConsensusTimestamp())).get();
+        var dbAccountEntity = entityRepository.findById(dbTransaction.getEntityId()).get();
+        assertEquals(expectedNanosTimestamp, dbAccountEntity.getExpiryTimeNs());
+    }
+
     @Test
     void fileAppendToExisting() throws Exception {
 
@@ -1615,20 +1640,24 @@ private TransactionRecord transactionRecord(TransactionBody transactionBody, Res
     }
 
     private Transaction fileCreateTransaction() {
+        return fileCreateTransaction(Timestamp.newBuilder()
+                .setSeconds(1571487857L)
+                .setNanos(181579000)
+                .build());
+    }
+
+    private Transaction fileCreateTransaction(Timestamp expirationTime) {
 
         final Transaction.Builder transaction = Transaction.newBuilder();
         final FileCreateTransactionBody.Builder fileCreate = FileCreateTransactionBody.newBuilder();
 
         // file create
         final String fileData = "Hedera hashgraph is great!";
-        final long expiryTimeSeconds = 1571487857L;
-        final int expiryTimeNanos = 181579000;
         final String key = "0a2212200aa8e21064c61eab86e2a9c164565b4e7a9a4146106e0a6cd03a8c395a110e92";
 
         // Build a transaction
         fileCreate.setContents(ByteString.copyFromUtf8(fileData));
-        fileCreate.setExpirationTime(Timestamp.newBuilder().setSeconds(expiryTimeSeconds).setNanos(expiryTimeNanos)
-                .build());
+        fileCreate.setExpirationTime(expirationTime);
         final KeyList.Builder keyList = KeyList.newBuilder();
         keyList.addKeys(keyFromString(key));
         fileCreate.setKeys(keyList);