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

ir/config: Fix unenforced morph.consensus.p2p.peers.min config default #2856

Merged
merged 1 commit into from
May 26, 2024

Conversation

cthulhu-rider
Copy link
Contributor

Originally posted by @evgeniiz321

following P2P config

p2p:
  listen:
    - localhost:59893

led to no consensus for 4x setup although it should have. Making it

p2p:
  listen:
    - localhost:59893
  peers:
    min: 2

fixed the problem. Thus, the declared default was not fulfilled

min: 1 # Optional minimum number of peers a node needs for normal operation. Defaults to consensus minimum
# of 'committee' size (ceil of 2/3N-1). Must not be greater than 2147483647. Note that consensus service
# won't start until at least 'min' number of peers are connected

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.65%. Comparing base (9a9a5d5) to head (3930c67).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2856      +/-   ##
==========================================
+ Coverage   23.63%   23.65%   +0.01%     
==========================================
  Files         770      770              
  Lines       44506    44510       +4     
==========================================
+ Hits        10519    10527       +8     
+ Misses      33132    33129       -3     
+ Partials      855      854       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

As declared in NeoFS IR configuration, min required number of peer
connections must default to ⌈2/3N-1⌉. Previously, IR app did not set
default value when `p2p` and/or `p2p.peers` sections were omitted, only
when `p2p.peers` is presented and `min` value is missing. This was
incorrect and also unresponsive to the admin: a forgotten value
setting had the effect of no consensus and no new blocks.

This fixes the behavior: now value is recalculated in any setting
without a field. Note that while explicitly specifying zero is
problematic in general, it does not default to preserve admin intent.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider cthulhu-rider force-pushed the bugfix/ir-config-default branch from 438171f to 3930c67 Compare May 24, 2024 00:19
@cthulhu-rider cthulhu-rider added bug Something isn't working neofs-ir Inner Ring node application issues labels May 24, 2024
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Checked that PR does what it says but do not know if some default values should be enforced in general.

@carpawell carpawell requested a review from AnnaShaleva May 24, 2024 14:13
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Yes, we need a safe default here. It was intended, but just failed to be correctly implemented.

@roman-khimov roman-khimov merged commit ea78a2d into master May 26, 2024
17 of 22 checks passed
@roman-khimov roman-khimov deleted the bugfix/ir-config-default branch May 26, 2024 07:41
@roman-khimov roman-khimov added this to the v0.42.1 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working neofs-ir Inner Ring node application issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants