These are chat archives for ethersphere/orange-lounge

15th
Nov 2017
Péter Szilágyi
@karalabe
Nov 15 2017 12:41
ethereum/go-ethereum#15468
Anton Evangelatov
@nonsense
Nov 15 2017 13:06
@holisticode https://twurst.com/articles/geth-config-file.html this might be related as well, although the blog post on blog.ethereum.org regarding geth 1.6 / config is newer
@lmars @holisticode @zelig nonsense/go-ethereum#2 - there are many differences between ethersphere/master and go-ethereum/master , so we should decide how to proceed with merging PRs. seeing that many of our PRs currently go directly to go-ethereum/master, we should probably review the diff between those two master branches, and cherry-pick commits, prior to reseting ethersphere/master to go-ethereum/master
Lewis Marshall
@lmars
Nov 15 2017 13:23
@nonsense we should just tag the current ethersphere/master with something so we don't lose it, and then reset it to go-ethereum/master.
the changes are so old, I suspect there is nothing in there that needs to remain
(if there is, we can just cherry-pick from the tagged commit)
Anton Evangelatov
@nonsense
Nov 15 2017 13:24
ok, i will do that
regarding ethereum/go-ethereum#15198 - i get the TestDiscoverySimulationExecAdapter tests (and other adapters) to pass without this commit, so I am not sure I can reproduce the problem. I will push another PR with my fix on top of this one, skipping the HACK commit.
Anton Evangelatov
@nonsense
Nov 15 2017 13:35
@lmars created master-legacy branch and tag. but i don’t think i have permissions to force-push go-ethereum/master on ethersphere/master so that they sync
i hope force push is disabled on the repo actually, but i don’t have access to its settings
Lewis Marshall
@lmars
Nov 15 2017 13:40
@nonsense ok great! Let's just drop the hack commit from the PR and then ping Felix for a review of everything else
I don't think I have access to the repositories settings either, maybe it is just Viktor who can do that
We could also just use a different branch for now (e.g. develop)
Anton Evangelatov
@nonsense
Nov 15 2017 13:42
i’d rather wait and sort out the master branch properly to be honest
Anton Evangelatov
@nonsense
Nov 15 2017 15:55

@lmars sorry to bring this up again, but I am confused. @nolash looked through ethersphere/go-ethereum#137 and explained that the following should be omitted from the PR:

  1. binary work (cmd/swarm)
  2. swarm/network changes relevant to the binary
  3. p2p/simulations
    so I wonder where should all unrelated to PSS changes go.

I updated the ethereum/go-ethereum#15198 which is p2p-simulations-fixes.

You explained that swarm-network-rewrite should get syncing implemented and then merged.

So my question - where does p2p-simulations fit and the changes from pss-networktest, which are independent of PSS? should these be merged in swarm-network-rewrite?

I don’t see how p2p-simulations-fixes and p2p-simulations are related, both seem to have a lot of changes and I am not sure which one is based on which.

lash
@nolash
Nov 15 2017 16:13
@nonsense I believe p2p-simulations-fixes is an attempt at fixing the dial problem, while p2p-simulations is the "pure p2p simulations" branch to be merged to master
Anton Evangelatov
@nonsense
Nov 15 2017 16:13
oh I see, that makes sense
lash
@nolash
Nov 15 2017 16:14
@nonsense according to my log they branch right at the start and dont seem to be related to each other
Anton Evangelatov
@nonsense
Nov 15 2017 16:14
considering that i couldn’t reproduce the dial problem (if this is the case) then p2p-simulation-fixes is just a slight refactor and change to the server logger.
thanks!
@nolash yes indeed they seemed unrelated, but names suggested otherwise
lash
@nolash
Nov 15 2017 16:15
perhaps, but there is a bunch of other commits in fixes that's not in the other
I doubt they are related, but maybe worth checking just in case
Lewis Marshall
@lmars
Nov 15 2017 16:15

where does p2p-simulations fit and the changes from pss-networktest

