From 4544d6217e864947b4744e39c2a4454b0673d51e Mon Sep 17 00:00:00 2001
From: Hyunjae Lee <linked0@gmail.com>
Date: Thu, 10 Feb 2022 16:01:37 +0900
Subject: [PATCH] Fix Flash failure on CI test

The unittests in `agora.test.Flash` fails often on CI because there is
a timing issue. Unittests expect `update_tx`s not to be sent as soon as
a block is externalized when the `allow_publish` is set to `false`. But
the `update_tx` is sent as soon as the `allow_publish` is to set `true`
by calling `setPublishEnable`, which is not to be supposed to be sent.
---
 source/agora/flash/Channel.d | 20 +++++++++++++-------
 source/agora/test/Flash.d    | 28 ++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/source/agora/flash/Channel.d b/source/agora/flash/Channel.d
index e48dbb8640b..ef236c667d4 100644
--- a/source/agora/flash/Channel.d
+++ b/source/agora/flash/Channel.d
@@ -739,10 +739,6 @@ LOuter: while (1)
 
     public void onUpdateTxExternalized (in Transaction tx, in uint utxo_idx, in bool is_last)
     {
-        this.state = ChannelState.StartedUnilateralClose;
-        this.onChannelNotify(this.own_pk, this.conf.chan_id, this.state,
-            ErrorCode.None);
-
         this.last_externalized_update_utxo = UTXO.getHash(tx.hashFull(), utxo_idx);
         if (is_last)
         {
@@ -753,7 +749,13 @@ LOuter: while (1)
         }
         else
         {
-            this.publishUpdateTx(this.channel_updates[$ - 1]);
+            this.taskman.setTimer(100.msecs,
+            {
+                this.publishUpdateTx(this.channel_updates[$ - 1]);
+                this.state = ChannelState.StartedUnilateralClose;
+                this.onChannelNotify(this.own_pk, this.conf.chan_id, this.state,
+                    ErrorCode.None, this.height);
+            });
         }
     }
 
@@ -2221,6 +2223,8 @@ LOuter: while (1)
 
     protected Transaction publishUpdateTx (in UpdatePair update)
     {
+        import agora.utils.PrettyPrinter;
+
         auto update_tx = update.update_tx.serializeFull.deserializeFull!Transaction();
         assert(update_tx.inputs.length == 1);
         assert(update_tx.outputs.length == 1);
@@ -2264,9 +2268,11 @@ LOuter: while (1)
                 update_tx.inputs[idx].unlock = genKeyUnlock(fee_sig);
 
         auto result = this.txPublisher(update_tx);
-        log.info("{}: Publishing update tx {}: {}. Result: {}",
+        // NOTE: Added the content of the update tx in the log for debugging
+        // about the failure on the CI build but will be removed after fixing it.
+        log.info("{}: Publishing update tx {}: {}. Result: {}. Tx: {}",
             this.own_pk.flashPrettify, update.seq_id, update_tx.hashFull().flashPrettify,
-            result);
+            result, update_tx.prettify);
         return update_tx;
     }
 
diff --git a/source/agora/test/Flash.d b/source/agora/test/Flash.d
index ff34d29e3ba..1c0cb849569 100644
--- a/source/agora/test/Flash.d
+++ b/source/agora/test/Flash.d
@@ -74,9 +74,10 @@ public interface TestFlashListenerAPI : FlashListenerAPI
     /// has succeeded / failed, and return true / false for success
     ErrorCode waitUntilNotified (Invoice);
 
-    /// wait until we get a notification about the given channel state,
-    /// and return any associated error codes
-    ErrorCode waitUntilChannelState (Hash, ChannelState, PublicKey node = PublicKey.init);
+    /// wait until we get a notification about the given channel state
+    /// with the specified height and return any associated error codes
+    ErrorCode waitUntilChannelState (Hash, ChannelState, PublicKey node = PublicKey.init,
+        Height height = Height(0), bool erase_state = true);
 
     /// Print out the contents of the log
     public void printLog ();
@@ -542,16 +543,24 @@ private class FlashListener : TestFlashListenerAPI
     }
 
     public ErrorCode waitUntilChannelState (Hash chan_id, ChannelState state,
-        PublicKey node = PublicKey.init)
+        PublicKey node = PublicKey.init, Height height = Height(0),
+        bool erase_state = true)
     {
-        scope (exit) this.channel_state.remove(chan_id);
+        // There is no case where we check the state with a specified
+        // height and no public key.
+        assert(height == Height(0) || node != PublicKey.init);
+
+        scope (exit) if (erase_state) this.channel_state.remove(chan_id);
         while (1)
         {
             if (auto chan_states = chan_id in this.channel_state)
             {
                 if (auto chan_state = node in *chan_states)
-                    if ((*chan_state).state >= state)
+                {
+                    if ((*chan_state).state >= state &&
+                        (*chan_state).height >= height)
                         return (*chan_state).error;
+                }
                 if (node == PublicKey.init)
                 {
                     auto states = chan_states.byValue
@@ -1925,6 +1934,13 @@ unittest
     network.clients[0].postTransaction(update_tx);
     assert(!network.clients[0].hasTransactionHash(update_tx.hashFull()));
 
+    // wait until all the flash nodes skip publishing `update tx` before
+    // the `setPublishEnable` is called.
+    network.listener.waitUntilChannelState(chan_id,
+        ChannelState.StartedUnilateralClose, WK.Keys.A.address, Height(4), false);
+    network.listener.waitUntilChannelState(chan_id,
+        ChannelState.StartedUnilateralClose, WK.Keys.C.address, Height(4));
+
     // allow normal node operation again
     alice.setPublishEnable(true);
     charlie.setPublishEnable(true);