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

MST: fixed expired incoming state logic #2211

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
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
7 changes: 5 additions & 2 deletions irohad/multi_sig_transactions/impl/mst_processor_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,12 @@ namespace iroha {
// -------------------| MstTransportNotification override |-------------------

void FairMstProcessor::onNewState(const shared_model::crypto::PublicKey &from,
ConstRefState new_state) {
MstState new_state) {
log_->info("Applying new state");
auto current_time = time_provider_->getCurrentTime();

// no need to add already expired batches to local state
new_state.eraseExpired(current_time);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is could be an explanation of why we throw batches from new_state

auto state_update = storage_->apply(from, new_state);

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems, there is missing expiredBatchesNotify invocation.

// updated batches
Expand All @@ -101,7 +103,8 @@ namespace iroha {
completedBatchesNotify(*state_update.completed_state_);

// expired batches
expiredBatchesNotify(storage_->getDiffState(from, current_time));
// not nesessary to do it right here, just use the occasion to clean storage
expiredBatchesNotify(storage_->extractExpiredTransactions(current_time));
}

// -----------------------------| private api |-----------------------------
Expand Down
2 changes: 1 addition & 1 deletion irohad/multi_sig_transactions/mst_processor_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ namespace iroha {
// ------------------| MstTransportNotification override |------------------

void onNewState(const shared_model::crypto::PublicKey &from,
ConstRefState new_state) override;
MstState new_state) override;

// ----------------------------| end override |-----------------------------

Expand Down
5 changes: 2 additions & 3 deletions irohad/network/mst_transport.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ namespace iroha {
* @param from - key of the peer emitted the state
* @param new_state - state propagated from peer
*/
virtual void onNewState(
const shared_model::crypto::PublicKey &from,
const MstState &new_state) = 0;
virtual void onNewState(const shared_model::crypto::PublicKey &from,
MstState new_state) = 0;

virtual ~MstTransportNotification() = default;
};
Expand Down
2 changes: 1 addition & 1 deletion test/module/irohad/multi_sig_transactions/mst_mocks.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace iroha {
public:
MOCK_METHOD2(onNewState,
void(const shared_model::crypto::PublicKey &from,
const MstState &state));
MstState state));
};

/**
Expand Down
27 changes: 27 additions & 0 deletions test/module/irohad/multi_sig_transactions/mst_processor_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -319,3 +319,30 @@ TEST_F(MstProcessorTest, emptyStatePropagation) {
another_peer};
propagation_subject.get_subscriber().on_next(peers);
}

/**
* @given initialized mst processor with empty state
*
* @when received other peer's state containing an outdated batch
*
* @then check that transport was not invoked
* @and queues are not pushed to
* @and the batch does not get into our state
*/
TEST_F(MstProcessorTest, receivedOutdatedState) {
// ---------------------------------| then |----------------------------------
EXPECT_CALL(*transport, sendState(_, _)).Times(0);
auto observers = initObservers(mst_processor, 0, 0, 0);

// ---------------------------------| when |----------------------------------
shared_model::crypto::PublicKey another_peer_key("another_pubkey");
auto transported_state = MstState::empty(getTestLogger("MstState"),
std::make_shared<TestCompleter>());
const auto expired_batch = makeTestBatch(txBuilder(1, time_before, 3));
transported_state += addSignaturesFromKeyPairs(expired_batch, 0, makeKey());
mst_processor->onNewState(another_peer_key, transported_state);

// ---------------------------------| then |----------------------------------
EXPECT_FALSE(storage->batchInStorage(expired_batch));
check(observers);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there could be another test which checks that expired transactions from new_state will be not propagated by the expired notifier.