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

[fix] #2232: Make Iroha print meaningful message when genesis has too many isi #2239

Merged
merged 6 commits into from
May 20, 2022

Conversation

Arjentix
Copy link
Contributor

Signed-off-by: Daniil Polyakov arjentix@gmail.com

Description of the Change

  • Added error message when can't process genesis transaction
  • Logger initialization now starts eralier to be able to capture genesis creating logs

Issue

Benefits

Now users will get more meaningful message when they have too many instructions inside transactions

Possible Drawbacks

None

Usage Examples or Tests

You can take a big genesis from issue description and run Iroha with it to check this out.

I don't think I can write a test for it. This is UI-specific problem

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label May 19, 2022
@appetrosyan appetrosyan added Enhancement New feature or request UI Something about the interface labels May 20, 2022
@@ -96,11 +96,15 @@ where
/// To make `Iroha` peer work all actors should be started first.
/// After that moment it you can start it with listening to torii events.
///
/// # Side effect
Copy link
Contributor

Choose a reason for hiding this comment

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

Good practice.

cli/src/lib.rs Outdated Show resolved Hide resolved
rt.block_on(peer.start_with_config_permissions_dir(
configuration.clone(),
GenesisNetwork::test(true),
AllowAll,
Copy link
Contributor

Choose a reason for hiding this comment

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

#2199

Refers to this. The permissions need to be specified inside the genesis block, not hard-coded in the init process.

core/src/genesis.rs Outdated Show resolved Hide resolved
core/src/sumeragi/fault.rs Outdated Show resolved Hide resolved
data_model/src/transaction.rs Outdated Show resolved Hide resolved
data_model/src/transaction.rs Outdated Show resolved Hide resolved
@appetrosyan
Copy link
Contributor

Need to update the tests too
https://github.com/hyperledger/iroha/runs/6514631883?check_suite_focus=true#step:5:186

@Arjentix Arjentix force-pushed the fix_genesis_too_many_isi branch 3 times, most recently from 24a7186 to 82567fc Compare May 20, 2022 11:07
@@ -213,7 +213,7 @@ struct QueryResponseTest {

#[allow(variant_size_differences)]
enum QueryResponseBody {
Ok(VersionedQueryResult),
Ok(Box<VersionedQueryResult>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After rebase clippy was warning me about very different variant sizes, so I have to Box it

Copy link
Contributor

Choose a reason for hiding this comment

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

consider removing allow.

@s8sato s8sato self-assigned this May 20, 2022
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #2239 (c206266) into iroha2-dev (64fb445) will increase coverage by 0.08%.
The diff coverage is 68.65%.

@@              Coverage Diff               @@
##           iroha2-dev    #2239      +/-   ##
==============================================
+ Coverage       63.74%   63.82%   +0.08%     
==============================================
  Files             127      127              
  Lines           23581    23592      +11     
==============================================
+ Hits            15031    15058      +27     
+ Misses           8550     8534      -16     
Impacted Files Coverage Δ
core/src/sumeragi/fault.rs 42.11% <0.00%> (-0.16%) ⬇️
cli/src/lib.rs 67.04% <14.28%> (-2.32%) ⬇️
data_model/src/transaction.rs 50.48% <52.38%> (-0.36%) ⬇️
core/src/genesis.rs 89.72% <75.00%> (-0.40%) ⬇️
cli/src/torii/tests.rs 94.17% <100.00%> (ø)
core/src/tx.rs 74.18% <100.00%> (+0.38%) ⬆️
core/test_network/src/lib.rs 46.74% <100.00%> (+5.68%) ⬆️
crypto/src/signature.rs 75.00% <0.00%> (-1.88%) ⬇️
actor/src/deadlock.rs 98.63% <0.00%> (-1.37%) ⬇️
tools/kura_inspector/src/print.rs 96.15% <0.00%> (-0.77%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64fb445...c206266. Read the comment docs.

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@Arjentix Arjentix merged commit 49407ff into hyperledger:iroha2-dev May 20, 2022
@Arjentix Arjentix deleted the fix_genesis_too_many_isi branch May 20, 2022 14:44
pesterev pushed a commit to pesterev/iroha that referenced this pull request May 25, 2022
…sis has too many isi (hyperledger#2239)

Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST UI Something about the interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants