Fix mixup with chain_id and network_id #1343
Conversation
pipermerriam
changed the title
pipermerriam
force-pushed
the
piper/implement-chain_id-builder-function
branch
3 times, most recently
from
4e7da20
to
e25e388
Compare
pipermerriam
force-pushed
the
piper/implement-chain_id-builder-function
branch
from
e25e388
to
f1b06d3
Compare
| self.run_task(self._periodically_report_stats()) | ||
| if self.event_bus is not None: | ||
| self.run_daemon_task(self.handle_peer_count_requests()) | ||
| self.run_daemon_task(self._periodically_report_stats()) |
There was a problem hiding this comment.
This is potentially not a good idea. It'll cause the node to crash if peer stats reporting crashes.
There was a problem hiding this comment.
Agreed, but good enough for now. Fail hard during alpha/beta, then we can soften it in production.
| @@ -856,7 +854,9 @@ def _add_peer(self, | |||
| async def _run(self) -> None: | |||
| # FIXME: PeerPool should probably no longer be a BaseService, but for now we're keeping it | |||
| # so in order to ensure we cancel all peers when we terminate. | |||
| self.run_task(self._periodically_report_stats()) | |||
| if self.event_bus is not None: | |||
| self.run_daemon_task(self.handle_peer_count_requests()) | |||
There was a problem hiding this comment.
@cburgdorf can you 👍 / 👎 this change. It moves the task into the _run method so that it only starts when the peer pool has started as well as making it a deamon task which will crash the whole trinity process if it fails.
There was a problem hiding this comment.
Yes, that makes sense. 👍 That said, I think this will move soon into its own child service or something similar.
| self.run_task(self._periodically_report_stats()) | ||
| if self.event_bus is not None: | ||
| self.run_daemon_task(self.handle_peer_count_requests()) | ||
| self.run_daemon_task(self._periodically_report_stats()) |
There was a problem hiding this comment.
Agreed, but good enough for now. Fail hard during alpha/beta, then we can soften it in production.
|
|
||
| async def handle_network_id_requests(self) -> None: | ||
| async def f() -> None: | ||
| # FIXME: There must be a way to cancel event_bus.stream() when our token is triggered, |
There was a problem hiding this comment.
nitpick: The comment could be redirected to mention that this hack will go away as soon as ethereum/lahja#14 is included in the next release
There was a problem hiding this comment.
Will cleanup independently
What was wrong?
The
eth.chains.base.Chainclass had anetwork_idproperty. This is wrong sincenetwork_idis above the EVM level. This should have beenchain_idHow was it fixed?
MAINNET_NEWORK_IDandROPSTEN_...to live undertrinity.constantsDEFAULT_PREFERRED_NODESto also live undertrinityto prevent some weird imports.MAINNET_CHAIN_IDandROPSTEN_CHAIN_IDtoethmodule.netJSON-RPC module to return thechain_id(currently served over the event bus by theNodeclass)Nodeclass into something stand-alone which serves up the various config values like this. still mentally fleshing this out.Cute Animal Picture