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.

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

[refactor] #4229: Removed MST aggregation #4308

Merged

Conversation

Stukalov-A-M
Copy link
Contributor

@Stukalov-A-M Stukalov-A-M commented Feb 20, 2024

Description

  • Removed MST aggregation and derived methods
  • Removed hash_of_payload method for Transactions
  • Removed merge_signatures method
  • Removed pending_transactions endpoint and derived methods

Now not fully signed tx will be declined by queue with SignatureCondition error

Linked issue

Benefits

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

client_cli/src/main.rs Outdated Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented Feb 20, 2024

Pull Request Test Coverage Report for Build 8095893403

Details

  • 38 of 73 (52.05%) changed or added relevant lines in 8 files are covered.
  • 3596 unchanged lines in 60 files lost coverage.
  • Overall coverage decreased (-0.3%) to 56.533%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/block.rs 0 1 0.0%
core/src/sumeragi/main_loop.rs 0 1 0.0%
torii/src/lib.rs 0 1 0.0%
torii/src/routing.rs 0 1 0.0%
core/src/gossiper.rs 0 2 0.0%
core/src/queue.rs 36 41 87.8%
client_cli/src/main.rs 0 7 0.0%
client/src/client.rs 2 19 10.53%
Files with Coverage Reduction New Missed Lines %
core/src/sumeragi/network_topology.rs 1 98.78%
tools/kagami/src/crypto.rs 1 12.12%
data_model/src/metadata.rs 2 78.87%
primitives/src/unique_vec.rs 2 91.24%
config/src/kura.rs 3 80.0%
config/src/logger.rs 5 72.73%
logger/src/lib.rs 5 94.12%
tools/kagami/src/main.rs 5 0.0%
crypto/src/signature/secp256k1.rs 7 96.09%
data_model/src/smart_contract.rs 7 0.0%
Totals Coverage Status
Change from base Build 7884695009: -0.3%
Covered Lines: 22517
Relevant Lines: 39830

💛 - Coveralls

@Erigara Erigara self-assigned this Feb 20, 2024
@Stukalov-A-M Stukalov-A-M force-pushed the removed_mst_aggregation branch 2 times, most recently from 3baf01a to 65d86cc Compare February 20, 2024 21:17
@Stukalov-A-M
Copy link
Contributor Author

@AlexStroke , could you pls. look at client_cli python tests. I removed check_mst flag from args, and assume that cli tests are dropped because of this.

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Feb 21, 2024
client/tests/integration/multisignature_transaction.rs Outdated Show resolved Hide resolved
client/tests/integration/multisignature_transaction.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/tx.rs Show resolved Hide resolved
@Erigara
Copy link
Contributor

Erigara commented Feb 22, 2024

@Stukalov-A-M about pytests, that's right pytests use --skip-mst-check by default

VAmuzing
VAmuzing previously approved these changes Feb 27, 2024
@s8sato
Copy link
Contributor

s8sato commented Feb 27, 2024

It seems to me that with this change, Iroha will temporarily lose the feature of multi-signature transactions.
Is that okay for you all?
My concerns are noted here #4229 (comment)

@Stukalov-A-M
Copy link
Contributor Author

It seems to me that with this change, Iroha will temporarily lose the feature of multi-signature transactions. Is that okay for you all? My concerns are noted here #4229 (comment)

We have discussed it with @Arjentix exactly an idea to keep MST via triggers and it seems logical to divide the MST question resolving on 2 steps:

  1. Remove current MST realization
  2. Design a new solution for MST

core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
@Stukalov-A-M Stukalov-A-M force-pushed the removed_mst_aggregation branch 3 times, most recently from e516c0b to bc69f3e Compare February 29, 2024 11:22
@Stukalov-A-M Stukalov-A-M added the Refactor Improvement to overall code quality label Feb 29, 2024
Signed-off-by: Stukalov-A-M <stukalov07@gmail.com>
@Stukalov-A-M Stukalov-A-M merged commit 96bf997 into hyperledger:iroha2-dev Feb 29, 2024
12 of 13 checks passed
@Stukalov-A-M Stukalov-A-M deleted the removed_mst_aggregation branch February 29, 2024 12:55
@@ -646,7 +615,7 @@ mod tests {
query_handle,
));
let queue = Queue::from_config(Config {
transaction_time_to_live: Duration::from_millis(200),
transaction_time_to_live: Duration::from_millis(300),
Copy link
Contributor

@DCNick3 DCNick3 Mar 4, 2024

Choose a reason for hiding this comment

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

This change breaks the test for me, and I don't think this change can be right without changing the expected numbers of transactions collected

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, i fixed this test in #4319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries iroha2-dev The re-implementation of a BFT hyperledger in RUST Refactor Improvement to overall code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants