Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Feature/rejected txs index #1846

Merged
merged 7 commits into from
Nov 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions irohad/ametsuchi/block_query.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <cmath>
#include <rxcpp/rx.hpp>

#include "ametsuchi/tx_cache_response.hpp"
#include "common/result.hpp"
#include "interfaces/iroha_internal/block.hpp"
#include "interfaces/transaction.hpp"
Expand Down Expand Up @@ -106,12 +107,14 @@ namespace iroha {
const shared_model::crypto::Hash &hash) = 0;

/**
* Synchronously checks whether transaction
* with given hash is present in any block
* @param hash - transaction hash
* @return true if transaction exists, false otherwise
* Synchronously checks whether transaction with given hash is present in
* any block
* @param hash - rejected transaction's hash
* @return TxCacheStatusType which returns status of transaction:
* Committed, Rejected or Missing
*/
virtual bool hasTxWithHash(const shared_model::crypto::Hash &hash) = 0;
virtual TxCacheStatusType checkTxPresence(
const shared_model::crypto::Hash &hash) = 0;

/**
* Get the top-most block
Expand Down
23 changes: 22 additions & 1 deletion irohad/ametsuchi/impl/postgres_block_index.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ namespace {
return (base % hash.hex() % height).str();
}

std::string makeRejectedTxHashIndex(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a bit different naming - storeRejectedTxHash

Otherwise make index sounds like CREATE INDEX index_name ON table_name (column1, column2, ...); will be executed under the hood :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. My intention was to keep the naming consisted with other methods here. I can add the task to provide better naming for that function

const shared_model::interface::types::HashType &rejected_tx_hash,
shared_model::interface::types::HeightType height) {
boost::format base(
"INSERT INTO height_by_rejected_hash(hash, height) VALUES ('%s', "
"'%s');");
return (base % rejected_tx_hash.hex() % height).str();
}

// make index account_id:height -> list of tx indexes
// (where tx is placed in the block)
std::string makeCreatorHeightIndex(
Expand Down Expand Up @@ -110,7 +119,8 @@ namespace iroha {
const shared_model::interface::Block &block) {
auto height = block.height();
auto indexed_txs = block.transactions() | boost::adaptors::indexed(0);
std::string index_query = std::accumulate(
auto rejected_txs_hashes = block.rejected_transactions_hashes();
std::string tx_index_query = std::accumulate(
indexed_txs.begin(),
indexed_txs.end(),
std::string{},
Expand All @@ -125,6 +135,17 @@ namespace iroha {
query += makeCreatorHeightIndex(creator_id, height, index);
return query;
});

std::string rejected_tx_index_query = std::accumulate(
rejected_txs_hashes.begin(),
rejected_txs_hashes.end(),
std::string{},
[height](auto query, const auto &rejected_tx_hash) {
query += makeRejectedTxHashIndex(rejected_tx_hash, height);
return query;
});

auto index_query = tx_index_query + rejected_tx_index_query;
try {
sql_ << index_query;
} catch (const std::exception &e) {
Expand Down
29 changes: 28 additions & 1 deletion irohad/ametsuchi/impl/postgres_block_query.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,11 +242,38 @@ namespace iroha {
};
}

bool PostgresBlockQuery::hasTxWithHash(
TxCacheStatusType PostgresBlockQuery::checkTxPresence(
const shared_model::crypto::Hash &hash) {
if (hasCommittedTxWithHash(hash)) {
return tx_cache_status_responses::Committed();
}
if (hasRejectedTxWithHash(hash)) {
return tx_cache_status_responses::Rejected();
}
return tx_cache_status_responses::Missing();
}

bool PostgresBlockQuery::hasCommittedTxWithHash(
const shared_model::crypto::Hash &hash) {
return getBlockId(hash) != boost::none;
}

bool PostgresBlockQuery::hasRejectedTxWithHash(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we separate hashes storage on two methods?
Why could we not store both kinds of hashes in the same table with different markers in a separate column?

something like:

+-----+----------------+--------------------+-----+
| ... | hash           | committed/rejected | ... |
+-----+----------------+--------------------+-----+
|     | 2ac7b67ca6b76c | committed          |     |
+-----+----------------+--------------------+-----+
|     | 89ac7b867a875a | rejected           |     |
+-----+----------------+--------------------+-----+

That approach will let us avoid looking up in two independent sources for the single transaction in the worst case.
Also, we will have only one method for a transaction look up.

const shared_model::crypto::Hash &hash) {
boost::optional<uint64_t> blockId = boost::none;
boost::optional<std::string> block_str;
const auto &hash_str = hash.hex();

sql_ << "SELECT height FROM height_by_rejected_hash WHERE hash = :hash",
soci::into(block_str), soci::use(hash_str);
if (block_str) {
blockId = std::stoull(block_str.get());
} else {
log_->info("No block with rejected transaction {}", hash.toString());
}
return (bool)blockId;
}

uint32_t PostgresBlockQuery::getTopBlockHeight() {
return block_store_.last_id();
}
Expand Down
8 changes: 6 additions & 2 deletions irohad/ametsuchi/impl/postgres_block_query.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ namespace iroha {

uint32_t getTopBlockHeight() override;

bool hasTxWithHash(const shared_model::crypto::Hash &hash) override;
TxCacheStatusType checkTxPresence(
const shared_model::crypto::Hash &hash) override;

expected::Result<wBlock, std::string> getTopBlock() override;

Expand All @@ -73,7 +74,6 @@ namespace iroha {
*/
std::vector<shared_model::interface::types::HeightType> getBlockIds(
const shared_model::interface::types::AccountIdType &account_id);

