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

chore: disable store protocol by default #1374

Merged
merged 2 commits into from
Nov 14, 2022
Merged

Conversation

jm-clius
Copy link
Contributor

@jm-clius jm-clius commented Nov 11, 2022

Opening this suggestion as a PR, because I believe it should be done before the release if we agree upon it.

Disables store protocol by default.

Reasoning

One of the most important considerations for configuration is to have good defaults. I believe the default for store (which now implies actually persisting historical messages) should be false:

  • the 95% use case for running a nwaku node is "run a relay-only node" - this should be the reasonable default
  • this use case should not preclude such nodes from querying store nodes as a client. This is now always the case, even with store disabled.
  • previously persisting messages would have been disabled by default (via the now deprecated persist-messages option)
  • operators who upgrade their relay nodes and previously didn't care about or configure store will now suddenly start storing messages unless they change their config, especially a problem for those on resource-restricted devices.

TL;DR

To work as it did before store should now by default be false. I think this is the most reasonable default use case. :D

@jm-clius jm-clius added this to the Release 0.13.0 milestone Nov 11, 2022
@status-im-auto
Copy link
Collaborator

status-im-auto commented Nov 11, 2022

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 922e663 #1 2022-11-11 15:20:59 ~16 min linux 📦bin
922e663 #1 2022-11-11 15:21:05 ~16 min macos 📄log
✔️ 922e663 #2 2022-11-12 11:49:58 ~13 min macos 📦bin
✔️ d749331 #3 2022-11-14 08:09:45 ~13 min macos 📦bin
✔️ d749331 #2 2022-11-14 08:10:53 ~15 min linux 📦bin

Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@LNSD
Copy link
Contributor

LNSD commented Nov 12, 2022

Do we explicitly set the store parameter to true in our fleet? We are good to merge this if we are explicitly specifying the store flag in the configuration, otherwise, we need to set it before merging.

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM, and as @LNSD mentioned, we should probably loop in @jakubgs as well

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm. Side comment, having a networkmonitor tool helps when making this kind of changes, since it allows us to measure the impact of this (i.e. decrease in number of peers providing store capability).

@jm-clius
Copy link
Contributor Author

Side comment, having a networkmonitor tool helps when making this kind of changes, since it allows us to measure the impact of this (i.e. decrease in number of peers providing store capability).

@alrevuelta good point. Will be interesting to see the results. From the already published results I think the number of store nodes will go down considerably, as we'll only be listing actual history-storing nodes from now on.

Do we explicitly set the store parameter to true in our fleet?

Luckily we do: cc @LNSD @rymnc

@jm-clius jm-clius merged commit 97eaa69 into master Nov 14, 2022
@jm-clius jm-clius deleted the chore/store-disable-default branch November 14, 2022 11:09
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