Skip to content

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

Merged
mversic merged 1 commit intohyperledger-iroha:mainfrom
Erigara:dynamic_commit_time
Oct 8, 2024
Merged

feat(sumeragi): dynamic commit time based on view change index#4957
mversic merged 1 commit intohyperledger-iroha:mainfrom
Erigara:dynamic_commit_time

Conversation

@Erigara
Copy link
Copy Markdown
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
View reviewed changes
@mversic mversic force-pushed the dynamic_commit_time branch from f9b4271 to e105222 Compare August 19, 2024 12:30
s8sato
s8sato reviewed Aug 21, 2024
View reviewed changes
Comment thread core/src/sumeragi/main_loop.rs Outdated
Comment thread core/src/sumeragi/main_loop.rs
Comment thread core/src/sumeragi/main_loop.rs Outdated
Comment thread core/src/sumeragi/main_loop.rs
s8sato
s8sato previously approved these changes Sep 3, 2024
View reviewed changes
@Erigara Erigara force-pushed the dynamic_commit_time branch from e105222 to e63df38 Compare September 3, 2024 15:08
@mversic mversic force-pushed the dynamic_commit_time branch from e63df38 to 42d3276 Compare September 4, 2024 03:34
mversic
mversic requested changes Sep 4, 2024
View reviewed changes
Comment thread core/src/sumeragi/view_change.rs Outdated
mversic
mversic reviewed Sep 4, 2024
View reviewed changes
Comment thread core/src/sumeragi/main_loop.rs Outdated
@mversic
Copy link
Copy Markdown
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
Copy Markdown
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.

@Erigara Erigara dismissed stale reviews from s8sato and SamHSmith via 193c3a1 September 4, 2024 11:11
@Erigara Erigara force-pushed the dynamic_commit_time branch from 42d3276 to 193c3a1 Compare September 4, 2024 11:11
mversic
mversic reviewed Sep 4, 2024
View reviewed changes
Comment thread core/src/sumeragi/main_loop.rs
@mversic
Copy link
Copy Markdown
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
Copy Markdown
Contributor

mversic commented Oct 8, 2024

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

@Erigara Erigara force-pushed the dynamic_commit_time branch from 193c3a1 to 43dfeeb Compare October 8, 2024 09:15
@Erigara @mversic
feat(sumeragi): dynamic commit time based on view change index
220ef08 
Signed-off-by: Shanin Roman <shanin1000@yandex.ru>
@mversic mversic force-pushed the dynamic_commit_time branch from 43dfeeb to 220ef08 Compare October 8, 2024 14:46
@mversic mversic requested a review from SamHSmith October 8, 2024 14:46
@mversic mversic mentioned this pull request Oct 8, 2024
Open
@mversic mversic merged commit 90940a1 into hyperledger-iroha:main Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mversic mversic mversic approved these changes

@s8sato s8sato s8sato approved these changes

@outoftardis outoftardis Awaiting requested review from outoftardis outoftardis is a code owner

@0x009922 0x009922 Awaiting requested review from 0x009922 0x009922 is a code owner

@DCNick3 DCNick3 Awaiting requested review from DCNick3 DCNick3 is a code owner

@dima74 dima74 Awaiting requested review from dima74 dima74 is a code owner

@SamHSmith SamHSmith Awaiting requested review from SamHSmith

Assignees

@SamHSmith SamHSmith

@Erigara Erigara

@s8sato s8sato

Labels

Consensus This issue is related to the Sumeragi consensus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consensus should continue working in the presense of heavy transactions

4 participants

@Erigara @mversic @SamHSmith @s8sato