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

put seed nodes into separate file #1809

Merged

Conversation

@crypto-ape
Copy link
Contributor

commented Jun 20, 2019

Hey Monkeys!

This change isolates the hard-coded seed nodes into separate file.

Ape out!

@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

  • I think it can be a good idea to put this in a separated file but i will like opinions from others.

  • I personally dont like the implementation with the #include in the middle of the code. Even if in this case it can be harmless, it just dont looks right. We can read the text file line by line and it should be a better approach IMHO.

  • The location of this file should not be at root, maybe libraries/egenesis or programs/witness_node are better places.

@01mk

This comment has been minimized.

Copy link

commented Jun 21, 2019

This is brilliant lazy programmer solution 😄

It would be much better to read it from some kind of config file.

@crypto-ape

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Yes, it was a brilliantly pragmatic solution.

  • I agree that the file should be located more appropriately, 'libraries/egenesis' does not sound bad.
  • Yes, the raw include is both elegant and ugly, not systematic. Reading a file line by line sounds right.

I will wait a while before refactoring it until a more profound consensus is reached.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

the raw include is both elegant and ugly

Agree. I like it. :-)

I think you can get rid of the CMakeLists change if you #include with double quotes not angled brackets.

seed-nodes.txt Outdated Show resolved Hide resolved
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

It would be much better to read it from some kind of config file.

We have a config file. The list here is meant to provide a working hardcoded default.
IMO using a tool like embed_genesis for something that can be solved with a simple #include is overkill.

seed-nodes.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,19 @@
"seed01.liondani.com:1776", // liondani (GERMANY)
"104.236.144.84:1777", // puppies (USA)
"128.199.143.47:2015", // Harvey (Singapore)

This comment has been minimized.

Copy link
@pmconrad

pmconrad Jun 21, 2019

Contributor

Inactive, please remove

seed-nodes.txt Outdated Show resolved Hide resolved
seed-nodes.txt Outdated Show resolved Hide resolved
seed-nodes.txt Outdated Show resolved Hide resolved
seed-nodes.txt Outdated Show resolved Hide resolved
@oxarbitrage

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Agree. I like it. :-)

Ok , i don't have anything against it apart from being a bit ugly as mentioned, the include do what is needed.

I think you can get rid of the CMakeLists change if you #include with double quotes not angled brackets.

It will look better to me if we can do this changes, it will make the intention more clear as it will distinct it from normal angled bracket headers plus save the CMakeList changes.

@pmconrad pmconrad added this to the 3.3.0 - Feature Release milestone Jun 21, 2019

@pmconrad pmconrad added this to In development in Feature Release (3.3.0) via automation Jun 21, 2019

@crypto-ape crypto-ape force-pushed the crypto-ape:put_seed_nodes_into_separate_file branch from 9450481 to e9efdca Jun 24, 2019

@crypto-ape

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I have refactored the PR.

  • the CMakeLists.txt is not modified
  • the raw include is used ("seed-nodes.txt")
  • the added file with seed node entries also contains link to the forum topic
  • the inactive nodes are removed (@pmconrad You can check this, I also inspected the servers with nc -vc address port.)
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Hm somehow this looks wrong. I meant to remove puppies, Harvey, lafona, wackou, BlockTrades, cube.
You left in Harvey but removed liondani. liondani seems to be a bit unreliable, but please leave him in.
I always get "Connection refused" for Harvey, was it successful for you?

lafona has a new seed: bts.lafona.net:1776, please re-add.

@crypto-ape crypto-ape force-pushed the crypto-ape:put_seed_nodes_into_separate_file branch from e9efdca to b228820 Jun 24, 2019

@crypto-ape

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

(I don't remember, but now I obtained a refusal from Harvey as You did.)

I have re-added lafona and removed Harvey.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Thanks!

@ryanRfox ryanRfox moved this from In development to In testing in Feature Release (3.3.0) Jul 16, 2019

@abitmore abitmore merged commit d23e666 into bitshares:develop Jul 24, 2019

2 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature Release (3.3.0) automation moved this from In testing to Done Jul 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.