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

archive flag for turning off GC #2099

Merged
merged 2 commits into from
Feb 9, 2020
Merged

archive flag for turning off GC #2099

merged 2 commits into from
Feb 9, 2020

Conversation

Kouprin
Copy link
Member

@Kouprin Kouprin commented Feb 6, 2020

Fixes #2094.

@Kouprin Kouprin requested a review from ilblackdragon February 6, 2020 03:08
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #2099 into staging will decrease coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #2099      +/-   ##
===========================================
- Coverage    86.83%   86.82%   -0.02%     
===========================================
  Files          177      177              
  Lines        34123    34134      +11     
===========================================
+ Hits         29632    29636       +4     
- Misses        4491     4498       +7
Impacted Files Coverage Δ
test-utils/testlib/src/node/mod.rs 100% <ø> (ø) ⬆️
core/chain-configs/src/client_config.rs 98.36% <100%> (+0.02%) ⬆️
near/src/config.rs 95% <100%> (+0.04%) ⬆️
chain/client/src/client.rs 93.28% <50%> (ø) ⬆️
chain/client/src/sync.rs 86.03% <0%> (-1.28%) ⬇️
chain/client/src/view_client.rs 69.92% <0%> (-1.01%) ⬇️
core/primitives/src/block.rs 95.26% <0%> (-0.72%) ⬇️
chain/client/src/client_actor.rs 85.33% <0%> (-0.66%) ⬇️
core/primitives/src/transaction.rs 77.57% <0%> (-0.45%) ⬇️
runtime/runtime/src/balance_checker.rs 96.7% <0%> (-0.37%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fa00ac...b57a358. Read the comment docs.

Copy link
Member

@ilblackdragon ilblackdragon left a comment

Choose a reason for hiding this comment

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

Yes, this looks correct

chain/client/src/types.rs Outdated Show resolved Hide resolved
near/src/main.rs Outdated
@@ -78,6 +78,7 @@ fn main() {
.arg(Arg::with_name("network-addr").long("network-addr").help("Customize network listening address (useful for running multiple nodes on the same machine)").takes_value(true))
.arg(Arg::with_name("rpc-addr").long("rpc-addr").help("Customize RPC listening address (useful for running multiple nodes on the same machine)").takes_value(true))
.arg(Arg::with_name("telemetry-url").long("telemetry-url").help("Customize telemetry url").takes_value(true))
.arg(Arg::with_name("archive").long("archive").help("Keep old blocks in the storage (default false)").takes_value(true))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is takes_value=true, if it's a boolean?
Look at fast parameter above for the init on line 66.

Separately, no need to provide the default, it is common sense that if there's a flag, than omitting it is the reverse of its behavior (whomever added produce_empty_blocks above for some reason also chose to make it a string parameter, but booleans should be takes_value=false, and not specify default value in the docs).

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Except issue Alex asked for, looks good to me

@Kouprin
Copy link
Member Author

Kouprin commented Feb 7, 2020

I don't like the solution.
The problem is, if we at least once wrongly run archive node without flag, we will lose all the data forever. People do mistakes, and I believe this case will happen one day.
@ilblackdragon @ailisp how can we store the flag permanently until it's dropped manually?

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

I suggest we extend the idea of @Kouprin and @mfornet for the adversarial node:

We are going to have several types of nodes in our system:

  • regular node (the only node for which we will be distributing the docker image);
  • archival node;
  • debug/diagnostics node;
  • adversarial node.

To run any of the above nodes, except the regular one, the user needs to provide the following three things:

  • The code should be compiled with the feature flag that guards conditional compilation;
  • The node should be started with a special command line flag;
  • There should be an intent file next to the node.

For example, if we want to run adversarial node we should compile it with adversarial_you_will_be_slashed feature, then pass --adversarial_you_will_be_slashed flag and create adversarial_you_will_be_slashed.txt file before starting the node.

Pros:

  • Uniform treatment of various types of the node:
    • Adversarial node and debug/diagnostics node use conditional compilation anyway;
  • Conditional compilation allows having optional external dependencies that might be used by heavy versions of the node, like "archival node" and "debug/diagnostics node";
  • Triple opt-in makes it really hard for someone to launch the wrong node accidentally. There is no need to have a clean-up step where we yank adversarial code when merging into master;

WDYT @Kouprin , @SkidanovAlex , @mfornet ?

@Kouprin
Copy link
Member Author

Kouprin commented Feb 7, 2020

I suggest we extend the idea of @Kouprin and @mfornet for the adversarial node:

We are going to have several types of nodes in our system:

  • regular node (the only node for which we will be distributing the docker image);
  • archival node;
  • debug/diagnostics node;
  • adversarial node.

To run any of the above nodes, except the regular one, the user needs to provide the following three things:

  • The code should be compiled with the feature flag that guards conditional compilation;
  • The node should be started with a special command line flag;
  • There should be an intent file next to the node.

For example, if we want to run adversarial node we should compile it with adversarial_you_will_be_slashed feature, then pass --adversarial_you_will_be_slashed flag and create adversarial_you_will_be_slashed.txt file before starting the node.

Pros:

  • Uniform treatment of various types of the node:

    • Adversarial node and debug/diagnostics node use conditional compilation anyway;
  • Conditional compilation allows having optional external dependencies that might be used by heavy versions of the node, like "archival node" and "debug/diagnostics node";

  • Triple opt-in makes it really hard for someone to launch the wrong node accidentally. There is no need to have a clean-up step where we yank adversarial code when merging into master;

WDYT @Kouprin , @SkidanovAlex , @mfornet ?

Is it a separate issue, isn't it?

@Kouprin
Copy link
Member Author

Kouprin commented Feb 7, 2020

@nearmax I'm okay with accepting this as is now and creating a new issue for you idea.

@MaksymZavershynskyi
Copy link
Contributor

@Kouprin

Is it a separate issue, isn't it?

If we go with the above proposal then the code in this PR should be changed. Instead of threading pub archive: bool, flag everywhere we should use #[cfg(feature = "archival_node")] everywhere (which is btw better because threading requires increasing number of function arguments and struct fields).

@MaksymZavershynskyi
Copy link
Contributor

@nearmax I'm okay with accepting this as is now and creating a new issue for you idea.

Ok

@Kouprin
Copy link
Member Author

Kouprin commented Feb 7, 2020

@Kouprin

Is it a separate issue, isn't it?

If we go with the above proposal then the code in this PR should be changed. Instead of threading pub archive: bool, flag everywhere we should use #[cfg(feature = "archival_node")] everywhere (which is btw better because threading requires increasing number of function arguments and struct fields).

There are different issues. This one is exactly for creating ability of system to turn off GC. Your suggestion is about how to make up the way of node running. This is great, but it's still different.

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.

5 participants