Add Storage for Pending Transactions #1598

igor-egorov · 2018-07-25T11:22:19Z

Signed-off-by: Igor Egorov [email protected]

Description of the Change

The storage listens to MST events and keeps track of semi signed transactions or batches of transactions.
The storage is a backend for "getPendingTransactions" query.
It returns all the transactions or batches of transactions created by query originator (single transactions or transactions from returned batches require more signatories to be processed).

Benefits

Storage for pending transactions is implemented and covered by tests.

Possible Drawbacks

This PR cannot be compiled until corresponding changes will be made in MST.

Usage Examples or Tests

cmake -H. -Bbuild
cmake --build build --target pending_txs_storage_test
cd build
ctest -R pending_txs_storage_test --output-on-failure

Alternate Designs

Storage is not divided to interface and implementation since having just the implementation is acceptable in that case.

Potentially, internal structures can be replaced with tbb::concurrent_{vector,hash_map}. We use two maps as underlying structures and concurrent tbb structures would not solve the problem of simultaneous lock of both maps.

l4l · 2018-07-25T11:51:19Z

test/module/irohad/pending_txs_storage/CMakeLists.txt

+    ## todo remove schema target, igor-egorov, IR-1512
+    ## schema is included due to a dependency bug in shared_model_interfaces
+    schema
+    shared_model_interfaces


These two are going from pending_txs_storage target

l4l · 2018-07-25T11:53:54Z

irohad/pending_txs_storage/pending_txs_storage.cpp

