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.

Sign up for GitHub

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

Jump to bottom

refactor: Fix confusing iavl command message #13653

Closed
wants to merge 1 commit into from
Closed

refactor: Fix confusing iavl command message #13653

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ is performed. Note, when enabled, gRPC will also be automatically enabled.
cmd.Flags().Uint64(FlagStateSyncSnapshotInterval, 0, "State sync snapshot interval")
cmd.Flags().Uint32(FlagStateSyncSnapshotKeepRecent, 2, "State sync snapshot to keep")

cmd.Flags().Bool(FlagIAVLFastNode, true, "Enable fast node for IAVL tree")
cmd.Flags().Bool(FlagIAVLFastNode, true, "Disable fast node for IAVL tree, set to 'false' to enable")
Copy link
Collaborator

@yihuang yihuang Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
cmd.Flags().Bool(FlagIAVLFastNode, true, "Disable fast node for IAVL tree, set to 'false' to enable")
cmd.Flags().Bool(FlagIAVLDisableFastNode, false, "Disable fast node for IAVL tree, it's enabled by default")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably change the default value of IAVLDisableFastNode in DefaultConfig as well.

Copy link
Contributor Author

@williamchong williamchong Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IAVL fast node is disabled by default, so renaming to FlagIAVLDisableFastNode would do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

will need core team members to chime in, but I think it's supposed to be enabled by default, which is the case with main branch and 0.46.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default value should remains true since current (v0.45.10) default behavior is disable Fast Node.

https://github.com/cosmos/cosmos-sdk/blob/v0.45.10/simapp/simd/cmd/root.go#L279

baseapp.SetIAVLDisableFastNode(cast.ToBool(appOpts.Get(server.FlagIAVLFastNode)))

https://github.com/cosmos/cosmos-sdk/blob/v0.45.10/store/rootmulti/store.go#L41

const iavlDisablefastNodeDefault = true

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW this CLI flag seems missing in 0.46, but the config is still there

Copy link
Member

@julienrbrt julienrbrt Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is actually missing on main too. Could you set main as base branch, so then we can backport.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the default behavior is different for >= 0.46
0.46 enables Fast Node by default (i.e. disableFastNode == false)

Copy link
Member

@julienrbrt julienrbrt Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But the default behavior is different for >= 0.46
0.46 enables Fast Node by default (i.e. disableFastNode == false)

I don't see the issue:

0.45

cmd.Flags().Bool(FlagDisableIAVLFastNode, true, "Disable fast node for IAVL tree")

0.46+

cmd.Flags().Bool(FlagDisableIAVLFastNode, false, "Disable fast node for IAVL tree")

Not having the flag in version after 0.45 is a CLI breaking change, so I think it is better to still keep the flag (true and false keep the same meaning anyway). We don't need to precise the default value in the message, as cobra does that already.

In the PR backported to 0.45 we can just swap the false to true. This way we keep consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@julienrbrt #13656 Please check if I understand correctly, then we can close this PR and follow up in the new one


// add support for all Tendermint-specific command line options
tcmd.AddNodeFlags(cmd)
Expand Down