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

Add Lightning Network support #18179

Closed
wants to merge 24 commits into from
Closed

Conversation

@icota
Copy link
Contributor

icota commented Feb 19, 2020

Configure with --enable-rust. Here's how to use:

$ ./bitcoin-cli help
...
 == Lightning ==
lnclosechannel "channel_id"
lnconnect "node"
lncreateinvoice "description" amount
lnfundchannel "node_id" amount
lngetchannels
lngetpeers
lnpayinvoice "invoice"
...

Connect to ACINQ testnet node:

$ ./bitcoin-cli  lnconnect 03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134@34.250.234.192:9735

See that we are connected:

$ ./bitcoin-cli lngetpeers 
[ 
  { 
    "id": "03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134"
  } 
]

Check if we have enough in our wallet:

$ ./bitcoin-cli getbalances 
{ 
  "mine": { 
    "trusted": 0.01000000, 
    "untrusted_pending": 0.00000000, 
    "immature": 0.00000000 
  } 
}

Fund a 0.001 tBTC channel with our peer:

$ ./bitcoin-cli lnfundchannel 03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134 0.001

$ ./bitcoin-cli lngetchannels 
[ 
  { 
    "id": "09e43999ea4fc33c0c8aa134727ea17ac1e2ac7e9356b1434fed9bfc8f091e1f", 
    "shortid": 0, 
    "capacity": 100000, 
    "status": "unconfirmed" 
  } 
]

Wait a couple of blocks and try again:

$ ./bitcoin-cli lngetchannels 
[ 
  { 
    "id": "09e43999ea4fc33c0c8aa134727ea17ac1e2ac7e9356b1434fed9bfc8f091e1f", 
    "shortid": 1831367457947189249, 
    "capacity": 100000, 
    "status": "confirmed" 
  } 
]

$ ./bitcoin-cli getbalances 
{ 
  "mine": { 
    "trusted": 0.00896940, 
    "untrusted_pending": 0.00000000, 
    "immature": 0.00000000 
  } 
}

Our channel is operational! Immediately go and purchase some beers on yalls.org:

$ ./bitcoin-cli lnpayinvoice lntb1500n1p0yxfm6pp5098ss9punvma2xusvdez40dnrnrr7zlf2hpqm5kvnc4g9fjk7pjqdp6g9jxggrjv4skxarfdahzpuyl3kazqar0ypuk7atjyp3k7mrvv43hg6t0dccqzpgxqr23ssp5kjpejv09t6yweaf02x7ywzpdr3z8utpnyw67x9lcvfuf6f6kwdns9qy9qsqx323heyl6e54cmrxn9gnuqsef7juacsl7w7cngvrvg2vft6qtll5f7c07vx004n62uwklnhfwd62tnjzw30gt5ucsz32yz3j6uanvhqqqgpkru

Close the channel:

$ ./bitcoin-cli lnclosechannel 09e43999ea4fc33c0c8aa134727ea17ac1e2ac7e9356b1434fed9bfc8f091e1f

Hot on the heels of rust-lightning 0.0.10 release and the LDK announcement I present you CoreLightning.

This PR is based on #17376 and it adds 4 more crates to the mix:
rust-lightning, rust-lightning-invoice, rust-bech32-bitcoin and num-traits. Peers and channels data is in lightning. Code will be impossible to review on GitHub due to submodule mess but for now this is a one-commit PR anyway so take a look there.

Even though this is nowhere near production ready I'm excited to see it work. Credit to @TheBlueMatt and the rust-bitcoin/lightning team. Truly awesome work.

For the sake of argument let's say everyone agrees that Lightning is a good thing and helpes Bitcoin scale. In that case I see built-in CoreLightning as useful in two ways:

  • Makes it easy for the Core install-base to adopt Lightning in one fell swoop. People will be more likely to use and build on top of Lightning if it's one upgrade away. You can use your existing wallet funds. No separate daemons or needless transactions. Institutional users such as exchanges would have no excuse not leverage Lightning.
  • I feel that Core could be more of a universal client in the future. It's somewhat industrial nowadays but I'd like 2021 to be the year of Core on the Phone 😎. I wrote about this a bit here.