+      const AccountIdType &accountId) const {
+    std::shared_lock<std::shared_timed_mutex> lock(mutex_);
+    auto creator_it = storage_.index.find(accountId);
+    if (storage_.index.end() != creator_it) {


could you revert the condition, so the code is more linirized, e.g:

if (storage_.index.end() == creator_it) { return {}; } ...

I prefer leaving it as is because of two reasons:

Such change will introduce style inconsistency with the following code (line 49)

For me, it is a matter of taste :)

I can apply the change if you insist on it.

kamilsa · 2018-08-14T07:20:10Z

irohad/pending_txs_storage/CMakeLists.txt

+#
+
+add_library(pending_txs_storage
+        impl/pending_txs_storage_impl.cpp


fix indentation

kamilsa · 2018-08-14T08:22:35Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+      BatchObservable preparedBatch,
+      BatchObservable expiredBatch) {
+    updated_batches_subscription_ =
+        updatedBatches.subscribe([this](const SharedState &updatedBatches) {


Confusing naming. updatedBatches variable already exists

Also use snake case for variables' name here and above

kamilsa · 2018-08-14T14:34:42Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+
+  PendingTransactionStorageImpl::SharedTxsCollectionType
+  PendingTransactionStorageImpl::getPendingTransactions(
+      const AccountIdType &accountId) const {


use snake for variables name

kamilsa · 2018-08-14T14:38:01Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+  void PendingTransactionStorageImpl::removeBatch(const SharedBatch &batch) {
+    auto creators = batchCreators(*batch);
+    auto hash = batch->reducedHash();
+    {


what is the point of making additional scope by parentheses?

Discovered that in this case an additional scope is not needed.

kamilsa · 2018-08-15T14:30:11Z

test/module/irohad/execution/query_execution_test.cpp

+      .Times(1);
+
+  validateAndExecute(query);
+}


add empty line at the end

Also, fix this issue in your IDE for automatic endlines

Better to have git hook or any another automated way of checking the presence of trailing newlines rather than automatic "newline completion".

kamilsa · 2018-08-15T14:47:08Z

test/module/irohad/pending_txs_storage/pending_txs_storage_fixture.cpp

+    : log_(logger::log("PendingTxsStorageTest")){};
+
+std::shared_ptr<PendingTxsStorageFixture::Batch>
+PendingTxsStorageFixture::generateSharedBatch(


please use batch helper's makeTestBatch method instead

You can use the same approach as from batch test https://github.com/hyperledger/iroha/blob/47c3bfa6c7f3244c76b08efebe819802c5cd1165/test/module/shared_model/backend_proto/proto_batch_test.cpp#L268

or put these functions to batch helper

Discussed offline. If the pr will be merged before the end of the week, then I will add a todo and fix that issue as a separate task.

UPD: now tests are using batch helpers

muratovv · 2018-08-17T08:34:00Z

irohad/pending_txs_storage/CMakeLists.txt

+
+target_link_libraries(pending_txs_storage
+    mst_state
+    ## todo remove schema target, igor-egorov, IR-1512


Seems, that this bug is already fixed in develop:https://github.com/hyperledger/iroha/blob/develop/shared_model/interfaces/CMakeLists.txt

muratovv · 2018-08-17T08:43:46Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+  }
+
+  PendingTransactionStorageImpl::SharedTxsCollectionType
+  PendingTransactionStorageImpl::getPendingTransactions(


The code in the function looks a bit complicated. Maybe refactor it with functional primitives or separate with different functions.

You are very welcome to propose the better solution.

muratovv · 2018-08-17T09:00:33Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+
+  std::set<PendingTransactionStorageImpl::AccountIdType>
+  PendingTransactionStorageImpl::batchCreators(
+      const TransactionBatch &batch) const {


Why isn't this function static?

There are no external users. Let's do it static function.

muratovv · 2018-08-17T09:01:27Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+    return creators;
+  }
+
+  void PendingTransactionStorageImpl::updatedBatchesHandler(


This function, also, seems a bit complicated. Maybe rework it?

You are very welcome to propose the better solution.

The code does nothing more except the required stuff.

muratovv · 2018-08-17T09:16:08Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+        });
+    prepared_batch_subscription_ =
+        preparedBatch.subscribe([this](const SharedBatch &preparedBatch) {
+          this->preparedBatchHandler(preparedBatch);


What is the point to call wrapper instead of removeBatch by itself?

The wrapper was removed.

Initially, it was not clear that it will contain the single function call.

muratovv · 2018-08-17T09:16:18Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.cpp

+        });
+    expired_batch_subscription_ =
+        expiredBatch.subscribe([this](const SharedBatch &expiredBatch) {
+          this->expiredBatchHandler(expiredBatch);


same as before

muratovv · 2018-08-17T09:21:02Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.hpp

+    std::set<AccountIdType> batchCreators(const TransactionBatch &batch) const;
+
+    /**
+     * Subscriptions on MST events


This comment looks related only to one subscription. I think better to use /// instead.
Or use it with this manner:

// ---------| Subscriptions on MST events |---------

Of course, better to wrap it by 80 symbols for clean-up. Also, you may request me for the python generator of those comments.

Will leave it as is. Subscriptions + comment are separated from the rest of code with empty lines. In my view, it is pretty clear that comment relates to all subscription objects. Even more, comment is written in plural form. Thus, everything here is ok.

I missed a bit with the comment, I imagine using // as in the example.

muratovv · 2018-08-17T09:24:49Z

irohad/pending_txs_storage/impl/pending_txs_storage_impl.hpp

+     */
+    mutable std::shared_timed_mutex mutex_;
+
+    struct {


This struct and purpose unobvious for me. Pls, add documentation for class(struct) and fields with explanation HOW it works.

muratovv · 2018-08-17T09:26:28Z

test/module/irohad/execution/query_execution_test.cpp

+      .Times(1);
+
+  validateAndExecute(query);
+}


Also, fix this issue in your IDE for automatic endlines

muratovv · 2018-08-17T09:30:52Z

test/module/irohad/pending_txs_storage/pending_txs_storage_test.cpp

+#include <rxcpp/rx.hpp>
+#include "module/irohad/pending_txs_storage/pending_txs_storage_fixture.hpp"
+
+TEST_F(PendingTxsStorageFixture, FixutureSelfCheck) {


Add given, when, then notation

Requested changes were applied

Signed-off-by: Igor Egorov <[email protected]>

Signed-off-by: Fedor Muratov <[email protected]>

igor-egorov · 2018-08-28T10:41:54Z

There is only one failed test - module_ordering_service_test. This pr will be merged without fixing it - we have a separate task for this https://soramitsu.atlassian.net/browse/IR-1659 .

Signed-off-by: Igor Egorov <[email protected]>

igor-egorov requested review from muratovv and kamilsa July 25, 2018 11:22

igor-egorov added needs-review pr awaits review from maintainers needs-correction pr/rfc is not completed and might be updated mst multisignature transactions labels Jul 25, 2018

l4l reviewed Jul 25, 2018

View reviewed changes

l4l force-pushed the develop branch 8 times, most recently from 4750870 to b2a5906 Compare July 25, 2018 17:15

igor-egorov force-pushed the feature/pending-txs-storage branch from 8c19ddb to fe9471b Compare July 26, 2018 15:24

igor-egorov force-pushed the feature/pending-txs-storage branch from fe9471b to 80a982d Compare August 7, 2018 06:07

igor-egorov changed the base branch from develop to feature/mst_batches August 7, 2018 06:08

igor-egorov force-pushed the feature/pending-txs-storage branch 2 times, most recently from 3469dd9 to 0dd422b Compare August 14, 2018 07:10

kamilsa previously requested changes Aug 15, 2018

View reviewed changes

igor-egorov force-pushed the feature/pending-txs-storage branch from 0dd422b to ad2e2df Compare August 15, 2018 15:12

muratovv previously requested changes Aug 17, 2018

View reviewed changes

igor-egorov requested review from kamilsa and muratovv August 20, 2018 11:51

kamilsa approved these changes Aug 21, 2018

View reviewed changes

muratovv approved these changes Aug 21, 2018

View reviewed changes

l4l removed the needs-correction pr/rfc is not completed and might be updated label Aug 27, 2018

l4l removed the needs-review pr awaits review from maintainers label Aug 27, 2018

muratovv closed this Aug 27, 2018

muratovv changed the base branch from feature/mst_batches to dev August 28, 2018 06:20

muratovv reopened this Aug 28, 2018

Igor Egorov added 9 commits August 28, 2018 09:38

Add Storage for Pending Transactions and Tests

9269aaa

Signed-off-by: Igor Egorov <[email protected]>

Prettify CMakeLists.txt for pending txs storage

1792206

Signed-off-by: Igor Egorov <[email protected]>

Add storage initialisation for pending txs

785b418

Signed-off-by: Igor Egorov <[email protected]>

Add pending transactions storage to query execution pipeline

aabf591

Signed-off-by: Igor Egorov <[email protected]>

Split pending txs storage on interface and implementation

0d7530e

Signed-off-by: Igor Egorov <[email protected]>

Add query execution test for pending txs storage

d94e94c

Signed-off-by: Igor Egorov <[email protected]>

Update branch, everything can be compiled and all the tests passing

1dc9f2b

Signed-off-by: Igor Egorov <[email protected]>

Fix review comments

e40fc0d

Signed-off-by: Igor Egorov <[email protected]>

Use mst batch helpers in tests for pending txs storage

fa91693

Signed-off-by: Igor Egorov <[email protected]>

muratovv force-pushed the feature/pending-txs-storage branch from c487745 to fa91693 Compare August 28, 2018 09:40

Fix rebase issues

2773bc1

Signed-off-by: Fedor Muratov <[email protected]>

igor-egorov merged commit a3dbd10 into dev Aug 28, 2018

igor-egorov deleted the feature/pending-txs-storage branch August 28, 2018 10:44

nickaleks pushed a commit that referenced this pull request Sep 10, 2018

Add Storage for Pending Transactions (#1598)

7cad928

Signed-off-by: Igor Egorov <[email protected]>

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

Add Storage for Pending Transactions (#1598)

8553d0b

Signed-off-by: Igor Egorov <[email protected]>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Storage for Pending Transactions #1598

Add Storage for Pending Transactions #1598

igor-egorov commented Jul 25, 2018 •

edited

Loading

l4l Jul 25, 2018

l4l Jul 25, 2018 •

edited

Loading

igor-egorov Jul 25, 2018

kamilsa Aug 14, 2018

kamilsa Aug 14, 2018

kamilsa Aug 14, 2018

kamilsa Aug 14, 2018

kamilsa Aug 14, 2018

igor-egorov Aug 17, 2018

kamilsa Aug 15, 2018

muratovv Aug 17, 2018

igor-egorov Aug 17, 2018

kamilsa Aug 15, 2018

kamilsa Aug 15, 2018

kamilsa Aug 15, 2018

igor-egorov Aug 17, 2018

igor-egorov Aug 20, 2018

muratovv Aug 17, 2018

muratovv Aug 17, 2018

igor-egorov Aug 17, 2018

muratovv Aug 17, 2018

igor-egorov Aug 17, 2018

muratovv Aug 17, 2018

igor-egorov Aug 17, 2018

muratovv Aug 17, 2018

igor-egorov Aug 17, 2018

muratovv Aug 17, 2018

muratovv Aug 17, 2018 •

edited

Loading

igor-egorov Aug 17, 2018

muratovv Aug 21, 2018

muratovv Aug 17, 2018

muratovv Aug 17, 2018

muratovv Aug 17, 2018

igor-egorov commented Aug 28, 2018

Add Storage for Pending Transactions #1598

Add Storage for Pending Transactions #1598

Conversation

igor-egorov commented Jul 25, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Alternate Designs

Choose a reason for hiding this comment

l4l Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-egorov commented Aug 28, 2018

igor-egorov commented Jul 25, 2018 •

edited

Loading

l4l Jul 25, 2018 •

edited

Loading

muratovv Aug 17, 2018 •

edited

Loading