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

Replay check to ordering gate #1868

Merged

Conversation

nickaleks
Copy link
Contributor

Signed-off-by: Nikita Alekseev n.alekseev2104@gmail.com

Description of the Change

This PR adds a replay check to ordering gate via transaction cache

Benefits

No replays in ordering gate

Possible Drawbacks

None

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
Copy link
Contributor

@kamilsa kamilsa left a comment

Choose a reason for hiding this comment

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

Take a look to the comments

@@ -19,27 +21,35 @@ using namespace framework::test_subscriber;

using ::testing::_;
using ::testing::ByMove;
using testing::NiceMock;
Copy link
Contributor

Choose a reason for hiding this comment

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

please add :: for consistency

return true;
},
[](const auto &status) {
// TODO: log replayed transactions when log is added
Copy link
Contributor

Choose a reason for hiding this comment

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

Add IR number

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
Copy link
Contributor

@MBoldyrev MBoldyrev left a comment

Choose a reason for hiding this comment

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

looks good, but did not test it

std::unique_ptr<shared_model::interface::Proposal>
OnDemandOrderingGate::removeReplays(
const shared_model::interface::Proposal &proposal) const {
auto not_processed_txs = [this](const auto &tx) {
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 renaming this function to something like tx_is_not_processed, because a) it currently looks very similar to the variable below, unprocessed_txs, which is the result of applying this filter, and b) it handles single tx, so no "txs"

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
@@ -41,14 +44,12 @@ OnDemandOrderingGate::OnDemandOrderingGate(
// request proposal for the current round
auto proposal = network_client_->onRequestProposal(current_round_);

auto final_proposal = this->processProposalRequest(proposal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use std::move(proposal) instead? Because proposal should not be used after processing it

current_round_.block_round, current_round_.reject_round, {});
}
if (not *proposal) {
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

This branch will not execute, it is guaranteed by onRequestProposal interface. So it is better to remove it.

const shared_model::interface::Proposal &proposal) const {
auto not_processed_txs = [this](const auto &tx) {
auto tx_result = tx_cache_->check(tx.hash());
return iroha::visit_in_place(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if it creates a visitor struct on each call. If it does, it is better to replace this construction with boost::get.

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>
@l4l l4l added the ordering label Nov 23, 2018
@nickaleks nickaleks merged commit 1b3dfd6 into trunk/tx_persistent_cache Nov 26, 2018
@nickaleks nickaleks deleted the feature/ordering_gate_replay_prevention branch November 26, 2018 08:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants