cleaning up addrmgr

[dskloet]

#840 (comment)

about addrmgr. I've mentioned a few times as well that it's probably the package that needs the most improvement/cleanup in the entire repository.

I thought I could maybe have a look at that. But I got confused right away

I think this should be modified instead such that the GetNewAddress callback returns a net.Addr

But as far as I can see GetNewAddress already returns net.Addr. What am I missing?

This is my first look into btcd so any help is appreciated.

[davecgh]

Hey @dskloet,

I saw you asking on IRC, but you ended up leaving before I could answer!

That part of the comment was already addressed in the connmgr as a part of the linked PR. The rest of the thread however was referring to the addrmgr package such as functions like its GetAddress and NetAddressKey functions.

In particular, the server setup of the connection manager sets up the address function as a closure here, which is conditionally defined just above it here.

Notice how in that function there are various conversions in and out of raw strings and then a final conversion to a net.Addr here.

[dskloet]

I waited on IRC for several hours! :-D

OK, so first (line 2414) it calls s.addrManager.GetAddress("any"), which returns an addrmgr.KnownAddress called addr.

Then on addr, it calls NetAddress(), which returns a wire.NetAddress, 3 times.

The first one is to create the group key. Should addmgr.GroupKey() be able to get a net.Addr instead of wire.NetAddress?

The second is just to get the port. I guess that can be done with net.Addr.

The third is to convert to net.Addr so there net.Addr definitely suffices.

In addition to NetAddress(), it also calls LastAttempt, so it seems the KnownAddress is still necessary.

So is the goal just to replace wire.NetAddress with net.Addr here?

By the way, I noticed that newServer() is 330 lines long. What do you think about splitting that up into smaller function?

Also s.addrManager.GetAddress("any") takes an argument which is always "any", and is completely ignored. Can we just get rid of that or is it required for a certain interface?

[dskloet]

Stupid question: How do I run all the tests?
If I remove the argument from GetAddress() and then run go test, it complains about the call in server.go but not about the call in addrmgr/addrmanager_test.go

[davecgh]

I believe the easiest way to handle it will be to make wire.NetAddress implement the net.Addr interface as suggested by @Roasbeef in the comment you first linked here.

