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

block.body definitly exists unless it's a light client or using fast sync in storage chain mode #153

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

liuchengxu
Copy link
Contributor

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Can we guarantee that it is impossible to run a light client node or use fast sync? Light client mode is not working/not available so that is fair I guess, but I'm not 100% sure what fast sync is.

Since we have customized struct for CLI arguments, maybe we should start stripping things down from it such that we don't allow configurations that are not working in the first place? There are quite a few arguments which are not applicable to Subspace in default flags.

@liuchengxu
Copy link
Contributor Author

The main idea of fast sync can be visited at paritytech/substrate#8884. I don't think it works in our case.

maybe we should start stripping things down from it such that we don't allow configurations that are not working in the first place

Yes, definitely possible, but it also has to be a quite dirty trick. I used it to support the CLI config file by first filtering the raw CLI argument list and then feeding them into the internal CLI constructor in ChainX but really not a fan of it. Based on my experience, basically, no one will run into this trap as long as it's not the default behavior and you don't show the way to do it. So we could save some energy for now. Can be another PR if you'd like to prune the flags incompatible with subspace seriously.

@liuchengxu liuchengxu force-pushed the fix-block-body-todo branch 2 times, most recently from fb9b8e1 to c4649e4 Compare November 26, 2021 03:13
@nazar-pc
Copy link
Member

I used it to support the CLI config file by first filtering the raw CLI argument list and then feeding them into the internal CLI constructor in ChainX but really not a fan of it

I think there is a better way:

We should be able to include some subcommands and skip others to get just the list we need.

@liuchengxu liuchengxu enabled auto-merge November 26, 2021 04:11
@liuchengxu
Copy link
Contributor Author

We should be able to include some subcommands and skip others to get just the list we need.

That works for the subcommands, but not for the inner params like NetworkParams in RunCmd, unless you reassemble or build a wrapper from the origin subcommand.

@nazar-pc
Copy link
Member

nazar-pc commented Nov 26, 2021

We should still be able to copy-paste those structs, customize them and map to whatever Substrate expects before moving forward. We just need to make sure that someone who is typing subspace-node --help only gets useful and working options.

@liuchengxu
Copy link
Contributor Author

Yeah, reassembling the subcommand can be less a dirty way.

@liuchengxu liuchengxu merged commit db4639f into main Nov 26, 2021
@liuchengxu liuchengxu deleted the fix-block-body-todo branch November 26, 2021 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants