From bd4d029fb14ddd561689566bc8c9c5146b2618ba Mon Sep 17 00:00:00 2001 From: Harrm Date: Mon, 15 Jan 2024 11:39:47 +0300 Subject: [PATCH] Fix UnknownTransaction::Custom encoding (#1910) * Fix UnknownTransaction::Custom encoding Co-authored-by: Ruslan Tushov --- core/authorship/impl/block_builder_impl.cpp | 6 +- core/primitives/apply_result.hpp | 5 +- core/primitives/transaction_validity.cpp | 6 +- core/primitives/transaction_validity.hpp | 136 +++++++++++------- core/runtime/module_instance.hpp | 19 +-- core/runtime/runtime_api/impl/core.cpp | 3 +- core/runtime/runtime_api/impl/lru.hpp | 4 +- .../impl/transaction_pool_impl.cpp | 8 +- .../core/primitives/primitives_codec_test.cpp | 4 +- 9 files changed, 116 insertions(+), 75 deletions(-) diff --git a/core/authorship/impl/block_builder_impl.cpp b/core/authorship/impl/block_builder_impl.cpp index d0ecac4979..72a8fd929b 100644 --- a/core/authorship/impl/block_builder_impl.cpp +++ b/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(reason)); + static_cast(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(reason)); + static_cast(reason.kind)); return BlockBuilderError::EXTRINSIC_APPLICATION_FAILED; }); }); diff --git a/core/primitives/apply_result.hpp b/core/primitives/apply_result.hpp index 90bb8c0efb..f8be859f0d 100644 --- a/core/primitives/apply_result.hpp +++ b/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); diff --git a/core/primitives/transaction_validity.cpp b/core/primitives/transaction_validity.cpp index 43f8724041..62cbd101cc 100644 --- a/core/primitives/transaction_validity.cpp +++ b/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 " diff --git a/core/primitives/transaction_validity.hpp b/core/primitives/transaction_validity.hpp index dd7ce4d346..4dc027c53e 100644 --- a/core/primitives/transaction_validity.hpp +++ b/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 (v) - 1; + s << static_cast(v.kind) - 1; + if (v.kind == InvalidTransaction::Custom) { + s << v.custom_value; + } + return s; } template static_cast(InvalidTransaction::ExhaustsResources)) { - v = InvalidTransaction::Custom; - } else { - v = static_cast(value); + v.kind = static_cast(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 (v) - 1; + s << static_cast(v.kind - 1); + if (v == UnknownTransaction::Custom) { + s << v.custom_value; + } + return s; } template static_cast(UnknownTransaction::NoUnsignedValidator)) { - v = UnknownTransaction::Custom; - } else { - v = static_cast(value); + v.kind = static_cast(value); + + if (value == UnknownTransaction::Custom) { + s >> v.custom_value; } + return s; } @@ -200,5 +234,5 @@ namespace kagome::primitives { boost::variant; } // 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) diff --git a/core/runtime/module_instance.hpp b/core/runtime/module_instance.hpp index 4fe9819afa..2442514c15 100644 --- a/core/runtime/module_instance.hpp +++ b/core/runtime/module_instance.hpp @@ -43,7 +43,7 @@ namespace kagome::runtime { template static outcome::result 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 static outcome::result decodedCall( + [[maybe_unused]] std::string_view method_name, outcome::result &&result) { OUTCOME_TRY(value, result); if constexpr (std::is_void_v) { @@ -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(callExportFunction(ctx, name, args_buf)); + return decodedCall(name, callExportFunction(ctx, name, args_buf)); } virtual outcome::result> getGlobal( diff --git a/core/runtime/runtime_api/impl/core.cpp b/core/runtime/runtime_api/impl/core.cpp index 3b6e1577a7..9aadee08b7 100644 --- a/core/runtime/runtime_api/impl/core.cpp +++ b/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(raw_res); + return ModuleInstance::decodedCall( + "Core_version", raw_res); }); } diff --git a/core/runtime/runtime_api/impl/lru.hpp b/core/runtime/runtime_api/impl/lru.hpp index 926a2181d2..ee1a8c05bd 100644 --- a/core/runtime/runtime_api/impl/lru.hpp +++ b/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(raw)); + OUTCOME_TRY(r, ModuleInstance::decodedCall(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(raw)); + OUTCOME_TRY(r, ModuleInstance::decodedCall(name, raw)); return lru_.exclusiveAccess([&](typename decltype(lru_)::Type &lru_) { return lru_.put(key, std::move(r), raw); }); diff --git a/core/transaction_pool/impl/transaction_pool_impl.cpp b/core/transaction_pool/impl/transaction_pool_impl.cpp index 9cdab4a2c5..2616deef68 100644 --- a/core/transaction_pool/impl/transaction_pool_impl.cpp +++ b/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 { - return validity_error; + return validity_error.kind; + }, + [](const primitives::UnknownTransaction &validity_error) + -> outcome::result { + return validity_error.kind; }); }, [&](primitives::ValidTransaction &&v) diff --git a/test/core/primitives/primitives_codec_test.cpp b/test/core/primitives/primitives_codec_test.cpp index 05e9244cc9..d68740f01c 100644 --- a/test/core/primitives/primitives_codec_test.cpp +++ b/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(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(val)); ASSERT_EQ(decoded_validity, unknown);