You're correct to note that there is no need to call NetAddress() 3 times. Saving the results in a local and using it would be more efficient. The fmt.Sprintf("%d", addr.NetAddress().Port) is also fairly expensive as fmt.Sprintf involves reflection. A quick optimization there would be to at least reverse the if to check for tries < 50 first so if could avoid the fmt.Sprintf when it's not. Even better would be to convert the activeNetParams.DefaultPort string to a uint16 once outside of the closure and then the if could be if tries < 50 && na.Port != defaultPort { which would avoid the need to convert multiple times altogether.

I'm prety sure the intention of the argument to GetAddress was so that it could support looking up addresses according to some type of filter such as IPv4, IPv6, and only those which support specific wire.ServiceFlags (full nodes, nodes that support bloom filters, nodes that support segwit, etc). As you noted, it currently is ignored (which is yet another reason I mentioned the package is in dire need of cleanup and further development). Also, the string parameter there is, in my opinion, pretty obviously not a good type. It really should be something that allows bitflags to be set so that the caller could request combinations such as peers that are full nodes and support IPv6 and bloom filters.

As far as the tests, go test $(glide nv) will run all of the tests while excluding the vendor directories. There is also a goclean.sh script which checks if the code is "clean" by running the tests with the race detector enabled along with additional linters that will report any issues.

You can also change directory to a specific package and run bare go test to only run the tests in that package.

[dskloet]

Don't you think it's better to remove the unused parameter for now? It can always be added back (with a more appropriate type) when it's going to be used.

And what about my suggestion to split the 300+ line function into smaller functions?

And it's been a while since I contributed to a project through github. I followed the instructions here and did

$ git clone https://github.com/btcsuite/btcd $GOPATH/src/github.com/btcsuite/btcd

but that doesn't work if I want to be able to make pull requests, does it? Should I first clone the repo inside github and git clone that to my local machine?

(I could start by adding a document Getting started contributing to collect the answer to all my silly questions about running tests, cloning the repo and making pull requests.)

I like to make changes as small as possible. Would, for example, a single PR for just optimizing the order on the if or one for removing the unused parameter, be OK with you? Or would you prefer reviewing several such improvements together in one somewhat larger PR?

[davecgh]

I don't have any objections to either removing the unused parameter or splitting the newServer function up a bit. I generally prefer smaller PRs since they are typically easier to review and can help prevent holding up uncontroversial changes over portions that are controversial. That is particularly true in larger refactorings (which granted none of what we discussed so far would be), but, for example, in the case where the RPC server is being split into a separate package, I much prefer PRs where it does it one step at a time such as first decoupling it from server through the addition of config structs and behavioral interfaces, then performing the minimal work necessary to move it to a separate package, etc.

In terms of your pull request questions, I created a gist some time ago which discusses pull request management here. It could be cleaned up and added to the main repository under some type of getting started as you suggest.

Thanks for your help!

[dskloet]

I was looking at making wire.NetAddress implement net.Addr. For that it needs to implement Network() (I think that just means return "tcp") and String() which is a bit trickier.
One thing that occurred to me is whether String() should compute the string each time or just once and store it in the object. And then I realized I don't even know if that would work since the wire.NetAddress might not be immutable.

I think it's good for all simple data objects to be immutable so I started looking at where wire.NetAddress instances are being mutated. There are some places but it doesn't seem fundamentally necessary. I thought to make it more strict and more obvious, I could make the fields of wire.NetAddress private and only expose getters. What do you think about that?

And then the question of whether to compute the string each time or cache it. Would it be called often? And are there many instances? Are we more CPU constrained or memory constrained or does it not really matter in this case?

[davecgh]

Yes, for Network it should only need to return "tcp" since there are no other supported ones (at least for now, there is really no fundamental reason Bitcoin couldn't work with other network types although the code does make assumptions based upon the guarantees TCP provides, so other changes would likely need to be made anyways).

As for String, I would definitely just compute it each time regardless of immutability. A simple return fmt.Sprintf("%s:%d", na.IP, na.Port) should do it. There really aren't many times when it will actually need to be called, so I personally wouldn't mess with memoizing the result there. Pretty much the only time it would be invoked is when it's selected for outgoing connection of which there are a max of 8 at any given time.

As for immutability, I too am a fan of it, but unfortunately, Go lacks compiler-enforced immutability (ala const or similar). I had previously brought up the question of using getters to somewhat mitigate that, but the consensus among the devs was by and large to use structs with exported fields for the wire types since callers are much more easily able to create them and they can make use of keyed structs to only initialize the fields they want to while leaving the others set at the zero value, etc. Along those same lines, there was a fair amount of push back about having the NewX functions since they force heap allocation even when stack allocation would otherwise be enough.

EDIT: Fixed the fmt.Sprintf statement to include na. on the port field.

EDIT2: On second thought that really should be using net.JoinHostPort to ensure it adds [] with IPv6. So, it would be return net.JoinHostPort(na.IP.String(), strconv.Itoa(int(na.Port)))

[dskloet]

Thanks for clarifying.
There are still some places where the fields of the struct are set directly, but I think it's always right after creating the object. Do you see any problem if I change that to providing all the fields at creation so the code is closer to what it would be if the struct was immutable?

[davecgh]

Sounds like a good idea to me. Most of the code tries to populate all the fields it can at creation time in general, so any cases with wire.NetAddress that aren't doing that really should be. I suspect addrmgr will be the biggest offender.

[dskloet]

One more thing. In NewNetAddressIPPort it explicitly rounds the timestamp to single second precision, but for example in peer.go it creates a wire.NetAddress directly with a high precision timestamp. I'd like to change that to call NewNetAddressIPPort instead so it doesn't have to set the timestamp explicitly, but it would result in a different timestamp precision. Why do we care about the precision in one place but not in the other?

[dskloet]

Btw, I noticed I was using spaces instead of tabs, but goclean.sh passed all the tests. So while it says it runs gofmt, it doesn't seem to be the case.

[davecgh]

Nice catch that it's inconsisent. I agree with changing it to use the function and making it consistent since it really should all be single second precision everywhere. On the plus side, it's not really a big deal here using sub-second precision sometimes since it's not consensus code and it will be truncated to single second precision when sending it over the wire since the wire protocol uses a unix-style timestamp encoded as a uint32 (known issue in Bitcoin that a new wire protocol message will be needed before ~2106 as a result as well as support for longer addresses like I2P addresses).

It's strange that you didn't get an error with spaces. I just tried changing a single tab to spaces (and disable the automatic gofmt my editor does which would fix it). Here is the result:

$ ./goclean.sh
++ grep -v 'ALL_CAPS\|OP_'
+++ glide novendor
++ tee /dev/stderr
++ gometalinter --disable-all --enable=gofmt --enable=golint --enable=vet --enable=gosimple --enable=unconvert --deadline=4m ./addrmgr/... ./blockchain/... ./btcec/... ./btcjson/... ./chaincfg/... ./cmd/... ./connmgr/... ./database/... ./integration/... ./limits/... ./mempool/... ./mining/... ./peer/... ./rpctest/... ./txscript/... ./wire/... .
btcd.go:1::warning: file is not gofmted with -s (gofmt)
+ test -z 'btcd.go:1::warning: file is not gofmted with -s (gofmt)'

[dskloet]

Now I just noticed that it's complaining about not finding glide but in the end it returns without error. Do you know where it should be able to find glide? Do I need to add something to my PATH?

Here's what I get from goclean.sh:

++ grep -v 'ALL_CAPS\|OP_'
++ tee /dev/stderr
+++ glide novendor
./goclean.sh: line 17: glide: command not found
++ gometalinter --disable-all --enable=gofmt --enable=golint --enable=vet --enable=gosimple --enable=unconvert --deadline=4m
./goclean.sh: line 17: gometalinter: command not found
+ test -z ''
++ glide novendor
./goclean.sh: line 24: glide: command not found
+ env GORACE=halt_on_error=1 go test -v -race -tags rpctest
=== RUN   TestCreateDefaultConfigFile
--- PASS: TestCreateDefaultConfigFile (0.02s)
=== RUN   TestHelp
--- PASS: TestHelp (0.02s)
PASS
ok    github.com/btcsuite/btcd  1.865s
+ set +x
ok    github.com/btcsuite/btcd/mining 0.040s  coverage: 17.8% of statements
?     github.com/btcsuite/btcd/mining/cpuminer  [no test files]
ok    github.com/btcsuite/btcd/peer 0.040s  coverage: 77.1% of statements
ok    github.com/btcsuite/btcd/wire 0.401s  coverage: 99.5% of statements
ok    github.com/btcsuite/btcd/mempool  0.052s  coverage: 56.9% of statements
ok    github.com/btcsuite/btcd/addrmgr  0.105s  coverage: 70.0% of statements
ok    github.com/btcsuite/btcd/txscript 0.664s  coverage: 95.4% of statements
?     github.com/btcsuite/btcd/integration  [no test files]
ok    github.com/btcsuite/btcd/chaincfg 0.002s  coverage: 96.7% of statements
ok    github.com/btcsuite/btcd/chaincfg/chainhash 0.002s  coverage: 100.0% of statements
ok    github.com/btcsuite/btcd/blockchain 3.353s  coverage: 69.8% of statements
ok    github.com/btcsuite/btcd/blockchain/indexers  0.037s  coverage: 18.3% of statements
?     github.com/btcsuite/btcd/blockchain/fullblocktests  [no test files]
ok    github.com/btcsuite/btcd/btcjson  0.008s  coverage: 99.9% of statements
ok    github.com/btcsuite/btcd/database 0.093s  coverage: 100.0% of statements
?     github.com/btcsuite/btcd/database/cmd/dbtool  [no test files]
ok    github.com/btcsuite/btcd/database/ffldb 1.658s  coverage: 89.4% of statements
ok    github.com/btcsuite/btcd/database/internal/treap  0.023s  coverage: 100.0% of statements
ok    github.com/btcsuite/btcd/btcec  1.316s  coverage: 97.4% of statements
?     github.com/btcsuite/btcd/limits [no test files]
ok    github.com/btcsuite/btcd/connmgr  0.039s  coverage: 63.1% of statements
?     github.com/btcsuite/btcd/rpctest  [no test files]

[dskloet]

And do you try to replicate the exact same behavior of Bitcoin Core even for non-consensus code? Or do you allow to do non-consensus things different/better from Core?

[davecgh]

I personally just make sure that $GOPATH/bin is in in my system $PATH since that is where all built Go programs are installed to. That includes glide, gometalinter, all of the individual linters it uses, and even btcd itself when you run go install. You can take a look at the .travis.yml file for the setup the CI build environment uses if you like.

We do not aim for exact behavior outside of consensus code and other required interoperability points such as the wire protocol. We do aim to provide a compatible RPC API for chain related RPCs although it is definitely not 100% compatible (internal implementation differences prevent some fields from being available). Additionally, we try to maintain a somewhat similar default mempool policy in regards to what transactions are considered "standard" and therefore accepted to the mempool and relayed to other nodes. Outside of those areas, we implement it however we feel best fits the code architecture, Go best practices, tradeoffs in regards to garbage collection, etc.

[dskloet]

Wow with $GOPATH/bin in my path goclean.sh produces waaay more output and takes much longer to run.
But it still doesn't find gometalinter and doesn't complain about gofmt and returns success on the command line.

[davecgh]

$ go get -v github.com/alecthomas/gometalinter
$ gometalinter --install

EDIT: That second line tells gometalinter to install all of the individual linters it makes use of.

[dskloet]

Shouldn't it be listed as a dependency so it automatically gets installed when I install btcd or something? And isn't it strange that goclean.sh nevertheless returns success?

[davecgh]

Well, it's not a dependency for btcd itself (nor any of the tests), so that's why it isn't installed along with it. For example, you can run go test $(glide nv) without the extra linters and it will all work properly.

The goclean.sh script is really specifically for the continuous integration build environment (TravisCI) which runs it on every new PR and every merge to master. The environment is setup properly via the .travis.yml and thus the script makes that assumption. That said, I don't imagine anyone would be opposed to adding a check in there to ensure the tools are available.

From speaking to several of the devs, most of us have our editors setup to automatically run gofmt on save and just run go test in the affected packages (again through the editor). Then when the PR is submitted, Travis will do the full suite of tests, lint checks, race detection, etc (the stuff defined in goclean.sh).

The reason for that is, as you noted, it takes a while to run them the full suite of tests and lints.

[dskloet]

It seems I'm missing a bunch of things. golint, vet, gosimple and unconvert.

I'm not familiar with go install. Would it be possible to make a go install target such that a new contributor can do

go install btcd-dev

to install all the all the dependencies for developing btcd?

Also I saw set -ex in the goclean.sh script but it doesn't seem to work for commands inside $(). I might have a look at making the script fail fast at some point.

[davecgh]

The gometalinter --install line is what installs all of the linters (golint, vet, gosimple, unconvert, and a few others that aren't actually used yet).

It's not possible to add a target like you can with with make. go install only takes a list of go packages to install (or uses the current package per the current directory if none specified). A script could be made though. I'm fairly certain it's only those two lines I posted previously (the one to get and install gometalinter and the other to have gometalinter install all the linters it uses) that you need.

[dskloet]

I'm now looking at having wire.NetAddress implement net.Addr.
As a first step I just made wire.NetAddress.String() return "foo" to see what would fail.

I have newAddressFunc in server.go return the NetAddress directly instead of converting to a new net.Addr. Then I ran goclean.sh and everything passed. Isn't that worrying?
Shouldn't there be test coverage that depends in some way on what is returned by newAddressFunc in server.go?

[davecgh]

Yes, that is part of the reason I mentioned that the addrmgr package and related code needs to be cleaned up. There is definitely not enough test coverage for it.

In this particular case, the reason there is no test failure is because the connmgr package contains unit tests which ensure that the provided address function is invoked under the correct circumstances and is handled properly when it returns incorrect information by stubbing the function with a mock.

However, as you note, there isn't a unit test in btcd proper that specifically checks the function being provided to the connection manager itself works as intended. That definitely should be remedied. I believe the easiest way to accomplish that would be to make the closure independently accessible and write a test which creates a new address manager instance and populates it with known values and then ensures the closure returns one of the correct values. It should also tests error conditions.

[dskloet]

I can try looking into that. Do you think there should also be an integration test that runs the server exercises this code?

[davecgh]

I would love for there to be more integration tests, though I suspect there is quite a bit of work that needs to be done to get to the point you can integration test the server itself. The reason I say that is that even though there has been quite a bit of recent decoupling of things out into packages like mempool, mining, connmgr, peer, etc, server is still pretty tightly coupled to certain things (most notably the RPC server), and it doesn't have any way to create an instance with mocks.

EDIT: I should note that the packages themselves (with the exception of addrmgr) are actually generally quite well tested. Most of where you run into lack of coverage is when it comes to integration and things that are not in packages.

[dskloet]

I was actually talking about running the real thing, maybe several instances, and then poking it with RPCs. Maybe some new RPCs should be introduced to poke at it enough to test it. I'm not familiar enough with Bitcoin and btcd but it seems like it should be possible in general.

[davecgh]

Ah ok, I see. I was thinking you meant the other way since I'm fairly positive there aren't currently any RPCs related to the address manager. For reference, the available RPCs are documented here.

So, if you wanted to go that route, you'd end up needing to add some extension RPCs that allow you to add/remove/query the address manager. That said, I personally am in favor of this approach as I think it is definitely the most robust in terms of ensuring all of the pieces fit together properly. I certainly concede it is a fair amount of work, but I do think it would be worth it.

[dskloet]

I have a stupid problem. I'm writing server_test.go and I need to create an instance of addrmgr.KnownAddress. I'm trying to use addrmgr.TstNewKnownAddress from addrmgr/internal_test.go like it is used in addrmgr/knownaddress_test.go. I've imported "github.com/btcsuite/btcd/addrmgr" but I keep getting

./server_test.go:34: undefined: addrmgr.TstNewKnownAddress

Any idea what I might be missing?

[davecgh]

Tests are only run on a per-package basis, so anything that is only defined is test files is not available outside of the package they're defined in. You would have to provide an exported NewKnownAddress function from knownaddress.go in order to get access. That would also naturally mean removing the TstNewKnownAddress in favor of it and updating the tests in knownaddress_test.go to use the newly exported NewKnownAddress.

[dskloet]

But knownaddress_test.go is in a different package. addrmgr_test as opposed to addrmgr.

I'm not sure anymore if I should create KnownAddress instances in my test, or if I should create an AddrManager and have it create the known addresses. But to create an AddrManager, I need to pass in a lookupFunc and I'm not sure what to do with that. I looked at addrmgr_test.go but it only passes in nil or a function that always returns an error so that seems to be untested as well?

[davecgh]

Correct about addrmgr versus addrmgr_test. The way the Go testing framework works is that both addrmgr and addrmgr_test are active when the tests are running for addrmgr. However, when you're running tests for another package (the main package in this case), the test code in all other packages (addrmgr_test in this case) is not active.

I personally would just pass in a lookupFunc. The following pseudocode should get you rolling with that.

type mockLookups struct {
	fieldsYouWantToControlBehavior int
	...
}

func (m *mockLookups) lookupFunc(host string) ([]net.IP, error) {
	// Whatever test-specific things you want to test in here.
	// You can check m.foo to alter what is returned based on configured fields in struct.
}

...
func Test....(t *testing.T) {
	m := mockLookups{configStuff....}
	amgr := addrmgr.New(dataDir, m.lookupFunc)
	...
}

EDIT: Oh, and about nil on the lookup func, the only time it's used is when a non IP address string is provided and a DNS lookup is needed. That is why the addrmgr tests all use nil, because they work with actual IPs.

[dskloet]

But I'm not trying to import addrmgr_test. addrmgr/internal_test.go declares itself to be in package addrmgr, not addrmgr_test. I guess this is just a strange quirk of Go itself that it assigns special meaning to filenames that end in _test.go when it comes to package visibility?

So I'm trying to test newAddressFunc from server.go. I've factored out a function that creates the newAddressFunc from an AddrManager and a getOutboundGroupCount function. But now I realize that the behavior depends on knownAddr.LastAttempt(). So either I need to be able to mess with the lastattempt of the known address, or I need to be able to mess with time.Now().

I think instead of passing an AddrManager to createNewAddrressFunc, I could pass just the GetAddress function and stub that out. Then I can pass it any KnowAddress I want.

Or I can try to create a fake clock and inject that into the server so I can manipulate time. But that seems like it could be a huge refactoring to do it consistently (though I think it would be good to have an injectable clock everywhere time.Now() is used).

What do you think?

By the way, the whole behavior of newAddressFunc and GetAddress seems very arbitrary. They both randomly try an address a bunch of times until there's something it likes, based on arbitrarily computed probabilities. The whole thing looks like it's O(n^3) because the buckets of addresses also don't have random access so the algorithm needs to iterate through (either a linked list for addrTried or a map for addrNew) to get a random element. I guess this is all part of "addrmgr needs to be cleaned up"? Or does some of this have good reasons?

[dskloet]

Thinking about it some more, I wonder why newAddressFunc is this lonely closure in server.go. Shouldn't that really be a method on AddrManager? That would require AddrManager to have access to OutBoundGroupCount() but I think it's reasonable to give it a reference to it. Then it could replace GetAddress, which is currently only called from newAddressFunc.

In order to test it properly, I'd have to go the fake clock route though but I think that's a good idea anyway.

What do you think?

[dskloet]

Dave?

[Roasbeef]

Hey @dskloet,

I guess this is just a strange quirk of Go itself that it assigns special meaning to filenames that end in _test.go when it comes to package visibility?

Yes, all files that end in _test.go are interpreted as special "test files" to go. Within such a file, the same package name as the enclosing folder can be used, or a different package name can be used instead to force all the tests to exercise the behavior of a package in a black-box manner.

So either I need to be able to mess with the lastattempt of the known address, or I need to be able to mess with time.Now().

Passing in GetNewAddress as a stub in the function seems to be a less invasive change as a whole. Alternatively, you can just directly mutate the lastAttempt of a test instance under examination.

I guess this is all part of "addrmgr needs to be cleaned up"? Or does some of this have good reasons?

This behavior is part of the address manager as it was originally implemented in Bitcoin Core. This paper presents a very comprehensive overview of the behavior of the addrmgr and also steps that can be taken to harden Bitcoin nodes from eclipse attacks.

I think you'll get faster responses and therefore will be able to iterate more quickly on this set of changes if you came over to the #btcd channel on freenode where we all hangout. Also, one additional way to iterate more quickly w.r.t newly proposed changes is to create a PR against the project, marking it as [WIP] in the title. This indicates that the PR isn't yet fully complete, but the author is seeking some feedback as to any design or testing decisions they've made so far .

I really appreciate the work you've been doing so far within the addrmgr package as a result of Dave and I's discussion on a past PR. However, felt it might be useful and produce higher impact PR's to step back for a second to a higher level to details exactly what we meant by "cleaning up the addrmgr":

• The original addrmgr is one of the oldest unmodified packages within the codebase. As a result, it's currently lagging behind the best coding practices w.r.t to code comments and style that have began to emerge within the coding culture of btcd. Many of these practices are currently spelt out within our contribution guidelines. One part of cleaning up the package would be minor refactorings and additional comments to increase the code quality of the package.
  * A larger more substantial change would be to re-write the serialization of the addrmgr's state to use the new (relative to the addrmgr) database package. Such a change should reduce the changes of corruption of the peers.json file as all mutations to the addrmgr's state would take place in atomic database transactions. This potential change is also described within this issue.
  • Finally, a much needed change would be to being to implement some or all of the eclipse attack counter-measures described in this paper. This would make all btcd nodes on the network more resistant to network partitioning and isolation attacks. Some of these counter measures have already been implemented in Bitcoin Core by one of the author's of the paper, so it'd be nice if we could also achieve partial parity here.
  • As a final bonus, it'd be great if we could implement some of the counter measures described in this paper against attacks that can be launched by an individual that controls an entire AS (Autonomous System) on the internet. To my knowledge no other Bitcoin node has yet implemented some or all of the counter measures described in the paper.

[dskloet]

Thanks for the suggestions. When I started making my small changes, I set out to make wire.NetAddress implement net.Addr as suggested before. I've invested in that now so I'd like to finish that before looking into other things. I also don't think I understand the code well enough yet to implement whole new functionality from those papers. But I think doing refactoring will help me understand the code better.

Would you be against introducing a fake clock to allow testing functionality that depends on time.Now()? I think that might be the best way to add enough test coverage to refactor safely. Directly mutating lastAttempt on a test KnownAddress is only an option if the KnownAddress is created in the test. But KnownAddress seems to be intended to be created by addrmgr so to test that functionality using a test instance isn't enough as an option.

I've connected to IRC several times and waited for hours without response. So I think async communication is the best option. And I'd prefer to agree on a direction before starting on code. I mean, I can start coding the fake clock but if you already know you don't want it, that's just a waste of time. It also suggests that int the contribution guidelines

announce your plans before you start work

[Roasbeef]

Yeah, admittedly most of the tasks I described above are a bit advanced. It definitely makes sense to ease your way into larger changes like that by refactoring bits here and there, which will allow you to get a better feel for the codebase as a whole.

About IRC: yeah most of us idle there using bouncers, or a remote tmux session, as a result we may be seen as online, but won't immediately respond to questions. In that case, if you stick around long enough once of us will catch your question in the scroll-back as we catch up to the history we missed.

OK, about the refactor: I think a straightforward route is to add a timeSource function closure to the addrmgr.New function which has a function signature of func() time.Time. During regular initialization in the server, the time.Now function can be passed in, and within the blackbox unittests, the test author can either pass in a static value, or a function which iterates through a scripted set of times in order to test bucket evictions, etc. With this modification, all calls to time.Now() within the addrmgr package now would morph into a call to a.timeSource(). This is a rather minute change but should allow a good bit of flexibility when testing.

[dskloet]

Cool. I'll give that a go next time I have some time.
That's how I would have done it in any other language but somehow I felt like the more Go way to do it would be to make a global clock that I would call instead of time.Now() and just replace the global clock in tests that need it.

[dskloet]

Apologies for the silence. Over the December break I lost momentum and I seem to have prioritized other projects since. I'd still like to get back to this eventually but I can't promise anything. I won't get upset if someone else decides to take addrmanager in a different direction in the meantime.