awfm9 / alvalor-go Goto Github PK

Unit testing the components that don't have their own executing threads (so everything but Node) should be straight forward. If it's not, we need to refactor them to be easily unit tested.

Currently, the sort parameter for the book sample function uses a full slice as input. It would make more sense to model it after the sort package and just provide a less function between two entries. We will have to do a bit more work internally, but the API will be cleaner.

We should document the code to make it more accessible for other users to start contributing. It's quite simple, but as a matter of principle, I think we should never neglect this.

Due to major changes in a way how components organized, we need to add extra tests/restore existing ones.

Once we reached the maximum number of desired peers, we should drop any further incoming connections, rather than dropping a random peer each time.

The current logger doesn't allow setting a log level and just dumbly dumps everything on standard error. We should improve it to actually provide proper output in a standardized format.

Some functions, like manage, are a bit too big and could be refactored into smaller parts.

We could add badges for Telegram, Slack and the GoDoc to the README.

Later, we can add things like test coverage & testing results.

I noticed that the fix in peer discovery to drop duplicate connections and connections to self lead to an issue where some peers will keep attempting to connect to the same address. When a peer accepts an incoming connection, he will still keep trying to connect to the listen address of that peer, because there is only a check for already connected addresses, not for already connected nonces.

It seems that there is still an issue propagating the listen addresses of listening nodes through the network.

Currently, we sort available addresses for outgoing connections randomly. Sorting them by the average difference between their IP address hash and the hash of all connected peers would allow us to somewhat geographically distribute the connections and make the peer-to-peer network more resilient.

It seems that network score mechanism has a bug regarding how score is calculated. For example, if node has 1 failure and 1 success, it would still have score of one due to this line of code:

math.Max(score/100, 1)

This means that it doesn't matter how many failures/drops it has, it would still go up to 1.

Another thing I noticed is active field filtering:

if !active && e.active {
     continue
}
entries = append(entries, e)

For the case when passed argument active == true and e.active == false, it would still add it to the result slice.

With the updated way to inject dependencies into handlers, it gives us the ability to separate state & execution logic in the manager as well. I will still need to explore a bit more how to cleanly separate this further, maybe we can also turn it into tons of separate functions with a more functional style. It would be great if we could find a way to have the glue code of the manager fully tested as well.

We should improve what we log and where we log it.

Currently, we use buffered channels for sends and just assume they won't block. However, if a peer stalls, the send on the channel could stall after some messages are queued. As such, sends on the channel should be wrapped with a select statement and time out if there is no place in the buffered channel. Otherwise, a simple send operation could block a lot of other operations.

Many parts of the network package currently use maps that are accessed concurrently. We can use the Go race detector to find race conditions and eliminate them.

It is hard to build networking code and components with their own goroutines to accommodate full unit testing, but we should at least make an effort to test as much as possible. It is worth considering to separate the state from the actors that drive the application.

We should change the codec interface to be chainable, so we can add as many compressors, serializers and other writers/readers in a row as we want. This will allow us to customize each connection with different compression, serialization or even network protocols in a flexible way.

If we disconnect a node intentionally, it should not create an error if possible. Right now, we simply close the connection and the receive goroutine will exit with an error, which will be propagated to the manage function of the node.

The comparison in the SimpleBook score implementation is wrong, leading to selection of the worst nodes instead of the best.

We need to add the transactions to the codec in order to be able to convert them to binary. One open question is how to use the hash of the serialized transactions as the ID.

The new network package does not have a proper API yet. We need to properly define the API we want to use, including how to send messages, subscribe for message, and so on.

With the networking code refactoring, it will be more neat to have the address book in a separate package. Different users, like services using the node, might have different preferences in how they want to prioritize peers.

We should add functionality to send discover messages to our peers in case we are out of network addresses to connect to, but have not filled our desired number of slots yet.

If we are above max peers or below min peers, we should add/remove enough nodes to reach our desired threshold right away. Maybe add some throttling to how many connections we establish at once.

We need to introduce some mechanisms to avoid simple denial of service attacks, such as flooding with invalid messages, flooding with valid messages, using up of connection slots, etc

We might want to have more control over the network layer. At the very least, we want to be able to manually add addresses and connect to those with higher priority. Maybe we want to consider given users the option to force connecting to a certain peer, or force disconnecting from a certain peer. Especially if we implement a reputation system on the second network layer.

In order to scale to many peers, we can't have a single bottleneck of a slow peer stalling all sends. We should add a queue per peer and make sure that an attempt to send will never block.

