improve reorg by using getbestblockhash

[LarryRuane]

This is the implementation of the suggestion here: #348 (comment). This should make that PR unnecessary (since the latest block height won't jump around). This new version also has the virtue of being simpler.

@adityapk00 and @nighthawk24, perhaps you can review and test these changes.

CC @pacu @defuse

[pacu]

Hey @LarryRuane, I just checked out your code and ran darksidewalletd locally with it. When I fired up one of my tests, lightwalletd failed with this error

{"app":"lightwalletd","error":"there was an attempt to call an unsupported RPC","level":"fatal","msg":"error zcashd getbestblockhash rpc","time":"2021-08-18T18:39:46-03:00"}
Lightwalletd died with a Fatal error. Check logfile for details.

[pacu]

Greener pastures! :)

[pacu]

Master branch:

Some regressions

[pacu]

I everything passes except for one test

func testReOrgChangesOutboundTxMinedHeight() throws {
        hookToReOrgNotification()
        /*
         1. create fake chain
         */
        try FakeChainBuilder.buildChain(darksideWallet: coordinator.service, branchID: branchID, chainName: chainName)
        
        try coordinator.applyStaged(blockheight: 663188)
        sleep(2)
        
        let firstSyncExpectation = XCTestExpectation(description: "first sync")
        /*
         1a. sync to latest height
         */
        try coordinator.sync(completion: { (s) in   
            firstSyncExpectation.fulfill()
        }, error: self.handleError)
        
        wait(for: [firstSyncExpectation], timeout: 5)
        
        sleep(1)
        let initialTotalBalance = coordinator.synchronizer.initializer.getBalance()
        
        let sendExpectation = XCTestExpectation(description: "send expectation")
        var p: PendingTransactionEntity? = nil
        
        /*
         2. send transaction to recipient address
         */
        coordinator.synchronizer.sendToAddress(spendingKey: self.coordinator.spendingKeys!.first!, zatoshi: 20000, toAddress: self.testRecipientAddress, memo: "this is a test", from: 0, resultBlock: { (result) in
            switch result {
            case .failure(let e):
                self.handleError(e)
            case .success(let pendingTx):
                p = pendingTx
            }
            sendExpectation.fulfill()
        })
        
        wait(for: [sendExpectation], timeout: 11)
        
        guard let _ = p else {
            XCTFail("no pending transaction after sending")
            try coordinator.stop()
            return
        }
        /**
         3. getIncomingTransaction
         */
        guard let incomingTx = try coordinator.getIncomingTransactions()?.first else {
            XCTFail("no incoming transaction")
            try coordinator.stop()
            return
        }
        
        let sentTxHeight: BlockHeight = 663189
        
        
        /*
         4. stage transaction at sentTxHeight
         */
        
        try coordinator.stageBlockCreate(height: sentTxHeight)
        
        try coordinator.stageTransaction(incomingTx, at: sentTxHeight)
        /*
         5. applyHeight(sentTxHeight)
         */
        try coordinator.applyStaged(blockheight: sentTxHeight)
        
        sleep(2)
        
        /*
         6. sync to latest height
         */
        let secondSyncExpectation =  XCTestExpectation(description: "after send expectation")
        
        try coordinator.sync(completion: { (s) in
            secondSyncExpectation.fulfill()
        }, error: self.handleError)
        
        wait(for: [secondSyncExpectation], timeout: 5)
        
        XCTAssertEqual(coordinator.synchronizer.pendingTransactions.count, 1)
        guard let afterStagePendingTx = coordinator.synchronizer.pendingTransactions.first else {
            return
        }
        
        /*
         6a. verify that there's a pending transaction with a mined height of sentTxHeight
         */
        
        XCTAssertEqual(afterStagePendingTx.minedHeight, sentTxHeight)
        
        /*
         7. stage 20  blocks from sentTxHeight
         */
        try coordinator.stageBlockCreate(height: sentTxHeight, count: 25)
        
        /*
         7a. stage sent tx to sentTxHeight + 2
         */
        try coordinator.stageTransaction(incomingTx, at: sentTxHeight + 2)
        
        /*
         8. applyHeight(sentTxHeight + 1) to cause a 1 block reorg
         */
        try coordinator.applyStaged(blockheight: sentTxHeight + 1)
        sleep(2)
        
        /*
         9. sync to latest height
         */
        self.expectedReorgHeight = sentTxHeight + 1
        let afterReorgExpectation = XCTestExpectation(description: "after reorg sync")
        
        try coordinator.sync(completion: { (s) in
            afterReorgExpectation.fulfill()
        }, error: self.handleError)
        
        wait(for: [reorgExpectation,afterReorgExpectation], timeout: 5)
        
        /*
         10. verify that there's a pending transaction with -1 mined height
         */
        guard let newPendingTx = coordinator.synchronizer.pendingTransactions.first else {
            XCTFail("No pending transaction")
            try coordinator.stop()
            return
        }
        
        XCTAssertEqual(newPendingTx.minedHeight, BlockHeight.empty())
        
        /*
         11. applyHeight(sentTxHeight + 2)
         */
        try coordinator.applyStaged(blockheight: sentTxHeight + 2)
        sleep(2)
        
        
        let yetAnotherExpectation = XCTestExpectation(description: "after staging expectation")
        
        /*
         11a. sync to latest height
         */
        try coordinator.sync(completion: { (s) in
            yetAnotherExpectation.fulfill()
        }, error: self.handleError)
        
        wait(for: [yetAnotherExpectation], timeout: 5)
        
        
        /*
         12. verify that there's a pending transaction with a mined height of sentTxHeight + 2
         */
        
        XCTAssertEqual(coordinator.synchronizer.pendingTransactions.count,1)
        guard let newlyPendingTx = try coordinator.synchronizer.allPendingTransactions().first else {
            XCTFail("no pending transaction")
            try coordinator.stop()
            return
        }
        
        XCTAssertEqual(newlyPendingTx.minedHeight, sentTxHeight + 2)
        
        /*
         13. apply height(sentTxHeight + 25)
         */
        
        try coordinator.applyStaged(blockheight: sentTxHeight + 25)
        
        sleep(2)
        
        let thisIsTheLastExpectationIPromess = XCTestExpectation(description: "last sync")
        /*
         14. sync to latest height
         */
        
        try coordinator.sync(completion: { (s) in
            thisIsTheLastExpectationIPromess.fulfill()
        }, error: self.handleError)
        
        wait(for: [thisIsTheLastExpectationIPromess], timeout: 5) // <<--- here it failed because dLwd crashed

This is the dLWD log

{"app":"lightwalletd","level":"info","msg":"Darkside active block height 663212 hash 1c871b65a7118d0c78c64c738aa85f400810b47bbeeee1924f3f0be9005c5f5c txcount 1","time":"2021-08-20T09:02:50-03:00"}
{"app":"lightwalletd","level":"info","msg":"Darkside active block height 663213 hash 61e020dd1cb2392be425cbf7ced68e0b864fcfbf359080c102043ea2fb04d5b0 txcount 1","time":"2021-08-20T09:02:50-03:00"}
{"app":"lightwalletd","level":"info","msg":"darkside: active blocks from 663150 to 663213, latest presented height 663214","time":"2021-08-20T09:02:50-03:00"}
panic: runtime error: index out of range [64] with length 64

goroutine 2363 [running]:
github.com/zcash/lightwalletd/common.darksideRawRequest(0x18f1881, 0x10, 0x20cd3e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
   [redacted]/lightwalletd/common/darkside.go:424 +0x1d44
github.com/zcash/lightwalletd/common.BlockIngestor(0xc0002fe000, 0x0)
   [redacted]/lightwalletd/common/common.go:305 +0x102
created by github.com/zcash/lightwalletd/common.startIngestor
   [redacted]/lightwalletd/common/common.go:279 +0x62

when I change this failing statement of the test, it works

/*
        13. apply height(sentTxHeight + 25)
        */
       
       try coordinator.applyStaged(blockheight: sentTxHeight + 24)

But, latest stageBlockCreate was
try coordinator.stageBlockCreate(height: sentTxHeight, count: 25)
this means 25 blocks height: 663189
was there a change in how the apply staged considers the staging or in how the stageBlockCreate considers the starting point?

[LarryRuane]

Thanks for all this information, @pacu, very helpful.

I unintentionally made the behavior of ApplyStaged stricter: If you stage blocks 10 and 11, and then ApplyStaged(height=12), it used to succeed, and darkside would "present" block 11 to lightwalletd as the latest block (so you would see "waiting for block 12"). Any height argument greater than 11 was treated the same as 11. The new code (this PR) was failing in this case (lightwalletd panics). I just pushed a change so that the old behavior is restored. Ideally, applying at height 12 should fail (but gracefully, not panic), but since in the past darkside has allowed this, let's keep the behavior the same. It should work now.

[pacu]

Ok that's great! Maybe we can discuss these changes to be added later!

[pacu]

All Darksidewalletd tests pass now :)