search

mit-dci / lit

Lightning Network node software
MIT License
551 stars 119 forks source link

Added support for p2p function calls #407

Closed delbonis closed 5 years ago

delbonis commented 6 years ago

This PR introduces a new concept of p2p function calls. Currently uses msgids 0xC0, 0xC1, and 0xC2.

Hopefully this will help us transition all of the msgids that are used to fake function-like calls to other peers to instead just be functions. See below:

MSGID_DLC_OFFER               = 0x90 // Offer a contract
MSGID_DLC_ACCEPTOFFER         = 0x91 // Accept the contract
MSGID_DLC_DECLINEOFFER        = 0x92 // Decline the contract

With this new system it's just a single 16-bit "function ID", and there's fewer barriers to having multiple operations in-progress since it operates with a system of callbacks.

msg := ...
peer.InvokeAsyncCall(msg, func(resp PeerCallMessage, err error) (bool, error) {
    // handle message/error
    return true, nil
})

There's also support for (configurable) timeouts and a blocking version of the call interface, see the bottom of lnp2p/peer.go.

Currently, this PR strictly adds behavior, it doesn't remove or (significantly) change anything.

Still need to write some tests for this PR, as well. Tests have been written.

delbonis commented 6 years ago

A potential next step would be to refactor the remote control and message forwarding system to use peer function calls underneath.

delbonis commented 6 years ago

retest this please

delbonis commented 6 years ago

Ok so now it all actually works and there's no major issues with it, should write a test using the new pingpong peer call.

delbonis commented 5 years ago

This kinda gets obsoleted by the new BOLTy stuff, maybe I'll reuse some of this code in the future.

Varunram commented 5 years ago

Agreed that we need to rework this after the BOLT stuff is done, but I have a small issue with the automation thing that we setup. It seems to move this into "Done" but I think we need a category that somehow says "the PR wasn't reviewed / ready for merge, but needs a rebase and is waiting for future changes".

delbonis commented 5 years ago

I'm not really sure why we need a "done" category since the way we use the Project view is like a long-lived thing, not like how, say, Bitcoin Core uses it for broad but independent changes.

Varunram commented 5 years ago

Yeah, I don't know really, I think "Done" is a default state for GitHub automation.

delbonis commented 5 years ago

Oh no I added the automaton for it around when we set it up, but I don't know if there's a way to just "delete" cards when closed.