/**
* Returns block id which contains transaction with a given hash
* @param hash - hash of transaction
Expand Down Expand Up @@ -101,6 +101,10 @@ namespace iroha {
std::string>
getBlock(shared_model::interface::types::HeightType id) const;

bool hasCommittedTxWithHash(const shared_model::crypto::Hash &hash);

bool hasRejectedTxWithHash(const shared_model::crypto::Hash &hash);

std::unique_ptr<soci::session> psql_;
soci::session &sql_;

Expand Down
6 changes: 6 additions & 0 deletions irohad/ametsuchi/impl/storage_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,12 @@ CREATE TABLE IF NOT EXISTS height_by_hash (
hash varchar,
height text
);

CREATE TABLE IF NOT EXISTS height_by_rejected_hash (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add drop table please (somewhere)

hash varchar,
height text
);

CREATE TABLE IF NOT EXISTS height_by_account_set (
account_id text,
height text
Expand Down
25 changes: 14 additions & 11 deletions irohad/torii/impl/command_service_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,20 @@ namespace torii {
return cached.value();
}

const bool is_present = storage_->getBlockQuery()->hasTxWithHash(request);

if (is_present) {
std::shared_ptr<shared_model::interface::TransactionResponse> response =
status_factory_->makeCommitted(request, "");
cache_->addItem(request, response);
return response;
} else {
log_->warn("Asked non-existing tx: {}", request.hex());
return status_factory_->makeNotReceived(request, "");
}
return iroha::visit_in_place(
storage_->getBlockQuery()->checkTxPresence(request),
[this,
&request](const iroha::ametsuchi::tx_cache_status_responses::Missing &)
-> std::shared_ptr<shared_model::interface::TransactionResponse> {
log_->warn("Asked non-existing tx: {}", request.hex());
return status_factory_->makeNotReceived(request, "");
},
[this, &request](const auto &) {
std::shared_ptr<shared_model::interface::TransactionResponse>
response = status_factory_->makeCommitted(request, "");
cache_->addItem(request, response);
return response;
});
}

/**
Expand Down
3 changes: 2 additions & 1 deletion test/module/irohad/ametsuchi/ametsuchi_mocks.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,8 @@ namespace iroha {
shared_model::interface::types::HeightType));
MOCK_METHOD1(getTopBlocks, std::vector<BlockQuery::wBlock>(uint32_t));
MOCK_METHOD0(getTopBlock, expected::Result<wBlock, std::string>(void));
MOCK_METHOD1(hasTxWithHash, bool(const shared_model::crypto::Hash &hash));
MOCK_METHOD1(checkTxPresence,
TxCacheStatusType(const shared_model::crypto::Hash &));
MOCK_METHOD0(getTopBlockHeight, uint32_t(void));
};

Expand Down
36 changes: 28 additions & 8 deletions test/module/irohad/ametsuchi/block_query_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <boost/filesystem.hpp>
#include <boost/optional.hpp>
#include <framework/specified_visitor.hpp>

#include "ametsuchi/impl/postgres_block_index.hpp"
#include "ametsuchi/impl/postgres_block_query.hpp"
Expand Down Expand Up @@ -67,6 +68,8 @@ class BlockQueryTest : public AmetsuchiTest {
.transactions(
std::vector<shared_model::proto::Transaction>({txn1_1, txn1_2}))
.prevHash(shared_model::crypto::Hash(zero_string))
.rejectedTransactions(
std::vector<shared_model::crypto::Hash>{rejected_hash})
.build();

// First tx in block 1
Expand Down Expand Up @@ -110,6 +113,7 @@ class BlockQueryTest : public AmetsuchiTest {
std::string creator2 = "user2@test";
std::size_t blocks_total{0};
std::string zero_string = std::string(32, '0');
shared_model::crypto::Hash rejected_hash{"rejected_tx_hash"};
};

/**
Expand Down Expand Up @@ -347,24 +351,40 @@ TEST_F(BlockQueryTest, GetTop2Blocks) {

/**
* @given block store with preinserted blocks
* @when hasTxWithHash is invoked on existing transaction hash
* @then True is returned
* @when checkTxPresence is invoked on existing transaction hash
* @then Committed status is returned
*/
TEST_F(BlockQueryTest, HasTxWithExistingHash) {
for (const auto &hash : tx_hashes) {
EXPECT_TRUE(blocks->hasTxWithHash(hash));
ASSERT_NO_THROW(boost::apply_visitor(
framework::SpecifiedVisitor<tx_cache_status_responses::Committed>(),
blocks->checkTxPresence(hash)));
}
}