Currently, we have a number of components in the network package that have their own internal loops. We could move this to single component with a single ticker, which would call/trigger the others. This would result in simpler code and easier testing.

We should have a book implementation that segments peers by certain IP range and improve the algorithm for selecting a random subset. Tagging them on retrieval to make sure we never return already provided addresses twice until we get some feedback from the consumer should also be done. We can look at btcd for a Go implementation of the Bitcoin way, which is quite solid.

Right now, because we want to allow multiple peers on the same IP address, we have no way of knowing whether a peer on an IP already has an outgoing connection to us when we try to establish an outgoing connection to him. Using the nonce in the handshake is not enough here. We can generate a random nonce for each node on startup and use that one to detect nodes uniquely.

Propagating errors from the library is easy for external calls, but for the goroutines running inside the application, we should have a single function call for each execution loop that returns an error. This way, we can bubble errors all the way up to the caller similarly to external calls.

I just noticed that peer discovery is really broken between incoming and outgoing peers.
Basically, if we want to create an outgoing connection, we should try to connect to the public address of the remote node. However, when exchanging over the peer discovery protocol, we just assume the incoming address is the correct one. Unfortunately, the outgoing connection uses a random port, so won't work for other peers that try to reach us. We need to improve the handshake to include the remotely advertised address.

Currently, they are defined as structs, but are not actually emitted to the subscriber when we successfully connect or generally disconnect from a peer on the network.

The transaction format is more or less stable now:

• list of transfers: sender, receiver, amount
• list of fees: payer, amount
• list of signatures

Then there are the special transactions:

• genesis transaction: move funds from ETH account to VAL account
• account creation: create new account with n-of-m signatures
• account deletion: remove an account and move all funds to another

Transactions for second layer usage will be added later.

Currently, not setting the force flag will lead to an endless loop, rather than returning false for the insert when a value node already exists.

We should make sure we never try to connect to our own node.

https://github.com/rs/zerolog/log

It seems even faster, but mostly, I just like the API better.

Currently, at least when an incoming connection is added while the program is shutting down, shutdown will block indefinitely until another shutdown signal is sent. We should somehow make sure that no new peers are created while we try to shut down.

The official approach to vendoring has now been implemented by the dep tool:

https://github.com/golang/dep

It works similarly to vendoring tools where you just need the lock file and you can pull the vendoring files yourself.

After moving book back into the network package, testing has become harder because it is now a hard dependency. It would be good to abstract all events that can happen in a handler as its own interface, and inject two interfaces into the handler on start.

Each handler at that point has: log, waitgroup, config, dependencies, events. I would love it if we could abstract the first three into dependencies, but that would be a bit too convoluted of an interface I believe. Go is just not made for this kind of functional programming.

We need to specify exact release/usage/copy license in readme file. In addition to that, it is necessary to specify license notice in each file which is applicable to it.

I want to keep it possible to run multiple nodes on the same IP as if they were different peers, so we can not use the IP address as unique identifier. Adding random nonces to the handshake avoids that, because we can detect our own nonce.

In general, we should only abstract something if it's useful for other people (hasher isn't) or if it would be a big change to make modifications (changing the hash function isn't). So I think I think I went a bit overboard with this one and it's fine to just directly use the hash function.

A slow pending peer right now could lead to opening more connections than we want, meaning we would have to drop some again later. Pending peers should be taken into account when checking peer numbers.

Currently, we blacklist peers based on address (IP & port). This creates problems for incoming connections, which always originate from a different random port. If we want to conserve the ability to run multiple nodes on a single IP address, we will need a more fine-grained approach:

• block peers based on IP address for serious offenses
• block peers based on nonce for invalid permanent parameters
• score & prioritize nodes based on nonce for possibly temporary errors
• enforce one connection per nonce
• make nonce persistent between starts

The Book interface already exposes an Add function to add addresses to the book. We should not have two ways of doing the same thing, so the constructor helper function should not take a slice of addresses.

It's about time to add a README to the repository to describe the project and its structure.

Our subscriber might be interested to know when certain events happen, like when we successfully establish a connection to a new peer, or when we disconnect a peer. We can implement this by using event types that we submit on the same channel; that way, they can even include some meta information. Maybe we can generalize the current packet class to something else to combine this.

We should somehow expose which peers we are connected to, otherwise we could run into issues on higher network layers that need to know if we are connected to a node or not.