-
Notifications
You must be signed in to change notification settings - Fork 134
Rename fluffy to portal/nimbus_portal_client #3289
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
Conversation
"AppData" / "Roaming" / "NimbusPortalClient" | ||
elif defined(macosx): | ||
"Library" / "Application Support" / "NimbusPortalClient" | ||
else: | ||
".cache" / "nimbus-portal-client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the new data-dir naming here, now would be a good moment to get it "right". Or rather, get it consistent.
In order to be consistent, perhaps I should go with .cache/nimbus/portal
or .cache/nimbus/portal-client
, as
nimbus_beacon_node does something similar: https://github.com/status-im/nimbus-eth2/blob/stable/beacon_chain/conf.nim#L1139-L1146
However, nimbus_execution_client does just nimbus
: https://github.com/status-im/nimbus-eth1/blob/master/execution_chain/config.nim#L55-L61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think .cache/nimbus/portal
or .cache/nimbus/portal-client
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go with .cache/nimbus/portal-client
so it fits with .cache/nimbus/portal-bridge
.
@@ -165,7 +166,7 @@ type | |||
stateDir* {. | |||
desc: "The directory where the state data is stored", | |||
defaultValue: defaultPortalBridgeStateDir(), | |||
defaultValueDesc: defaultPortalBridgeStateDir(), | |||
defaultValueDesc: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if for this one we should stick with defaultDataDir
/ "Bridge" / "State", as opposed to for example "nimbus/portal-bridge". Basically the question if we should keep it under the same top level direction as nimbus_portal_client
or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I prefer nimbus/portal-bridge
or similar top level directory to keep the bridge data separate from the fluffy/portal-client data.
# Currently still keeping the fluffy name here. Change should first be made to | ||
# write the Unix epoch as seqNum in ENR and then this file no longer needs to | ||
# be read. | ||
const enrFileName = "fluffy_node.enr" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I can more easily fix in a second round after we add unix epoch timestamp as seqNum instead of just increment current number by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we just rename the file on startup if it exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I could, and I still can.
More (somewhat) recently some clients have started putting the unix epoch timestamp in there as sequence number, as that will always increase and fits the designated size for it.
Our way (and of many clients before) is to read out the old ENR file for the seqNum, and increase it on change + write back. With the timestamp you do not really need to read/write the old ENR. It would only be there for other applications/users to use basically. Potential downside is that ENRs will be passed along a little more frequent on discv5 handshake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Sounds like a reasonable idea. I did find it helpful to have the enr on disk when debugging and testing various things by the way but not a big deal because I believe we also log it at INFO level if I'm not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still keep it on disk. But it would just be written to at startup, and we don't read from it. But I'm not sure, we can also keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did change it to move the file to a name without fluffy. And did the same for the lock file to make sure it gets cleaned up.
portal/database/content_db.nim
Outdated
"working database (out of memory?)" | ||
) | ||
else: | ||
# TODO: unfortunate that this was called fluffy and not contentdb | ||
# How to deprecate this naming? | ||
SqStoreRef.init(path, "fluffy", manualCheckpoint = false).expectDb() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky one. I guess all I can do is check if the fluffy filename exists and if so keep it as fluffy
, else call it contentdb
. Similar to the approach taken for the general data-dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I believe we could just rename the sqlite db file from fluffy.sqlite3 to contentdb.sqlite3 on startup, if it exists. I haven't tested this but apparently it should work: https://www.slingacademy.com/article/how-to-rename-drop-an-sqlite-database/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point, that should be fine as it is slightly different than the data-dir
situation in the sense that a user cannot set it anyhow. So pretty safe to rename I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done that move in c3e2df6
For 'Step 3: Move documentation to nimbus-eth2 (and thus the nimbus.guide)', perhaps we should move the entire nimbus.guide website to a separate Github repo or if we truely want to make nimbus-eth1 our mono repo then move the entire website into Nimbus-Eth1 into a top level docs or website folder. |
Not necessarily against this, but if that would be decided as the way forward, the first step would be anyhow to consolidate all the docs into 1 docs folder and webpage (on whichever repo that lives). So I think it is fine to first do that move without setting up another repo. |
3b2e9a5
to
2153643
Compare
A bunch of renames to remove the fluffy naming and related changes - fluffy dir -> portal dir - fluffy binary -> nimbus_portal_client - portal_bridge binary -> nimbus_portal_bridge - + renamed related make targets - Restructure of portal directory for the applications (client + bridge) - Rename of default data dir for nimbus_portal_client and nimbus_portal_bridge - Remove most of fluffy naming in code / docs - Move docker folder one level up - Move grafana folder into metrics folder Items that are of importance regarding backwards compatiblity: - Kept make target for fluffy and portal_bridge - Data-dir is first check to see if the legacy dir exists, if not the new dir is created - ENR file is currently still name fluffy_node.enr TODO: - ContentDb file is still called fluffy.sqlite3, need to figure out what to do with this, probably also check on if filename exists - Rename Fluffy tags at Copyright notice - Docs update
Also fix nimble tasks
- Change lock file name to portal_node.lock - Fix debug docker files - Fix portal testnet script
@@ -12,7 +12,7 @@ | |||
This repository contains development work on an execution-layer client to pair with [our consensus-layer client](https://github.com/status-im/nimbus-eth2). This client focuses on efficiency and security and strives to be as light-weight as possible in terms of resources used. | |||
|
|||
This repository is also home to: | |||
- [Fluffy](./fluffy/README.md), a | |||
- [Nimbus Portal client](./portal/README.md), a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- [Nimbus Portal client](./portal/README.md), a | |
- [Nimbus Portal Client](./portal/README.md), a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional (Nimbus & Portal are both considered a name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that 'Client' is part of the name because you will always say 'Nimbus Portal Client' not just 'Nimbus Portal'. Not a big deal either way :)
|
||
# Legacy target, same as nimbus_portal_bridge, deprecated | ||
portal_bridge: | build deps rocksdb | ||
echo -e "\033[0;31mWarning:\033[0m The portal_bridge target and binary is deprecated, use 'make nimbus_portal_bridge' instead" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to deprecate make portal_bridge
. Would be good to keep it as an alias for make nimbus_portal_bridge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we cannot do this however as make portal_bridge
builds the binary named as portal_bridge
. To keep our fleet building working, I can change/remove that however after we change it for our infra roles.
A bunch of renames to remove the fluffy naming and related changes
Items that are of importance regarding backwards compatiblity:
Other steps of this rename as it is more than just this repo: