Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move router handlers to separate files #1352

Merged
merged 13 commits into from
Apr 9, 2020
Merged

Move router handlers to separate files #1352

merged 13 commits into from
Apr 9, 2020

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Mar 20, 2020

This build on #1347.

The goal is to simplify the router by moving handlers to dedicated stateless functions in separated files, for syncing, announcement validation, route calculation, etc. That's quite a big change, but it's very repetitive.

Also, all gossip messages are now acknowledged by the router with a GossipDecision reply.

@pm47 pm47 requested review from sstone and t-bast March 20, 2020 13:12
@pm47 pm47 force-pushed the router-handlers branch 2 times, most recently from 2fada2b to 7327b97 Compare March 24, 2020 11:08
@pm47 pm47 marked this pull request as ready for review March 24, 2020 11:09
@pm47 pm47 force-pushed the router-handlers branch from e646c4c to 2b25ef7 Compare March 31, 2020 17:08
@codecov-io
Copy link

codecov-io commented Mar 31, 2020

Codecov Report

Merging #1352 into master will increase coverage by 0.23%.
The diff coverage is 94.37%.

@@            Coverage Diff             @@
##           master    #1352      +/-   ##
==========================================
+ Coverage   86.40%   86.64%   +0.23%     
==========================================
  Files         119      123       +4     
  Lines        9306     9364      +58     
  Branches      390      384       -6     
==========================================
+ Hits         8041     8113      +72     
+ Misses       1265     1251      -14     
Impacted Files Coverage Δ
...air-core/src/main/scala/fr/acinq/eclair/Logs.scala 89.79% <ø> (+2.04%) ⬆️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 88.33% <ø> (ø)
.../src/main/scala/fr/acinq/eclair/db/NetworkDb.scala 100.00% <ø> (ø)
...src/main/scala/fr/acinq/eclair/db/PaymentsDb.scala 80.00% <ø> (ø)
...la/fr/acinq/eclair/db/sqlite/SqliteNetworkDb.scala 97.29% <ø> (ø)
.../scala/fr/acinq/eclair/payment/PaymentEvents.scala 100.00% <ø> (ø)
.../scala/fr/acinq/eclair/payment/PaymentPacket.scala 90.90% <ø> (ø)
...scala/fr/acinq/eclair/payment/send/Autoprobe.scala 0.00% <ø> (ø)
...clair/payment/send/MultiPartPaymentLifecycle.scala 98.27% <ø> (ø)
...r/acinq/eclair/payment/send/PaymentInitiator.scala 95.89% <ø> (ø)
... and 23 more

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great clean-up!

I think we can go further, but if we do can you make sure you don't rebase so that we can keep the history of what we've already reviewed (these refactorings take time to properly review, so better to do them only once!).

Or we can choose to do follow-up PRs for some of my comments.

@@ -351,6 +352,7 @@ class PeerConnection(nodeParams: NodeParams, switchboard: ActorRef, router: Acto
setTimer(ResumeAnnouncements.toString, ResumeAnnouncements, IGNORE_NETWORK_ANNOUNCEMENTS_PERIOD, repeat = false)
d.behavior.copy(fundingTxAlreadySpentCount = d.behavior.fundingTxAlreadySpentCount + 1, ignoreNetworkAnnouncement = true)
}
case _ => d.behavior // other rejections are not considered punishable offenses
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth defining a trait next to GossipDecision to flag which cases should lead to a connection close?
This way we'll make sure anytime we update the list of potential gossip decisions in router, we correctly flag the new ones?

Copy link
Member Author

@pm47 pm47 Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we're not doing the exact same action. Another way would be to just replace the case _ => catch-all by more precise matchers. Compiler would warn us if we're missing a new type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But couldn't this catch-all be expressed by a specific trait defined in the router?

Copy link
Member Author

@pm47 pm47 Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if the trait meant "should not lead to a connection close".

@pm47 pm47 force-pushed the router-handlers branch from 7cf4cd5 to ad47ec4 Compare April 7, 2020 13:26
@pm47
Copy link
Member Author

pm47 commented Apr 7, 2020

I've moved all static methods to respective handlers file, and also took the opportunity to move router-related types to the Router object.

As this touches a lot of files and may generate conflicts, I suggest we merge this rather sooner than later.

t-bast
t-bast previously approved these changes Apr 9, 2020
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this!

@pm47
Copy link
Member Author

pm47 commented Apr 9, 2020

Rebased, waiting for review from @sstone.

Copy link
Member

@sstone sstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I did not like passing an akka context to most helper methods, which means they are not "pure", but this makes Router much more readable and it is still possible to tests all side effects cleanly.
A "pure" way of doing this would be to have methods that return the new state and all side effects but we would need to model them all (ack message, reply to sender, publish event, send message to self, send message to another actor...) and it's probably overkill for now but I'll open an issue to keep that in mind.

@t-bast
Copy link
Member

t-bast commented Apr 9, 2020

I completely agree with @sstone, that was exactly my reaction too :)
It's a good first step towards that direction, and doing everything in one step would have been too much.

@pm47 pm47 merged commit a58678e into master Apr 9, 2020
@pm47 pm47 deleted the router-handlers branch April 9, 2020 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants