routing: moved routing package inside lnd#70
Conversation
There was a problem hiding this comment.
It isn't clear to me why this function is needed. The function below NewID seems to be sufficient, as within our graph vertexes are always public keys serialized in compressed format.
There was a problem hiding this comment.
NewIDDebug deleted.
There was a problem hiding this comment.
Why is graph.ID converted to a string before encoding the element? It would
I don't think that graph.ID should be converted to a string before encoding the element. Instead the relevant fields should be publicly exposed, then serialized individually one level up (within the Encode/Decode) functions. The encoding should be as efficient as possible, so a raw binary encoding of the struct should be used.
There was a problem hiding this comment.
Deleted. lnwire decoupled from graph.
There was a problem hiding this comment.
Similar comments apply to the handling of the case statements from here to the comment above.
Most of the logic that's currently within the case statement of each of the routing related structs should be within the Encode/Decode methods of their respective lnwire structs.
There was a problem hiding this comment.
The error is being ignored here.
There was a problem hiding this comment.
Deleted. lnwire decoupled from graph.
There was a problem hiding this comment.
The error is being ignored here.
There was a problem hiding this comment.
Deleted. lnwire decoupled from graph
There was a problem hiding this comment.
This init function is no longer needed as we now have (after my review comments are addressed) manual serialization of the relevant objects in the lnwire package.
There was a problem hiding this comment.
These gob functions are no longer needed.
There was a problem hiding this comment.
I don't think this function is needed since the graph itself is currently publicly exported.
There was a problem hiding this comment.
All publicly exported variables in this package are missing godoc-style comments. Also, I think this package should be converted to an internal golang package.
There was a problem hiding this comment.
The errors here shouldn't be ignored. Also I think many of the functions should not be publicly exported. Instead, only the necessary public entrance points need to be exposed.
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
LGTM ✨
Thanks for all the hardwork on this PR! This'll serve as a fine basis for routing within the daemon.
I'll get this merged as soon as the merge conflicts are resolved on this branch.
Use [33]byte for graph vertex representation. Delete unneeded stuff: 1. DeepEqual for graph comparison 2. EdgePath 3. 2-thread BFS 4. Table transfer messages and neighborhood radius 5. Beacons Refactor: 1. Change ID to Vertex 2. Test use table driven approach 3. Add comments 4. Make graph internal representation private 5. Use wire.OutPoint as EdgeId 6. Decouple routing messages from routing implementation 7. Delete Async methods 8. Delete unneeded channels and priority buffer from manager 9. Delete unneeded interfaces in internal graph realisation 10. Renamed ID to Vertex
|
I've rebased and squashed commits. Is it bug in the tests or in the code? |
|
There's some occasional negative interaction between the integration tests that I haven't yet quite pinned down which causes the occasional flakiness you're seeing. I have a few more items on my personal TODO list, but will hopefully eliminate the flakiness in the next few days. |
….beta Upgrade to LND 0.14.2 release
2 participants
Moved routing tools inside lnd. Changed internal type of graph.ID
from string to [33]byte.