We should push all current simulation fixes (i.e. any changes in p2p/simulations which aren't on go-ethereum/master) to p2p-simulation-fixes, get that merged.

We should rebase swarm-network-rewrite onto p2p-simulation-fixes (and ultimately go-ethereum/master once those fixes are merged), and include any changes to swarm/network in that branch.

We should rebase pss onto swarm-network-rewrite and include any fixes / changes (e.g. pull request pss-networktest into pss, or just push the changes directly to the pss branch and drop pss-networktest)

Then, I think we should merge these into ethersphere/master so we can start making incremental changes to swarm / pss / simulations without doing this dance over and over
Anton Evangelatov
@nonsense
Nov 15 2017 16:18
@lmars and also merge p2p-simulations branch to go-ethereum/master right?
Lewis Marshall
@lmars
Nov 15 2017 16:18
that is already merged
Anton Evangelatov
@nonsense
Nov 15 2017 16:18
ohh.. ok
Lewis Marshall
@lmars
Nov 15 2017 16:18
ethereum/go-ethereum#14982
lash
@nolash
Nov 15 2017 16:20
I wasn't aware of this sorry @nonsense - maybe we should delete merged branches?
Lewis Marshall
@lmars
Nov 15 2017 16:21
yep, there are a lot of stale branches in ethersphere
Anton Evangelatov
@nonsense
Nov 15 2017 16:22
p2p-simulation-fixes is already marked for review, so anything that is not pss related could just go into a new branch once p2p-simulations-fixes is merged.
Lewis Marshall
@lmars
Nov 15 2017 16:23
sure
I would also like to see us running tests in ethersphere
so we don't need to wait for a go-ethereum merge before realising the tests are broken
lash
@nolash
Nov 15 2017 16:24
@lmars well yeah if they work better than the ones on ethereum, cos they seem broken half the time
Anton Evangelatov
@nonsense
Nov 15 2017 16:24
i agree
yeah, we should fix go-ethereum tests as well
lash
@nolash
Nov 15 2017 16:25
no point in having them if they dont work - and if there isnt a routine to fix them if they stop working, which seems to me to be the case
holisticode
@holisticode
Nov 15 2017 16:27
@nolash could you please have a look:
let's discuss here if there's something left to discuss
Lewis Marshall
@lmars
Nov 15 2017 16:27
when you say "they don't work" you mean every single test doesn't pass? I don't think that is a reason not to run any tests
Anton Evangelatov
@nonsense
Nov 15 2017 16:28
@holisticode Dferdi1199 appears to be a stupid bot, I saw it committed some nonsense EIP too. I guess just ignore it.
@lmars they do work, there are only 1-2 failures to do with the downloader and something to do with mock accounts.
holisticode
@holisticode
Nov 15 2017 16:28
yeah I was ignoring that one
did I link to the wrong one?
Anton Evangelatov
@nonsense
Nov 15 2017 16:29
@holisticode no, I just wondered who that is, so I looked it up. just in case anyone else wondered.
Lewis Marshall
@lmars
Nov 15 2017 16:29
I mean, it is actually really difficult to maintain such a large codebase with 100% passing integration tests, believe me I've tried!
holisticode
@holisticode
Nov 15 2017 16:29
ah ok :) thanks
Anton Evangelatov
@nonsense
Nov 15 2017 16:30
@lmars yes of course :) i am not sure if those failures can be fixed easily or not, but if we can help, then it'd be great. we’d avoid having to check the log output on every single PR.
Lewis Marshall
@lmars
Nov 15 2017 16:30
you fix them all, then next week someone merges something which introduces another intermittent failure, you end up in a cycle of just fixing tests
Anton Evangelatov
@nonsense
Nov 15 2017 16:31
i’d rather sort out our branch issues, before looking into those tests.
lash
@nolash
Nov 15 2017 16:32
@lmars maybe I don't fully understand the complexities of the test bots, but looking through the error messages last time there were some cryptic messages about blocks(?) or chunks(?) not matching or something, and no errors that looked like problems from the go test routines
I try to ask in go-ethereum about this, but there is always silence as reply
Lewis Marshall
@lmars
Nov 15 2017 16:32
we should definitely offer help, but it only takes a minute to look through test output, if we find it difficult I'd be happy to just look at the output as part of reviewing, it is usually quite easy to spot if test breakage is related to the changes being made
Anton Evangelatov
@nonsense
Nov 15 2017 16:32
@nolash i don’t think this is a test failure. there are however test failures in the go tests further up. the coverage warning is probably fine, although i don’t understand it either.
Lewis Marshall
@lmars
Nov 15 2017 16:32
anything to do with blocks is clearly not related to Swarm :)
lash
@nolash
Nov 15 2017 16:33
coverage that was it, thanks @nonsense
Lewis Marshall
@lmars
Nov 15 2017 16:33
and ultimately, it is up to the go-ethereum team to finally merge, and so they won't merge if a test failure actually needs fixing
lash
@nolash
Nov 15 2017 16:34
So things are actually merged into ethereum-master without tests passing?
Lewis Marshall
@lmars
Nov 15 2017 16:34
sure, if those failures are known, intermittent failures on master
lash
@nolash
Nov 15 2017 16:36
yeah theyre all red x's
I remember it looked the same a couple of weeks ago
Lewis Marshall
@lmars
Nov 15 2017 16:42
So, I think having tests on ethersphere means we at least have some indication of whether the changes we have made are genuinely broken (e.g. the tests would have easily highlighted the swarm binary as being broken long ago, the code just wouldn't have compiled)
lash
@nolash
Nov 15 2017 16:46
@lmars yeah but I think the swarm binary for example was deliberately left broken. Is there any way to make these tests more fine-grained? I'm just worried that if there's always a red fail cross reviewers will tend to pay less attnetion to it
and honestly guys, we all pass our relevant tests before we commit to PRs right? :)
there's always a red fail cross
holisticode
@holisticode
Nov 15 2017 16:47
and honestly guys, we all pass our relevant tests before we commit to PRs right? :)
lash
@nolash
Nov 15 2017 16:47
That is to say, if it's fail or not fail by one unrelated error in the codebase, these warnings are bound to receive less attention over time
holisticode
@holisticode
Nov 15 2017 16:48
how realistic is it to actually have swarm kick off only our "relevant" tests in ethersphere?
lash
@nolash
Nov 15 2017 16:48
@holisticode I have no idea, that's why I'm asking :)
oh
I meant
locally
holisticode
@holisticode
Nov 15 2017 16:48
I don't think it's that difficult
no automatic test running when we create PRs
it could be the same travis structure, but just a subset of the packages
if that makes sense
lash
@nolash
Nov 15 2017 16:49
I tend to always run tests locally on the parts I've changed before committing.
@holisticode yeah something like that would be good
holisticode
@holisticode
Nov 15 2017 16:49
yeah sure so do I
we'd also gain some more control of the workflow
Lewis Marshall
@lmars
Nov 15 2017 16:51
we can run a subset of packages with go run build/ci.go test packages...: https://github.com/ethereum/go-ethereum/blob/master/build/ci.go#L27
holisticode
@holisticode
Nov 15 2017 16:52
I'd see for example an automatic gofmt hook a good thing
yep @lmars
holisticode
@holisticode
Nov 15 2017 16:54
I just don't know if we need some formal ACK before doing such a thing
travis is an external tool and we would generate additional builds
Lewis Marshall
@lmars
Nov 15 2017 16:54
Travis is free to use for open source projects
holisticode
@holisticode
Nov 15 2017 16:54
oh :) nice
I have a question regarding the config file
so AFAIU, geth runs from the assumption that if no config file is provided, it would load flags from the command line
Lewis Marshall
@lmars
Nov 15 2017 16:57
yep
holisticode
@holisticode
Nov 15 2017 16:57
any other property is then default
swarm though always generates a config file
Lewis Marshall
@lmars
Nov 15 2017 16:57
yes, which is incorrect
holisticode
@holisticode
Nov 15 2017 16:57
should we switch to the same behavior?
ah ok
so can we assume that whatever is written to the config file at first boot is a default value?
Lewis Marshall
@lmars
Nov 15 2017 16:58
I'm not sure what you mean
Swarm should no longer be responsible for creating the config file
holisticode
@holisticode
Nov 15 2017 16:59
right
so any value written to the config file now when it's first created is the default value?
Lewis Marshall
@lmars
Nov 15 2017 17:00
I guess, let me look
I guess they are the defaults yes
holisticode
@holisticode
Nov 15 2017 17:01
also, I haven't checked deeply, but can all values in the config file also set via command-line?
Lewis Marshall
@lmars
Nov 15 2017 17:02
do you mean in geth?
holisticode
@holisticode
Nov 15 2017 17:02
no in swarm
because if not, either we need to create additional flags or that property isn't configurable anymore
Lewis Marshall
@lmars
Nov 15 2017 17:02
I don't think they have to be a one-to-one mapping, it is probably feasible to say that some things are only in the config file and not the command line, and vice versa, but in general they will probably appear in both
the config file should be the primary way to do fine grained configuration
(by editing it by hand)
holisticode
@holisticode
Nov 15 2017 17:03
great
Lewis Marshall
@lmars
Nov 15 2017 17:04
there is another question as to whether we involve environment variables (which helps for containerised deploys), but I don't think we need to worry about that for this first iteration
holisticode
@holisticode
Nov 15 2017 17:05
as in ENV VARS overwriting config file / cmd line params or for additional params?
Lewis Marshall
@lmars
Nov 15 2017 17:06
so typically, command line flags take precedence, then env vars, then config file, all for the same set of parameters
but ignore env vars, it was just a passing point, command line + config file is good enough :+1:
holisticode
@holisticode
Nov 15 2017 17:11
:+1:
(I guess there's no support yet for env vars in the binary as such)
Viktor Trón
@zelig
Nov 15 2017 19:38
great guys team on fire :smile:
re prs I agree that the decisions are gonna be finalised after balint started
@lmars 's plan about merge schedule etc is spot on. let's follow that
yes the current ethersphere master is irrelevant can be reset to follow upstream
@nonsense glad you don't have the dial problem but not sure how. maybe if others could try and run the discovery tests and try with 300+ nodes
Anton Evangelatov
@nonsense
Nov 15 2017 19:44
@zelig I tried with 128 nodes and 160 nodes… 256 nodes were failing, but this must be a local limit on my machine - i had similar problems with the PSS tests
Viktor Trón
@zelig
Nov 15 2017 19:44
merged branches should be deleted let me do this with balint at some point
Anton Evangelatov
@nonsense
Nov 15 2017 19:44
@zelig do you remember which adapter you ran tests with? exec or sim?
the discovery tests I mean.
Viktor Trón
@zelig
Nov 15 2017 19:44
sim both socket and net pipe
Anton Evangelatov
@nonsense
Nov 15 2017 19:45
ok
Viktor Trón
@zelig
Nov 15 2017 19:45
on my machine 256 were working
Anton Evangelatov
@nonsense
Nov 15 2017 19:45
asking, because i had to change the way kademlia is setup in the tests, in order to get the exec adapter to work
Viktor Trón
@zelig
Nov 15 2017 19:45
problems started around 350
oh
what needed changed?
Anton Evangelatov
@nonsense
Nov 15 2017 19:46
it was adding peers with enodes and endpoints to 127.0.0.1:30303, instead of the actual ports processes were listening on
Viktor Trón
@zelig
Nov 15 2017 19:47
re the config. I'm really happy if this is picked
picked up
I would agree that swarm should not generate the config file
environment variables should totally be supported out of the box. if fjls pick for toml doesn't support them I'll be disappointed
also for ease and completeness command line flags should be the same set as config variables and env vars
Lewis Marshall
@lmars
Nov 15 2017 19:53
I don't think geth uses env vars (it requires setting the EnvVar field in the flags like I did for p2psim: https://github.com/ethereum/go-ethereum/blob/master/cmd/p2psim/main.go#L47-L52)
Viktor Trón
@zelig
Nov 15 2017 20:24
so can we support config via env vars or not
holisticode
@holisticode
Nov 15 2017 20:25
Well to me it looks like we should grab the opportunity and add them
it will add a bit more work but not that much either, unless I am missing something