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

feat(sumeragi): dynamic commit time based on view change index #4957

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Aug 9, 2024

Description

  • Commit time is now linear function of view change index (capped at Duration::MAX)
  • Add more logs to track messages arriving to sumeragi

Note: #4929 would also nice to have because it prevents degradation of view change updates.

Linked issue

Closes #4265

Benefits

This allow to consensus to pass heavy transactions which cause infinite loop previously.

How to test?

Currently it's not possible to modulary test consensus :(

So i've did my tests on bare metal iroha with 4 nodes.

Parameters:

config.toml:

[network]
transaction_gossip_period_ms = 10_000

[transaction]
time_to_live_ms = 3_600_000

genesis.json:

    {
      "Sumeragi": {
        "BlockTimeMs": 100
      }
    },
    {
      "Sumeragi": {
        "CommitTimeMs": 100
      }
    }

Notice that transaction_gossip_period_ms was crucial here, because without it i faced #4952.

Check this comment for the transaction i've submitted to iroha.

@Erigara Erigara added the Consensus This issue is related to the Sumeragi consensus label Aug 9, 2024
@Erigara Erigara self-assigned this Aug 9, 2024
@Erigara Erigara force-pushed the dynamic_commit_time branch 2 times, most recently from 6e8d9ff to f9b4271 Compare August 9, 2024 07:14
@Erigara Erigara marked this pull request as ready for review August 9, 2024 08:00
@SamHSmith SamHSmith self-assigned this Aug 12, 2024
SamHSmith
SamHSmith previously approved these changes Aug 15, 2024
core/src/sumeragi/main_loop.rs Outdated Show resolved Hide resolved
core/src/sumeragi/main_loop.rs Outdated Show resolved Hide resolved
core/src/sumeragi/main_loop.rs Outdated Show resolved Hide resolved
core/src/sumeragi/main_loop.rs Outdated Show resolved Hide resolved
s8sato
s8sato previously approved these changes Sep 3, 2024
core/src/sumeragi/view_change.rs Outdated Show resolved Hide resolved
@mversic
Copy link
Contributor

mversic commented Sep 4, 2024

this will increase pipeline_time even in the presence of malicious blocks (not just too complex blocks). How about if we add a shift so that first increase in pipeline time is after x first view changes? Thereafter the increase should happen on every view change

@Erigara
Copy link
Contributor Author

Erigara commented Sep 4, 2024

this will increase pipeline_time even in the presence of malicious blocks (not just too complex blocks). How about if we add a shift so that first increase in pipeline time is after x first view changes? Thereafter the increase should happen on every view change

Added shift parameter and made it max_faults + 1 by default, because we can have at most max_faults malicious blocks in a row.

@mversic
Copy link
Contributor

mversic commented Sep 5, 2024

I approve that the growth is linear with view_change_index. We don't want to have exponential growth because malicious peers can have larger impact on the network liveness in that case

@mversic mversic self-requested a review September 15, 2024 00:50
@mversic mversic requested a review from s8sato September 15, 2024 00:50
@mversic
Copy link
Contributor

mversic commented Oct 8, 2024

let's merge this and split that discussion into a separate ticket

Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@mversic mversic merged commit 90940a1 into hyperledger-iroha:main Oct 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Consensus This issue is related to the Sumeragi consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consensus should continue working in the presense of heavy transactions
4 participants