/**
* @given block store with preinserted blocks
* user1@test AND 1 tx created by user2@test
* @when hasTxWithHash is invoked on non-existing hash
* @then False is returned
* @when checkTxPresence is invoked on non-existing hash
* @then Missing status is returned
*/
TEST_F(BlockQueryTest, HasTxWithInvalidHash) {
shared_model::crypto::Hash invalid_tx_hash(zero_string);
EXPECT_FALSE(blocks->hasTxWithHash(invalid_tx_hash));
TEST_F(BlockQueryTest, HasTxWithMissingHash) {
shared_model::crypto::Hash missing_tx_hash(zero_string);
ASSERT_NO_THROW(boost::apply_visitor(
framework::SpecifiedVisitor<tx_cache_status_responses::Missing>(),
blocks->checkTxPresence(missing_tx_hash)));
}

/**
* @given block store with preinserted blocks containing rejected_hash1 in one
* of the block
* @when checkTxPresence is invoked on existing rejected hash
* @then Rejected is returned
*/
TEST_F(BlockQueryTest, HasTxWithRejectedHash) {
ASSERT_NO_THROW(boost::apply_visitor(
framework::SpecifiedVisitor<tx_cache_status_responses::Rejected>(),
blocks->checkTxPresence(rejected_hash)));
}

/**
Expand Down
19 changes: 10 additions & 9 deletions test/module/irohad/torii/torii_service_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,15 @@ TEST_F(ToriiServiceTest, CommandClient) {
* @then ensure those are not received
*/
TEST_F(ToriiServiceTest, StatusWhenTxWasNotReceivedBlocking) {
std::vector<shared_model::proto::Transaction> txs;
std::vector<shared_model::interface::types::HashType> tx_hashes;

ON_CALL(*block_query, checkTxPresence(_))
.WillByDefault(
Return(iroha::ametsuchi::tx_cache_status_responses::Missing()));

// create transactions, but do not send them
for (size_t i = 0; i < TimesToriiBlocking; ++i) {
auto tx = TestTransactionBuilder().creatorAccountId("accountA").build();
txs.push_back(tx);
tx_hashes.push_back(tx.hash());
tx_hashes.push_back(shared_model::crypto::Hash{std::to_string(i)});
}

// get statuses of unsent transactions
Expand Down Expand Up @@ -323,11 +324,11 @@ TEST_F(ToriiServiceTest, StatusWhenBlocking) {
std::make_shared<iroha::validation::VerifiedProposalAndErrors>();
validation_result->verified_proposal =
std::make_unique<shared_model::proto::Proposal>(
TestProposalBuilder()
.height(1)
.createdTime(iroha::time::now())
.transactions(txs)
.build());
TestProposalBuilder()
.height(1)
.createdTime(iroha::time::now())
.transactions(txs)
.build());
validation_result->rejected_transactions.emplace(
failed_tx_hash,
iroha::validation::CommandError{
Expand Down