tcp_client::init bug fix

[mszulcz-mitre]

GitHub Issue #131 describes a bug that makes it impossible to trigger the error "Failed to start coordinator client" in sentinel_2pc::controller::init (src/uhs/twophase/sentinel_2pc/controller.cpp, lines 38-41). This pull request contains a commit that fixes the bug using the method described in Issue #131. The fix causes the unit test tcp_rpc_test.send_fail_test to fail, so the next commit in this pull request fixes it by testing for behavior that's expected after the bug fix. The final commit in this pull request adds a new unit test that triggers the error that was previously impossible due to the bug.

[ykaravas]

I saw this line a few weeks ago and i was banging my head trying to figure out what i was missing!

[mszulcz-mitre]

@HalosGhost The unit and integration tests pass when I run them in Docker on my machine but are failing here. Here's the last few lines from the output:

/home/runner/work/opencbdc-tx/opencbdc-tx/tests/integration/atomizer_end_to_end_test.cpp:58: Failure
Value of: m_ctl_watchtower->get_block_height() > 0
  Actual: false
Expected: true
scripts/test.sh: line 9:  8659 Aborted                 (core dumped) $PWD/$1
Error: Process completed with exit code 134.

This appears to be the same problem that @davebryson encountered in PR #110. In your response to the problem in that PR, you mentioned that you ran the tests again and they passed. Could you please give that a shot?

[HalosGhost]

@mszulcz-mitre I pulled the changes locally and needed to add the below patch to get the code to compile; however, once I did everything built and tested fine. I cannot imagine this is what was causing the issue for CI (if so, what a weird way for it to fail); but please go ahead and merge in these missing includes. Regardless, after that's merged, I'll dive in and we'll see how the CI feels about it.

diff --git a/src/uhs/twophase/coordinator/interface.hpp b/src/uhs/twophase/coordinator/interface.hpp
index 291cf2be..79a5276f 100644
--- a/src/uhs/twophase/coordinator/interface.hpp
+++ b/src/uhs/twophase/coordinator/interface.hpp
@@ -6,6 +6,8 @@
 #ifndef OPENCBDC_TX_SRC_COORDINATOR_INTERFACE_H_
 #define OPENCBDC_TX_SRC_COORDINATOR_INTERFACE_H_
 
+#include <functional>
+
 #include "uhs/transaction/transaction.hpp"
 
 namespace cbdc::coordinator {
diff --git a/src/util/common/logging.hpp b/src/util/common/logging.hpp
index 8b12f376..ff1dcc0c 100644
--- a/src/util/common/logging.hpp
+++ b/src/util/common/logging.hpp
@@ -13,6 +13,7 @@
 #include <mutex>
 #include <optional>
 #include <sstream>
+#include <memory>
 
 namespace cbdc::logging {
     /// No-op stream destination for log output.

[mszulcz-mitre]

@HalosGhost Thanks for doing that!

I'm surprised the patches were needed to build the code. Did I mess something up? The code without them built on my machine and it seems to have passed the GitHub build check without them too. And none of my commits touched the patched files.

At any rate, I'm more than happy to add the patches. When I do, though, running scripts/lint.sh raises errors about them:

mszulczewski@sa-dev:~/Code/OpenCBDC/opencbdc-tx$ ./scripts/lint.sh 
Linting...
src/uhs/twophase/coordinator/interface.hpp:9:1: error: code should be clang-formatted [-Wclang-format-violations]
#include <functional>
^
src/util/common/logging.hpp:8:1: error: code should be clang-formatted [-Wclang-format-violations]
#include <chrono>
^

I copied and pasted the patches from your message, but to confirm that nothing got messed up, here's my output from git diff. Aside from the lines that start with 'index' , it matches your git diff output exactly (I diff'ed the diffs).

mszulczewski@sa-dev:~/Code/OpenCBDC/opencbdc-tx$ git diff
diff --git a/src/uhs/twophase/coordinator/interface.hpp b/src/uhs/twophase/coordinator/interface.hpp
index 291cf2b..79a5276 100644
--- a/src/uhs/twophase/coordinator/interface.hpp
+++ b/src/uhs/twophase/coordinator/interface.hpp
@@ -6,6 +6,8 @@
 #ifndef OPENCBDC_TX_SRC_COORDINATOR_INTERFACE_H_
 #define OPENCBDC_TX_SRC_COORDINATOR_INTERFACE_H_
 
+#include <functional>
+
 #include "uhs/transaction/transaction.hpp"
 
 namespace cbdc::coordinator {
diff --git a/src/util/common/logging.hpp b/src/util/common/logging.hpp
index 8b12f37..ff1dcc0 100644
--- a/src/util/common/logging.hpp
+++ b/src/util/common/logging.hpp
@@ -13,6 +13,7 @@
 #include <mutex>
 #include <optional>
 #include <sstream>
+#include <memory>
 
 namespace cbdc::logging {
     /// No-op stream destination for log output.

I've been scratching my head about the format errors, but I can't figure out the problem. Any thoughts?

[HalosGhost]

@mszulcz-mitre this is one of those odd things about compilation environments that I genuinely don't understand. It seems that my local environment is a little more picky (in some circumstances) about #include directives than others. Additionally, I made a mistake and added those two includes, but didn't clang-format them; if you run clang-format on those files, that will fix whatever formatting issue (probably include ordering) it's seeing.

As for whether you did anything wrong, I seriously doubt it; just a compiler idiosyncrasy.

[HalosGhost]

@mszulcz-mitre I think we're all good to merge. Can you squash to a single commit, and I'll merge it.

[HalosGhost]

@mszulcz-mitre sorry to have gotten back to this quite a while later, but can you please modify the commit subject line to be a little more clear? (“Fix bug” doesn't really convey anything when people are looking through the commit list.)

[HalosGhost]

@mszulcz-mitre I do think this is ready-to-merge, but I'd much prefer a more descriptive commit heading than “Fix bug”. Would you mind updating that message when you get the chance? (e.g., “Allow tcp_client::init to fail correctly”)

[mszulcz-mitre]

@HalosGhost No problem! I updated the commit to use your more descriptive commit message.

[HalosGhost]

All set and ready. Merging.