More informative error messages for invalid txids (#6359)

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [5.0.2]
+
+[5.0.2]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.2
+
+### Bug Fixes
+
+- The `/tx` endpoint returns more accurate error messages for incorrectly formed transactions ids (#6359).
+
 ## [5.0.1]
 
 [5.0.1]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.1

include/ccf/tx_id.h

@@ -42,8 +42,8 @@ namespace ccf
   // transaction executed by CCF.
   struct TxID
   {
-    View view;
-    SeqNo seqno;
+    View view = VIEW_UNKNOWN;
+    SeqNo seqno = SEQNO_UNKNOWN;
 
     std::string to_str() const
     {
@@ -64,7 +64,8 @@ namespace ccf
         const auto view_sv = sv.substr(0, separator_idx);
         const auto [p, ec] =
           std::from_chars(view_sv.begin(), view_sv.end(), tx_id.view);
-        if (ec != std::errc() || p != view_sv.end())
+        if (
+          ec != std::errc() || p != view_sv.end() || tx_id.view == VIEW_UNKNOWN)
         {
           return std::nullopt;
         }
@@ -74,7 +75,9 @@ namespace ccf
         const auto seqno_sv = sv.substr(separator_idx + 1);
         const auto [p, ec] =
           std::from_chars(seqno_sv.begin(), seqno_sv.end(), tx_id.seqno);
-        if (ec != std::errc() || p != seqno_sv.end())
+        if (
+          ec != std::errc() || p != seqno_sv.end() ||
+          tx_id.seqno == SEQNO_UNKNOWN)
         {
           return std::nullopt;
         }

src/node/rpc/test/frontend_test.cpp

@@ -485,7 +485,8 @@ void prepare_callers(NetworkState& network)
 
   init_network(network);
 
-  InternalTablesAccess::create_service(tx, network.identity->cert, ccf::TxID{});
+  InternalTablesAccess::create_service(
+    tx, network.identity->cert, ccf::TxID{2, 1});
   user_id = InternalTablesAccess::add_user(tx, {user_caller});
   member_id = InternalTablesAccess::add_member(tx, member_cert);
   invalid_member_id = InternalTablesAccess::add_member(tx, invalid_caller);

src/node/rpc/test/node_frontend_test.cpp

@@ -98,7 +98,7 @@ TEST_CASE("Add a node to an opening service")
   }
 
   InternalTablesAccess::create_service(
-    gen_tx, network.identity->cert, ccf::TxID{});
+    gen_tx, network.identity->cert, ccf::TxID{2, 1});
   REQUIRE(gen_tx.commit() == ccf::kv::CommitResult::SUCCESS);
   auto tx = network.tables->create_tx();
 
@@ -199,7 +199,7 @@ TEST_CASE("Add a node to an open service")
     up_to_ledger_secret_seqno, make_ledger_secret());
 
   InternalTablesAccess::create_service(
-    gen_tx, network.identity->cert, ccf::TxID{});
+    gen_tx, network.identity->cert, ccf::TxID{2, 1});
   InternalTablesAccess::init_configuration(gen_tx, {1});
   InternalTablesAccess::activate_member(
     gen_tx,

tests/e2e_logging.py

@@ -273,6 +273,35 @@ def send_corrupt_variations(content):
     return network
 
 
+@reqs.description("Test invalid transactions ids in /tx endpoint")
+def test_invalid_txids(network, args):
+    primary, _ = network.find_primary()
+
+    # These are not valid transactions IDs at all, one cannot ask about their status
+    invalid_params = ["a.b", "-1.-1", "0.0", "1.0", "0.1", "2.0"]
+
+    with primary.client() as c:
+        for txid in invalid_params:
+            r = c.get(f"/tx?transaction_id={txid}")
+            assert r.status_code == http.HTTPStatus.BAD_REQUEST, r.status_code
+            assert (
+                r.body.json()["error"]["code"] == "InvalidQueryParameterValue"
+            ), r.body.json()
+
+    # These are valid transaction IDs, but cannot happen in CCF because views start
+    # at 2, so while it is ok to ask about them, their consensus status is always Invalid,
+    # meaning that they are not, and can never be committed.
+    invalid_txs = ["1.1", "1.2"]
+
+    with primary.client() as c:
+        for txid in invalid_txs:
+            r = c.get(f"/tx?transaction_id={txid}")
+            assert r.status_code == http.HTTPStatus.OK, r.status_code
+            assert r.body.json()["status"] == "Invalid", r.body.json()
+
+    return network
+
+
 @reqs.description("Alternative protocols")
 @reqs.supports_methods("/log/private", "/log/public")
 @reqs.at_least_n_nodes(2)
@@ -1303,7 +1332,7 @@ def test_view_history(network, args):
             seqno_to_views = {}
             for seqno in range(1, commit_tx_id.seqno + 1):
                 views = []
-                for view in range(1, commit_tx_id.view + 1):
+                for view in range(2, commit_tx_id.view + 1):
                     r = c.get(f"/node/tx?transaction_id={view}.{seqno}", log_capture=[])
                     check(r)
                     status = TxStatus(r.body.json()["status"])
@@ -2056,6 +2085,7 @@ def run_parsing_errors(args):
 
         test_illegal(network, args)
         test_protocols(network, args)
+        test_invalid_txids(network, args)
 
 
 if __name__ == "__main__":