Fix UnknownTransaction::Custom encoding (#1910)

* Fix UnknownTransaction::Custom encoding

Co-authored-by: Ruslan Tushov <turuslan@users.noreply.github.com>

core/authorship/impl/block_builder_impl.cpp

@@ -71,7 +71,7 @@ namespace kagome::authorship {
               tx_error,
               [this, &extrinsic](
                   const primitives::InvalidTransaction &reason) -> return_type {
-                switch (reason) {
+                switch (reason.kind) {
                   case primitives::InvalidTransaction::ExhaustsResources:
                     return BlockBuilderError::EXHAUSTS_RESOURCES;
                   case primitives::InvalidTransaction::BadMandatory:
@@ -82,7 +82,7 @@ namespace kagome::authorship {
                         "Extrinsic {} cannot be applied and was not pushed to "
                         "the block. (InvalidTransaction response, code {})",
                         extrinsic.data.toHex().substr(0, 8),
-                        static_cast<uint8_t>(reason));
+                        static_cast<uint8_t>(reason.kind));
                     return BlockBuilderError::EXTRINSIC_APPLICATION_FAILED;
                 }
               },
@@ -92,7 +92,7 @@ namespace kagome::authorship {
                         "Extrinsic {} cannot be applied and was not pushed to "
                         "the block. (UnknownTransaction response, code {})",
                         extrinsic.data.toHex().substr(0, 8),
-                        static_cast<uint8_t>(reason));
+                        static_cast<uint8_t>(reason.kind));
                 return BlockBuilderError::EXTRINSIC_APPLICATION_FAILED;
               });
         });

core/primitives/apply_result.hpp

