Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix] #3627: Transaction atomicity through cloning #3636

Merged
merged 3 commits into from
Jun 26, 2023
Merged

[fix] #3627: Transaction atomicity through cloning #3636

merged 3 commits into from
Jun 26, 2023

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Jun 22, 2023 •
edited by appetrosyan
Loading

Description

Apply transaction on cloned wsv and replace original one in case of successful execution.

Linked issue

Partially closes #3627

Benefits

Transaction execution within block is atomic.

Drawbacks

Cloning wsv will be costly in case of large wsv.

However on the current benchmarks i don't see significant differences.

We should explore time/space tradeoffs for improved solution.
Consider implementation of transaction rollbacks.

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@Erigara Erigara added Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Jun 22, 2023
@Erigara Erigara self-assigned this Jun 22, 2023
appetrosyan
appetrosyan previously approved these changes Jun 22, 2023
View reviewed changes
core/src/block.rs Outdated Show resolved Hide resolved
core/src/block.rs Outdated Show resolved Hide resolved
@Erigara Erigara force-pushed the transaction_atomicity branch 2 times, most recently from 9a7d4ee to ec43083 Compare June 23, 2023 08:29
appetrosyan
appetrosyan previously approved these changes Jun 23, 2023
View reviewed changes
appetrosyan
appetrosyan reviewed Jun 23, 2023
View reviewed changes
core/src/block.rs Outdated Show resolved Hide resolved
core/src/block.rs Outdated Show resolved Hide resolved
core/src/block.rs Outdated Show resolved Hide resolved
core/src/block.rs Outdated Show resolved Hide resolved
appetrosyan
appetrosyan previously approved these changes Jun 26, 2023
View reviewed changes
@appetrosyan appetrosyan assigned QuentinI and unassigned SamHSmith Jun 26, 2023
SamHSmith
SamHSmith previously approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@SamHSmith SamHSmith left a comment

Choose a reason for hiding this comment

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

LGTM

QuentinI
QuentinI previously approved these changes Jun 26, 2023
View reviewed changes
core/src/sumeragi/mod.rs Show resolved Hide resolved
core/benches/validate_blocks/validate_blocks.rs Outdated Show resolved Hide resolved
core/benches/validate_blocks/validate_blocks.rs Show resolved Hide resolved
@Erigara Erigara dismissed stale reviews from SamHSmith and appetrosyan via 621e797 June 26, 2023 10:28
@Erigara Erigara force-pushed the transaction_atomicity branch 2 times, most recently from 621e797 to 1d9fab1 Compare June 26, 2023 10:37
appetrosyan
appetrosyan previously approved these changes Jun 26, 2023
View reviewed changes
SamHSmith
SamHSmith previously approved these changes Jun 26, 2023
View reviewed changes
@Erigara Erigara dismissed stale reviews from SamHSmith and appetrosyan via a01ac30 June 26, 2023 14:28
@Erigara
[refactor] hyperledger-iroha#3615: Add validate blocks benchmark
f61cd0e 
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara
[fix] hyperledger-iroha#3627: Transaction atomicity through cloning
d69bb8a 
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara
[fix] hyperledger-iroha#3639: Fix connected_peers_with_f_1_0_1
dcb733b 
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@Erigara Erigara merged commit eb4a876 into hyperledger-iroha:iroha2-dev Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuentinI QuentinI QuentinI approved these changes

@appetrosyan appetrosyan appetrosyan approved these changes

@SamHSmith SamHSmith SamHSmith approved these changes

@s8sato s8sato Awaiting requested review from s8sato

@mversic mversic Awaiting requested review from mversic

@Arjentix Arjentix Awaiting requested review from Arjentix

@outoftardis outoftardis Awaiting requested review from outoftardis

@ilchu ilchu Awaiting requested review from ilchu

@pesterev pesterev Awaiting requested review from pesterev

@0x009922 0x009922 Awaiting requested review from 0x009922

Assignees

@appetrosyan appetrosyan

@QuentinI QuentinI

@Erigara Erigara

Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Erigara @SamHSmith @appetrosyan @QuentinI