Code is not very robust, it needs a lot of love in regards to making it more "rusty" and figuring how to better manage the C++/Rust FFI boundary. Before I invest any more time in this I'd like to get some guidance and concept (N)ACKs.

JeremyRubin and others added 24 commits Mar 29, 2019
…st code.

The demonstration library compiles a rust hello world example and auto-generates
a header which can be included in C++ code.

Co-Authored-By: Jeremy Rubin <j@rubin.io>
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
Various changes by: Matt Corallo <git@bluematt.me>
Also, break circular dependency with a new helper lib courtesy of
Cory Fields <cory-nospam-@coryfields.com>.
This starts the process of moving the last bits of
validation-critical logic out of net_processing - the fallback
logic of which blocks to download is incredibly critical to
validation and would likely also be used for any parallel block
sync systems. Further, assumeutxo will require additional ability
to select blocks for download, which is a validation-specific
concept and shouldn't really live in net_processing.

This moves a few responsibilities to the FindNextBlocksToDownload
callsite instead of passing in a CNodeId, but otherwise has no
functional changes.
This makes it more general than just net_processing
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.

Places pindexBestHeader is used:

 * Various GUI displays of the best header and getblockchaininfo["headers"],
   I don't think changing this is bad, and if anything this is less confusing
   in the presence of an invalid block.
 * IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
   isn't some crazy invalid chain is better than the alternative, even in the
   case where you are rejecting the current chain due to hardware error (since
   hopefully in that case you won't get any new blocks anyway).
 * ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
   block we're connecting leads to something with nMinimumChainWork (preventing
   a user-set assumevalid from having bogus work) and that the block we're
   connecting leads to pindexBestHeader (I'm not too worried about this one -
   it's nice to "disable" assumevalid if we have a long invalid headers chain,
   but I don't see it as a critical protection).
 * BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
   requested block is within a month of the "current chain". I don't think this
   is a meaningful difference, if we're rejecting the current tip we're
   trivially fingerprintable anyway, and if the chain really does have a bunch
   of invalid crap near the tip, using the best not-invalid header is likely a
   better criteria.
 * ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
   of whether a block request is "historical" for the purpose of bandwidth
   limiting. Similarly, I don't see why this is a meaningful change.
 * We use pindexBestHeader for requesting missing headers on receipt of a
   headers/compact block message or block inv as well as for initial getheaders.
   I think this is definitely wrong, using the best not-invalid header for such
   requests is much better.
 * We use pindexBestHeader to define the "current chain" for deciding when
   we're close to done with initial headers sync. I don't think this is a
   meaningful change.
 * We use pindexBestHeader to decide if initial headers sync has timed out. If
   we're rejecting the chain due to hardware error this may result in
   additional cases where we ban a peer, but this is already true, so I think
   its fine.
Note that we have to use LTO to work around the missing der_lax
symbol in rust-secp256k1.
See rust-bitcoin/rust-secp256k1#168
CoreLightning is a rusty interface to Lightning Network
It uses the rust-lightning library
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Feb 19, 2020

While this is an impressive piece of work, I don't think it makes sense to add layer 2 functionality to this project. Using the same repository, the same codebase, will automatically make reviews harder and will increase the risk of introducing critical bugs that impact all areas of the software.

Instead of clutching everything together, we should try to achieve process separation (wallet / node / validation / GUI / etc.).

What are the benefits of this over a separated Bitcoin-Core & c-lightning (or any other ln daemon)?
If the user interface should be improved, one could developer a new type of a bitcoin-cli (a command-line client) that combines the calls to Bitcoin Core and a lightning daemon.

@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Feb 19, 2020

Thanks @icota for this cool PoC, that's really impressive but I do share Jonas opinion here. Better would be to spend time on process separation/interface cleaning/reusable library than pouring more stuff in one repository which would slow down development process of every component involved. It would be great for bitcoin ecosystem to reuse Core components like wallet utxo-management or fee-bumping if we turn them more library-like a la rust-lightning and avoid a lot of reinventing the wheel again and again. It's true they aren't easy piece of software to get right.

Enforcing a specific runtime on users is a hazardous move given the huge diversity of use-cases. A mobile wallet is easier to build as one monolithic process whereas exchange maybe interested to have a micro-services approach for high-availability. And there is also resources-constrained, embedded devices where you're not going to run a full bitcoin stack on it. Running a full-node is already bandwidth-expensive in a lot of places, not even mentionning mobile data plan. So IMO modularity is a better way to serve a broad, diverse base of users/requirements.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 19, 2020

What are the benefits of this over a separated Bitcoin-Core & c-lightning (or any other ln daemon)?

Presumably one hypothetical benefit is not having a separate wallet.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Feb 19, 2020

Assume you were to rebase this on top of @ryanofsky's process isolated wallet, could you keep all of the rust integrated stuff in the wallet process? Can the rust ln networking stuff be running a separate process from most of the wallet stuff?

I think it would be a pretty decent design that you can offer a wallet module compiled with or without lightning, and then the "core node" software (which should have a minimum amount of stuff) can be just about validation.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 20, 2020

Can you split the rust-lightning code out of this, at least? Check for it in configure and dynamic link to its library...

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 20, 2020

I'm very divided about this. I's impressive work, but also, I don't want this project to compete with c-lightning, lnd et al. It's their own specialty, they do what they do well, and maintaining the core validation layer is enough work for us. There shouldn't be more going through the bottleneck of bitcoin core review.
(also there's the more abstract but not less important concern of layer 2 being permissionless-should "we" choose lightning as "winner", should we integrate every possible, mature, layer 2 then? I've always been in favor of limiting the scope of this project, not extending it)

I'm really sorry to say this after you did all this work.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 20, 2020

Presumably one hypothetical benefit is not having a separate wallet.

Presumably, if people want to pursue advanced mobile wallet functionality (I heard about Bluetooth integrations, or device power management, NFC and now L2 integration)—this is all very cool, but I think we need to limit the scope of this project, these are so far from block/transaction validation, the critical mission of this project. So bitcoin core's wallet would have to split to a separate repository with different maintainers.

(it can still be distributed together with the node, so from the perspective of an end-user not much would change, but avoids maintenance headaches, and the risks of complex, highly coupled systems, like we don't want some wallet commit adding some shiny new feature to accidentally break validation)

@icota

This comment has been minimized.

Copy link
Contributor Author

icota commented Feb 20, 2020

Thank you all for the feedback, I really appreciate it.

What are the benefits of this over a separated Bitcoin-Core & c-lightning (or any other ln daemon)?
If the user interface should be improved, one could developer a new type of a bitcoin-cli (a command-line client) that combines the calls to Bitcoin Core and a lightning daemon.

@jonasschnelli To open a channel in your scenario we are doing two additional steps (newaddr and sendtoaddress). Built-in CoreLightning approach saves the user time and money since we are broadcasting only one transaction instead of two. Huge UX win in my opinion.

Enforcing a specific runtime on users is a hazardous move given the huge diversity of use-cases. A mobile wallet is easier to build as one monolithic process whereas exchange maybe interested to have a micro-services approach for high-availability.

@ariard I absolutely agree with this. I don't believe in a one-size-fits-all approach. I am familiar with @ryanofsky and your work on process separation. But crucially multiprocess is optional at compile time and one can still build a monolithic binary.

I say crucially because, on mobile, monolithic is the only way to go. Multiple processes and IPC pose huge challenges on Android and this situation is not likely to improve (iOS is completely locked down). Take a look at this c-lightning issue and the following comments.

Long story short mobile OSes want to manage the lifecycle of the apps. Spawning processes and various IPC goes against that grain.

Assume you were to rebase this on top of @ryanofsky's process isolated wallet, could you keep all of the rust integrated stuff in the wallet process?

@JeremyRubin I'm not entirely sure. Is tx broadcast and notifying of new blocks part of the wallet process? Right now division of labour is like this: Rust code is dealing with Lightning only and Core does everything else.

Having said all that I realise now that I got carried away a little bit. I was following work on #16834, #17376, #16762 and I lost sight that while fetching headers and blocks is a core (heh) competency of Core - Lightning is probably not.

But before I close this I'd like to take the opportunity to propose a way forward.

What about having CoreLightning as a separate library in its own repository? This way at configure time one could decide to link against it and the only thing needed on the Core side would be the rusty interface to stuff lightning needs like signing and broadcasting tx, fee estimation...

Can you split the rust-lightning code out of this, at least? Check for it in configure and dynamic link to its library...

@luke-jr It would have to look for much more than rust-lightning so I think the above approach is more appropriate.

I'm really sorry to say this after you did all this work.

@laanwj I really appreciate the sentiment but don't be sorry. This was a great learning experience for me and I learned a ton. I would even go so far to say I had fun. 😮

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Feb 20, 2020

I agree with @laanwj and most of the other commentors here. These comments:

Presumably, if people want to pursue advanced mobile wallet functionality (I heard about Bluetooth integrations, or device power management, NFC and now L2 integration)—this is all very cool, but I think we need to limit the scope of this project, these are so far from block/transaction validation, the critical mission of this project. So bitcoin core's wallet would have to split to a separate repository with different maintainers.

came from a discussion me and @laanwj had after I took at look at @icota's "mobile roadmap".

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Feb 20, 2020

Can you make this repository a submodule? If that works, then hopefully you only need some modest changes in this repo, e.g. a way to insert RPC methods, or helper classes like BlockProviderState. That way you get to have a monolithic app for mobile.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 20, 2020

@luke-jr It would have to look for much more than rust-lightning so I think the above approach is more appropriate.

It's not. Besides, what else would it need to look for? If rust-lightning itself has dependencies, consumers of rust-lightning shouldn't need to care...

@ariard

This comment has been minimized.

Copy link
Contributor

ariard commented Feb 20, 2020

I say crucially because, on mobile, monolithic is the only way to go. Multiple processes and IPC pose huge challenges on Android and this situation is not likely to improve (iOS is completely locked down). Take a look at this c-lightning issue and the following comments.

IMO the way to go is first to make Core wallet running as a standalone process, then when it's well-done you can start to think about runtime integration with other bitcoin protocol stack like LN or Coinjoin feature-gated behind compilation flags. But anyway it should wait for core wallet being in its own repository, to avoid pouring any more burden maintenance/review on this project.

@icota

This comment has been minimized.

Copy link
Contributor Author

icota commented Feb 20, 2020

Besides, what else would it need to look for? If rust-lightning itself has dependencies, consumers of rust-lightning shouldn't need to care...

rust-lightning-invoice is another dependency (of this PR). But okay, two dependencies are not a big deal.

Can you make this repository a submodule? If that works, then hopefully you only need some modest changes in this repo, e.g. a way to insert RPC methods, or helper classes like BlockProviderState. That way you get to have a monolithic app for mobile.

That is one possiblity. I'm leaning more toward librarising bitcoind in some way, in addition to subverting RPC. Helper classes are probably not needed, I'd guess RPC already has everything required.

"Core Mobile" with its flashy UI links against a hypothetical "libbitcoind" an does an internal RPC dance. Power management, other mobile garbage and lightning is in that separate repository. Disruption and maintenance burden to this repository is kept to minimum. Everybody wins.

Following a conversation with @fanquake today I will open an issue in the coming days where we can brainstorm this and other approaches to mobile and lightning.

I'm closing this ill-concieved PR. 🔐

@icota icota closed this Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.