@@ -27,11 +27,10 @@ namespace kagome::primitives {
   namespace dispatch_error {
     /// Some unclassified error occurred.
     struct Other {
+      SCALE_TIE(1)
       std::string value;
     };
-    // string value is not currently encodes in rust implementation,
-    // thus we use empty coder
-    SCALE_EMPTY_CODER(Other);
+
     /// Failed to lookup some data.
     struct CannotLookup {};
     SCALE_EMPTY_CODER(CannotLookup);

core/primitives/transaction_validity.cpp

@@ -6,7 +6,7 @@
 
 #include "primitives/transaction_validity.hpp"
 
-OUTCOME_CPP_DEFINE_CATEGORY(kagome::primitives, InvalidTransaction, e) {
+OUTCOME_CPP_DEFINE_CATEGORY(kagome::primitives, InvalidTransaction::Kind, e) {
   using E = kagome::primitives::InvalidTransaction;
   switch (e) {
     case E::Call:
@@ -40,8 +40,8 @@ OUTCOME_CPP_DEFINE_CATEGORY(kagome::primitives, InvalidTransaction, e) {
   return "Unknown InvalidTransaction error";
 }
 
-OUTCOME_CPP_DEFINE_CATEGORY(kagome::primitives, UnknownTransaction, e) {
-  using E = kagome::primitives::UnknownTransaction;
+OUTCOME_CPP_DEFINE_CATEGORY(kagome::primitives, UnknownTransaction::Kind, e) {
+  using E = kagome::primitives::UnknownTransaction::Kind;
   switch (e) {
     case E::CannotLookup:
       return "Could not lookup some information that is required to validate "

core/primitives/transaction_validity.hpp

@@ -91,37 +91,51 @@ namespace kagome::primitives {
   };
 
   /// Transaction is invalid. Details are described by the error code.
-  enum class InvalidTransaction : uint8_t {
-    /// The call of the transaction is not expected.
-    Call = 1,
-    /// General error to do with the inability to pay some fees (e.g. account
-    /// balance too low).
-    Payment,
-    /// General error to do with the transaction not yet being valid (e.g. nonce
-    /// too high).
-    Future,
-    /// General error to do with the transaction being outdated (e.g. nonce too
-    /// low).
-    Stale,
-    /// General error to do with the transaction's proofs (e.g. signature).
-    BadProof,
-    /// The transaction birth block is ancient.
-    AncientBirthBlock,
-    /// The transaction would exhaust the resources of current block.
-    ///
-    /// The transaction might be valid, but there are not enough resources left
-    /// in the current block.
-    ExhaustsResources,
-    /// Any other custom invalid validity that is not covered by this enum.
-    Custom,
-    /// An extrinsic with a Mandatory dispatch resulted in Error. This is
-    /// indicative of either a malicious validator or a buggy
-    /// `provide_inherent`. In any case, it can result in dangerously overweight
-    /// blocks and therefore if found, invalidates the block.
-    BadMandatory,
-    /// A transaction with a mandatory dispatch. This is invalid; only inherent
-    /// extrinsics are allowed to have mandatory dispatches.
-    MandatoryDispatch,
+  struct InvalidTransaction {
+    enum Kind : uint8_t {
+      /// The call of the transaction is not expected.
+      Call = 1,
+      /// General error to do with the inability to pay some fees (e.g. account
+      /// balance too low).
+      Payment,
+      /// General error to do with the transaction not yet being valid (e.g.
+      /// nonce
+      /// too high).
+      Future,
+      /// General error to do with the transaction being outdated (e.g. nonce
+      /// too
+      /// low).
+      Stale,
+      /// General error to do with the transaction's proofs (e.g. signature).
+      BadProof,
+      /// The transaction birth block is ancient.
+      AncientBirthBlock,
+      /// The transaction would exhaust the resources of current block.
+      ///
+      /// The transaction might be valid, but there are not enough resources
+      /// left
+      /// in the current block.
+      ExhaustsResources,
+      /// Any other custom invalid validity that is not covered by this enum.
+      Custom,
+      /// An extrinsic with a Mandatory dispatch resulted in Error. This is
+      /// indicative of either a malicious validator or a buggy
+      /// `provide_inherent`. In any case, it can result in dangerously
+      /// overweight
+      /// blocks and therefore if found, invalidates the block.
+      BadMandatory,
+      /// A transaction with a mandatory dispatch. This is invalid; only
+      /// inherent
+      /// extrinsics are allowed to have mandatory dispatches.
+      MandatoryDispatch,
+    };
+    Kind kind;
+    uint8_t custom_value{};
+
+    bool operator==(const InvalidTransaction &other) const {
+      return kind == other.kind
+          && (kind != Kind::Custom || custom_value == other.custom_value);
+    }
   };
 
   template <class Stream,
@@ -130,7 +144,11 @@ namespace kagome::primitives {
     // -1 is needed for compatibility with Rust; indices of error codes start
     // from 0 there, while in kagome they must start from 1 because of
     // std::error_code policy
-    return s << static_cast<uint8_t>(v) - 1;
+    s << static_cast<uint8_t>(v.kind) - 1;
+    if (v.kind == InvalidTransaction::Custom) {
+      s << v.custom_value;
+    }
+    return s;
   }
 
   template <class Stream,
@@ -143,23 +161,34 @@ namespace kagome::primitives {
     // start from 0 there, while in kagome they must start from 1 because of
     // std::error_code policy
     value++;
-    if (value > static_cast<uint8_t>(InvalidTransaction::ExhaustsResources)) {
-      v = InvalidTransaction::Custom;
-    } else {
-      v = static_cast<InvalidTransaction>(value);
+    v.kind = static_cast<InvalidTransaction::Kind>(value);
+    if (v.kind == InvalidTransaction::Custom) {
+      s >> value;
+      v.custom_value = value;
     }
     return s;
   }
 
   /// An unknown transaction validity.
-  enum class UnknownTransaction : uint8_t {
-    /// Could not lookup some information that is required to validate the
-    /// transaction.
-    CannotLookup = 1,
-    /// No validator found for the given unsigned transaction.
-    NoUnsignedValidator,
-    /// Any other custom unknown validity that is not covered by this enum.
-    Custom
+  struct UnknownTransaction {
+    enum Kind : uint8_t {
+      /// Could not lookup some information that is required to validate the
+      /// transaction.
+      CannotLookup = 1,
+      /// No validator found for the given unsigned transaction.
+      NoUnsignedValidator,
+      /// Any other custom unknown validity that is not covered by this enum.
+      Custom
+    };
+
+    bool operator==(Kind kind) const {
+      return this->kind == kind;
+    }
+
+    bool operator==(const UnknownTransaction &) const = default;
+
+    Kind kind;
+    uint8_t custom_value{};
   };
 
   template <class Stream,
@@ -168,7 +197,11 @@ namespace kagome::primitives {
     // -1 is needed for compatibility with Rust; indices of error codes start
     // from 0 there, while in kagome they must start from 1 because of
     // std::error_code policy
-    return s << static_cast<uint8_t>(v) - 1;
+    s << static_cast<uint8_t>(v.kind - 1);
+    if (v == UnknownTransaction::Custom) {
+      s << v.custom_value;
+    }
+    return s;
   }
 
   template <class Stream,
@@ -181,11 +214,12 @@ namespace kagome::primitives {
     // start from 0 there, while in kagome they must start from 1 because of
     // std::error_code policy
     value++;
-    if (value > static_cast<uint8_t>(UnknownTransaction::NoUnsignedValidator)) {
-      v = UnknownTransaction::Custom;
-    } else {
-      v = static_cast<UnknownTransaction>(value);
+    v.kind = static_cast<UnknownTransaction::Kind>(value);
+
+    if (value == UnknownTransaction::Custom) {
+      s >> v.custom_value;
     }
+
     return s;
   }
 
@@ -200,5 +234,5 @@ namespace kagome::primitives {
       boost::variant<ValidTransaction, TransactionValidityError>;
 }  // namespace kagome::primitives
 
-OUTCOME_HPP_DECLARE_ERROR(kagome::primitives, InvalidTransaction)
-OUTCOME_HPP_DECLARE_ERROR(kagome::primitives, UnknownTransaction)
+OUTCOME_HPP_DECLARE_ERROR(kagome::primitives, InvalidTransaction::Kind)
+OUTCOME_HPP_DECLARE_ERROR(kagome::primitives, UnknownTransaction::Kind)

core/runtime/module_instance.hpp

@@ -43,7 +43,7 @@ namespace kagome::runtime {
     template <typename... Args>
     static outcome::result<common::Buffer> encodeArgs(const Args &...args) {
       if constexpr (sizeof...(args) > 0) {
-        return common::map_result(scale::encode(args...), [](auto&& vec) {
+        return common::map_result(scale::encode(args...), [](auto &&vec) {
           return common::Buffer{vec};
         });
       }
@@ -52,6 +52,7 @@ namespace kagome::runtime {
 
     template <typename Result>
     static outcome::result<Result> decodedCall(
+        [[maybe_unused]] std::string_view method_name,
         outcome::result<common::Buffer> &&result) {
       OUTCOME_TRY(value, result);
       if constexpr (std::is_void_v<Result>) {
@@ -64,12 +65,14 @@ namespace kagome::runtime {
           // Check whether the whole byte buffer was consumed
           if (s.hasMore(1)) {
             static auto logger = log::createLogger("Executor", "runtime");
-            SL_ERROR(logger,
-                     "Runtime API call result size exceeds the size of the "
-                     "type to initialize {} (read {}, total size {})",
-                     typeid(Result).name(),
-                     s.currentIndex(),
-                     s.span().size_bytes());
+            SL_ERROR(
+                logger,
+                "Runtime API call '{}' result size exceeds the size of the "
+                "type to initialize {} (read {}, total size {})",
+                method_name,
+                typeid(Result).name(),
+                s.currentIndex(),
+                s.span().size_bytes());
             return outcome::failure(ModuleInstance::Error::INVALID_CALL_RESULT);
           }
           return outcome::success(std::move(t));
@@ -103,7 +106,7 @@ namespace kagome::runtime {
                                                      std::string_view name,
                                                      const Args &...args) {
       OUTCOME_TRY(args_buf, encodeArgs(args...));
-      return decodedCall<Res>(callExportFunction(ctx, name, args_buf));
+      return decodedCall<Res>(name, callExportFunction(ctx, name, args_buf));
     }
 
     virtual outcome::result<std::optional<WasmValue>> getGlobal(

core/runtime/runtime_api/impl/core.cpp

@@ -26,7 +26,8 @@ namespace kagome::runtime {
           OUTCOME_TRY(
               raw_res,
               ctx.module_instance->callExportFunction(ctx, "Core_version", {}));
-          return ModuleInstance::decodedCall<primitives::Version>(raw_res);
+          return ModuleInstance::decodedCall<primitives::Version>(
+              "Core_version", raw_res);
         });
   }
 

core/runtime/runtime_api/impl/lru.hpp

@@ -38,7 +38,7 @@ namespace kagome::runtime {
       }
       OUTCOME_TRY(ctx, executor.ctx().ephemeralAt(block));
       OUTCOME_TRY(raw, ctx.module_instance->callExportFunction(ctx, name, {}));
-      OUTCOME_TRY(r, ModuleInstance::decodedCall<V>(raw));
+      OUTCOME_TRY(r, ModuleInstance::decodedCall<V>(name, raw));
       return lru_.exclusiveAccess([&](typename decltype(lru_)::Type &lru_) {
         return lru_.put(block, std::move(r), raw);
       });
@@ -92,7 +92,7 @@ namespace kagome::runtime {
       OUTCOME_TRY(raw_arg, ModuleInstance::encodeArgs(arg));
       OUTCOME_TRY(raw,
                   ctx.module_instance->callExportFunction(ctx, name, raw_arg));
-      OUTCOME_TRY(r, ModuleInstance::decodedCall<V>(raw));
+      OUTCOME_TRY(r, ModuleInstance::decodedCall<V>(name, raw));
       return lru_.exclusiveAccess([&](typename decltype(lru_)::Type &lru_) {
         return lru_.put(key, std::move(r), raw);
       });

core/transaction_pool/impl/transaction_pool_impl.cpp

@@ -81,9 +81,13 @@ namespace kagome::transaction_pool {
           return visit_in_place(
               e,
               // return either invalid or unknown validity error
-              [](const auto &validity_error)
+              [](const primitives::InvalidTransaction &validity_error)
                   -> outcome::result<primitives::Transaction> {
-                return validity_error;
+                return validity_error.kind;
+              },
+              [](const primitives::UnknownTransaction &validity_error)
+                  -> outcome::result<primitives::Transaction> {
+                return validity_error.kind;
               });
         },
         [&](primitives::ValidTransaction &&v)

test/core/primitives/primitives_codec_test.cpp

@@ -172,7 +172,7 @@ TEST_F(Primitives, EncodeBlockIdBlockNumberSuccess) {
  * @then obtained result matches predefined value
  */
 TEST_F(Primitives, EncodeTransactionValidityInvalidSuccess) {
-  InvalidTransaction invalid{1};
+  InvalidTransaction invalid{InvalidTransaction::Call};
   EXPECT_OUTCOME_TRUE(val, encode(invalid))
   EXPECT_OUTCOME_TRUE(decoded_validity, decode<InvalidTransaction>(val));
   ASSERT_EQ(decoded_validity, invalid);
@@ -184,7 +184,7 @@ TEST_F(Primitives, EncodeTransactionValidityInvalidSuccess) {
  * @then obtained result matches predefined value
  */
 TEST_F(Primitives, EncodeTransactionValidityUnknown) {
-  UnknownTransaction unknown{2};
+  UnknownTransaction unknown{UnknownTransaction::Kind::Custom, 42};
   EXPECT_OUTCOME_TRUE(val, encode(unknown))
   EXPECT_OUTCOME_TRUE(decoded_validity, decode<UnknownTransaction>(val));
   ASSERT_EQ(decoded_validity, unknown);