ArkEcosystem / core

The ARK Core Blockchain Framework. Check https://learn.ark.dev for more information.
https://ark.io
MIT License
338 stars 285 forks source link

Forger does not read .env for P2P port #872

Closed vRobM closed 6 years ago

vRobM commented 6 years ago

Describe the bug Forger does not read .env for P2P port

To Reproduce Steps to reproduce the behavior: set p2p port to 5002 in core config start forger, see log validate ~/.ark/.env contains p2p port

Additional context Relay does this right, does it share code?

faustbrian commented 6 years ago

If you change the P2P Port you also need to adjust your hosts config for core-forger. Works without any issues here if I do it properly.

vRobM commented 6 years ago

where might that be?

faustbrian commented 6 years ago

In the plugins.js as all other configuration.

vRobM commented 6 years ago

Thanks, I found it here: https://github.com/ArkEcosystem/core/blob/develop/packages/core/lib/config/devnet/plugins.js

It appears it's not set up like the others.

I wanted to edit the forger section to add support for variables, but it seems to be using a fixed name that is different than what the other services use. 'hosts' (host:port) instead of 'host' with a separate 'port' setting.

I don't want to break it, and not sure how to convert it to the unified model already present.

faustbrian commented 6 years ago

The unified model already present? This is not some super complex change, it's a simple string you have to change if you change your P2P Port.

If your port is 5000 you do hosts: ['http://127.0.0.1:5000'].

vRobM commented 6 years ago

That is not the change I wanted to make.

As stated in my previous comment, I wanted to add support for using the .env file like the relay and other services use as listed in the plugins.js, since the forger is for some reason hard coded. (did someone else write the forger compared to the rest?)

separating the hosts: ['host:port'] into host: ['host'] port: ['port'] is more complex than a few line change as it affects other parts of the code.

alexbarnsley commented 6 years ago

Lots of people wrote lots of parts, I'm not sure if you're trying to assign blame there?

Anyway, the best way to express what you're after is to do a pull request. It goes through testing and review so you're less likely to break something if that's what you're worried about.

faustbrian commented 6 years ago

The forger is hardcoded and not using and environment variable because it is an array of hosts. Those hosts can be http://ip:port or just domains like https://relay.domain.com without a port so having a host + port config would just add useless complexity by having to check in the code if the port is empty and then just using the host without a port.

vRobM commented 6 years ago

No blame needed. Just an observation that it is very different.

Array of hosts? Is there a grander vision for distributed forging of multiple things?

Yes, I agree on the code complexity. What's a better option to have it be configurable from commander?

faustbrian commented 6 years ago

Hosts are your relays. If you run your forger behind a firewall your relay will be somewhere else then 127.0.0.1 and if one of your relays goes offline you could miss a block because your forger will be unable to broadcast the forged block.

hosts: [
    'http://127.0.0.1:4003',
    'https://r1.domain.com',
    'https://r2.domain.com',
    'https://r3.domain.com',
    'http://123.123.123:564',
    'https://r4.domain.com',
    'https://r5.domain.com'
]

This is how your hosts configuration could look like if you had multiple relays running. As long as one of them responds your block will be broadcasted and avoid that you miss a block because of a relay being dead.

Small update on Core 3.0. It will be possible to forge multiple different coins at the same time to form a unified chain of total world market dominance. /s

vRobM commented 6 years ago

Ok great, so it sounds like it's worth exposing the hosts: [ .. ] block in the .env file to keep tweaks in one place.

This brings up the question of when is TLS/cert support interesting to add?

Lol, Core 3.0? Bring it.

fix commented 6 years ago

well on HTTPS support:

faustbrian commented 6 years ago

As @fix said there is no point in us implementing anything like that. You can just throw your relay or forger behind nginx and cloudflare and call it a day.