From c3940ad3d8ba2ede365adfb95dd75272c303554d Mon Sep 17 00:00:00 2001
From: Henry Tsai <henrytsai@outlook.com>
Date: Fri, 19 Mar 2021 15:07:16 -0700
Subject: [PATCH] fix(ref-imp): better transactions API behavior when bitcoin
 service is in forked state

---
 lib/bitcoin/BitcoinProcessor.ts        | 18 +++++++++-------
 lib/core/Observer.ts                   |  2 ++
 tests/bitcoin/BitcoinProcessor.spec.ts | 29 +++++++++-----------------
 3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/lib/bitcoin/BitcoinProcessor.ts b/lib/bitcoin/BitcoinProcessor.ts
index fd6e75713..b7f0a9cc3 100644
--- a/lib/bitcoin/BitcoinProcessor.ts
+++ b/lib/bitcoin/BitcoinProcessor.ts
@@ -449,6 +449,17 @@ export default class BitcoinProcessor {
       };
     }
 
+    // NOTE: this conditional block is technically an optional optimization,
+    // but it is a useful one especially when Bitcoin service's observing loop wait period is longer than that of the Core service's observing loop:
+    // This prevents Core from repeatedly reverting its DB after detecting a fork then repopulating its DB with forked/invalid data again.
+    if (!await this.verifyBlock(lastProcessedBlock.height, lastProcessedBlock.hash)) {
+      Logger.info('Bitcoin service in a forked state, not returning transactions until the DB is reverted to correct chain.');
+      return {
+        moreTransactions: false,
+        transactions: []
+      };
+    }
+
     const [transactions, lastBlockSeen] = await this.getTransactionsSince(since, lastProcessedBlock.height);
 
     // Add normalizedFee to transactions because internal to bitcoin, normalizedFee live in blockMetadata and have to be joined by block height
@@ -472,13 +483,6 @@ export default class BitcoinProcessor {
       }
     }
 
-    // make sure the last processed block hasn't changed since before getting transactions
-    // if changed, then a block reorg happened.
-    if (!await this.verifyBlock(lastProcessedBlock.height, lastProcessedBlock.hash)) {
-      Logger.info('Requested transactions hash mismatched blockchain');
-      throw new RequestError(ResponseStatus.BadRequest, SharedErrorCode.InvalidTransactionNumberOrTimeHash);
-    }
-
     // if last processed block has not been seen, then there are more transactions
     const moreTransactions = lastBlockSeen < lastProcessedBlock.height;
 
diff --git a/lib/core/Observer.ts b/lib/core/Observer.ts
index 0c06ef409..d3725ae23 100644
--- a/lib/core/Observer.ts
+++ b/lib/core/Observer.ts
@@ -191,6 +191,8 @@ export default class Observer {
    * Waits until all unresolvable transactions due for retry are processed.
    */
   private async processUnresolvableTransactions () {
+    Logger.info(`Processing previously unresolvable transactions if any...`);
+
     const endTimer = timeSpan();
     const unresolvableTransactions = await this.unresolvableTransactionStore.getUnresolvableTransactionsDueForRetry();
     Logger.info(`Fetched ${unresolvableTransactions.length} unresolvable transactions to retry in ${endTimer.rounded()} ms.`);
diff --git a/tests/bitcoin/BitcoinProcessor.spec.ts b/tests/bitcoin/BitcoinProcessor.spec.ts
index bd15678fb..9a730d68b 100644
--- a/tests/bitcoin/BitcoinProcessor.spec.ts
+++ b/tests/bitcoin/BitcoinProcessor.spec.ts
@@ -646,30 +646,21 @@ describe('BitcoinProcessor', () => {
       }
     });
 
-    it('should fail if verifyBlock fails after transactionStore query', async (done) => {
+    it('should return no transactions if bitcoin service is in a forked state.', async () => {
       const expectedHeight = randomNumber();
       const expectedHash = randomString();
       const expectedTransactionNumber = TransactionNumber.construct(expectedHeight, 0);
+
+      blockMetadataStoreGetLastSpy.and.returnValues(Promise.resolve('unused'));
+
+      // The 2nd return value simulates a forked state in the bitcoin service.
       const verifyMock = spyOn(bitcoinProcessor, 'verifyBlock' as any).and.returnValues(Promise.resolve(true), Promise.resolve(false));
-      const getTransactionsSinceMock = spyOn(bitcoinProcessor, 'getTransactionsSince' as any).and.returnValue([[], 0]);
-      const mockLastProcessedBlock = {
-        height: 1234,
-        hash: 'some hash',
-        previousHash: 'previous hash'
-      };
-      blockMetadataStoreGetLastSpy.and.returnValue(Promise.resolve(mockLastProcessedBlock));
 
-      try {
-        await bitcoinProcessor.transactions(expectedTransactionNumber, expectedHash);
-        fail('expected to throw');
-      } catch (error) {
-        expect(error.status).toEqual(httpStatus.BAD_REQUEST);
-        expect(error.code).toEqual(SharedErrorCode.InvalidTransactionNumberOrTimeHash);
-        expect(getTransactionsSinceMock).toHaveBeenCalled();
-        expect(verifyMock).toHaveBeenCalledTimes(2);
-      } finally {
-        done();
-      }
+      const response = await bitcoinProcessor.transactions(expectedTransactionNumber, expectedHash);
+      expect(blockMetadataStoreGetLastSpy).toHaveBeenCalledTimes(1);
+      expect(verifyMock).toHaveBeenCalledTimes(2);
+      expect(response.moreTransactions).toBeFalsy();
+      expect(response.transactions.length).toEqual(0);
     });
 
     it('should make moreTransactions true when last block processed is not reached', async (done) => {