lightningdevkit / rust-lightning Goto Github PK

This should allow implementors to easily compare the list of open channels with list of connected peers and connect to any missing peers (by quering the Router's database of node_id -> addresses mapping).

Need to investigate exposing enough to let clients implement DNS querying for address bootstrap without tracking all the details of BOLT 10.

We should see if we can take advantage of the borrow checker to drop a bunch of Arcs and hold references in a number of places, may require #80, though. Should also look into nice things like dropping the secp_ctx in Channels and holding a reference to the ChannelManager one, too.

Just adding this issue for an obvious TODO so I can assign myself.

Currently we require users to interrogate ChannelManager for info on non-public local channels, which makes it particularly hard to comply with BOLT 7 "SHOULD accept channel_updates for its own channels (even if non-public), in order to learn the associated origin nodes' forwarding parameters." I think the obvious solution is to track local channels in Router.

Tagging 0.1 so I can call BOLT 7 completed in #129 "with the exception of other 0.1-tagged issues"

Per
https://github.com/lightningnetwork/lightning-rfc/blame/4b62d26af93a07166d6a9de2cb5eff80849d9c19/02-peer-protocol.md#L172,

temporary_channel_id is required to be unique only within the same peer.

Then, this seems too restrictive:
https://github.com/rust-bitcoin/rust-lightning/blob/f975639d036b08706fc007aa327bffbc0a5f217b/src/ln/channelmanager.rs#L1265-L1267

In the spec,

channel_reserve_satoshis is the minimum amount that the other node is to keep as a direct payment.

And, as my understanding, this means it's the value that the node receiving Open/AcceptChannel should keep.

We do channel.thier_channel_reserve_satoshis = msg.channel_reserve_satoshis
So channel.their_channel_reserve_satoshis is used when checking conditions for our channel reserve and I found it pretty confusing.

Perhaps,
channel.our_channel_reserve_satoshis = msg.channel_reserve_satoshis
and
Channel::get_their_channel_reserve_satoshis is better?

Was thinking of the new TODO in channel about optional additional constraints, the user should be able to configure them easily if not opting for default policy. So need a Config interface we can pass everywhere, with a lock, and that can be updated on-fly

I presume this would be a HandleError action specific to particularly-bad peers, but may need some kind of tracking of how much a peer is doing something bad, and also need to keep score by remote address, not just pubkey.

The bitcoind's estimatesmartfee method needs a conf_target parameter describing the numbers of blocks the TX should wait, it's the same with btcd's "getinfo" method. Shouldn't ConfirmationTarget have a method called to retrieve the number of blocks desired to wait?

It should fail the channel when message handling routines return Err and optionally send an error message to the peer, but it currently doesn't.

//TODO: Handle errors and close channel (or so)

This issue tracks BOLT #9: Assigned Feature Flags, this here so we can check all requirements.

• option_data_loss_protect
• initial_routing_sync
• option_upfront_shutdown_script
• gossip_querie

Currently, the transactions fees are not excluded from the funder's value_to_self_msat before being compared to channel reserve when sending/receiving add_update_fee

Follow up on #189
See TODO in TheBlueMatt@1f31e03

https://gist.github.com/yuntai/9ddc6349df50fcb63c4f4a4dbf5f9484

Not sure it's okay to push this here in the issue list, but wanted to share this dump-out I created for HTLC switching in the payment process across three nodes.
The transcript shows events, messages exchanged and commitment txs. Message() shows the sighash of commitment txs.

At least it was very helpful for me to understand things so I thought it might be so also to other new-comers.

ChainWatchInterface offers filter by spent outpoint. These are no longer available in proposed BIP158 base filter. Instead only filter by spent script possible. Please change interface otherwise the lightning node will depend on full nodes not using filters.

We panic in the channe.rs:1814 panic after failing the self.channel_state < ChannelState::FundingCreated as u32 check in channel_monitor(&self) when called from channelmanager's block_connected if channel.channel_monitor().would_broadcast_at_height(height) { line. Just filing an issue and not fixing yet (which is an obvious fix) cause I want to wait and see if the new fuzz testing changes I have pending will catch this first (they should, pretty trivially).

Need to make sure we're compliant with all the various actions we're supposed to take as specified in the BOLTs, as well as ensure ErrorActions are always Some() in HandleError. Some of these should be pretty easy, some will be way more involved.

• BOLT 1
• BOLT 2
• BOLT 3
• BOLT 4
• BOLT 5
• BOLT 7
• BOLT 8
• BOLT 9

BOLT 10 we don't have any implementation of yet (#58), and BOLT 11 is in rust-lightning-invoice.

There's one or two TODOs in ChannelMonitor to handle all the listed cases, plus some TODOs in channel-force-close in ChannelManager to update ChannelMonitor, and the test in ChannelManager which construct networks, do various things to them, and then check monitor validity should be fleshed out way way more.

In writing https://github.com/TheBlueMatt/rust-lightning-bitcoinrpc I almost misused ChainWatchInterface, almost calling ChannelManager::block_connected and PeerHandler::block_connected instead of only calling ChainWatchInterfaceUtil::block_connected (which will call the others for you).

I'm not actually sure what a better interface looks like. I played around a bit with making some things pub(crate) in chaininterface but couldn't come up with anything I actually liked, sadly.

...is almost entirely unhandled.

Generally, we need the final claim keys (ie channel_monitor_claim_key and channel_close_key) to be provided by the user (probably derived in some reliable way) and the rest are really our internal keys but should be generated from a secret the client gives us.

Right now we just download a full route db dump from the first few nodes we connect to. This works but is really, really bad. We should be way smarter, as well as persist route db to disk.

Option_data_loss_protect is currently not implemented as mentioned in Bolt 2

Hello @TheBlueMatt et al.,

Nice project, I really wanna see the LN daemon written in Rust happen and I wondering if there's a some kind of list of long- and short-term goals (or TODOs) for this particular project so new contributors (and me, hopefully) who know Rust and something about Bitcoin/Cryptography/Distributed would put their hands on and make a contribution.

On the first glance it's kinda hard to see what is already implemented and what should be (for example, list of BOLT's and subsequent tasks/challenges that are implemented/done already and what aren't).

We need to convert the few printlns we have to some kind of user-provided logging interface, as well as add a ton of additional logs. I think its fine if we just make it another Arc<trait> that gets passed around everywhere and we call it directly with lines. Would be nice to avoid the format!() overhead when we dont need it, so maybe we can provide build options to turn off debug logging and avoid most of it?

Problem is ChannelMonitor become out-of-date due to different reasons (reorg, revocations, close, ...). We should get them out of SimpleManyChannelMonitor when it's accurate.

I'm afraid that we need to keep all previous state until channel close, I mean that's a good component of LN security model..

As suggested by BOLT 7 in "Recommendations for Routing" to avoid making it obvious which hops a route is taking.

I was looking into doing this as an easy starter task, but had some questions. https://github.com/rust-bitcoin/rust-lightning/blob/master/src/util/transaction_utils.rs

1. re: the TODO to make static and put in utils (+ sort inputs).

• not sure what "make static" would mean for a standalone function? I'm new to rust so probably overlooking some concept.
• it seems to already be in a utils file, and is called as transaction_utils::sort_outputs. Is there anything else to do (except implement sort_inputs)?

2. re: the TODO for ordering of scripts shouldn't be len-based.

• If not len-based, then what should the ordering be based on?
• I can poke around at other implementations to see what they do, but figured I'd ask first.

The signature from the funder doesn't seem to be saved anywhere so if the push_msat is sizeable the fundee miss out a good payout?

close_channel() expects channel_id, but channel_id is never reported to the client currently. If user_id is unique for each open_channel(), we can use it instead, but it doesn't apply to the fundee side as the user_id will always be zero. I think we can make another API event, ChannelEstablisehd { user_id, channel_id }.

Probably needs at least #58 so we can bootstrap, but once we have a node db, figuring out some "suggested channels" for clients would be really cool.

We currently drop all error messages, which we (a) shouldn't do (we need to log them) and (b) shouldn't do (need to close the channel referenced in the error message).

I see a lot of try! in the code, is there a reason to prefer it over ?

While reviewing, I had a doubt, where do we implement initiation of our own of removing timeout incoming HTLCs ? Seems not to be present in any block_connected callbacks right ?

In several places...good first issue cause defining a trait for it and using that to emulate current behavior should be easy, but doing more may take effort.

See further #151

Add RemoteDOSError and maybe others kind of log levels for logs which can be triggered in an infinite quantity by a remote peer. Add also warning comments for logger implementers on how to avoid (rate-limit it, ring-buffer)
#rust-bitcoin 16 Aug 2018 22:52

There's a bunch of unimplemented!()s for this right now, so should be obvious where it goes, but implementing the put-channel-on-ice logic may not be trivial, needs investigation.

Additional payloads for some failure codes are not reported back (e.g. htlc_msat for amount_below_minimum)
And some error checkings are missing - amount_below_msat, expiry_too_soon, expiy_too_far .

We should start requiring full mutation coverage for functions as we build out our unit tests.
https://github.com/llogiq/mutagen looks like a good candidate, unless @apoelstra thinks we should use https://github.com/apoelstra/halfsleep.
Tagging 0.2 as our goal for 0.2 should be to move more towards better test coverage.

We currently implement update_fee by just updating the local fee, but it needs to do a full commitment dance to get updated (ie we need to store a previous one and a commitment sate of the new one). We also need to implement sending update_fees.

https://github.com/rust-bitcoin/rust-lightning/blob/master/src/ln/router.rs#L356-L364

some questions:

1. which callsite(s) would invoke this function? trying to see how to test it.
2. where should we store the state for marking it bad?

• I'm thinking struct NodeInfo. We can add a field that stores a timestamp for when it was marked bad, which will let us calculate the exponential decay. Let me know if you have other thoughts.

cc @TheBlueMatt

This was discovered (and documented) in #208, but its a really terrible API, so should be fixed. I suppose the easy way to do so is to generate these events in response to the commitment_signed dance instead of the initial update_fulfill_htlc message, which I guess is OK, though would be nice to tell the user faster.

Follow up on #187 and #200.

Implement this from the spec

if the sender cannot afford the new fee rate on the receiving node's current commitment transaction:
SHOULD fail the channel,
but MAY delay this check until the update_fee is committed.

I wonder we need user-facing API to update relay parameter for

• cltv_expiry_delta
• htlc_minimum_msat
• fee_base_msat
• fee_proportional_millionths

and fire channel_update message (if there is actual update). (related to #169?)

While writing test code for #157, I found that it'd be much easier (and more realistic) to actually change channel config to simulate error condition.

Right now all three are used interchangably in a few places, which is nonsense and just leads to confusion. Probably switch to Sha256dHash when things are actually Sha256dHashes (mostly block hashes and txids) and use our own types for most other things, or just [u8; 32] when it makes sense.

Option_upfront_shutdown_script as specified in Bolt 2 is currently not implemented

As of #167 we track HTLCs separately as required by BOLT 2 for privacy. However, we currently reject/accept all to-us HTLCs at once when the user gives us a preimage/no-such-preimage callback. This allows an attacker to (in a really race-y way) query us if we are the intended recipient of a payment that was routed through them by simply sending us a small value HTLC with the same hash as the one they saw. This will likely require an API change, and users may end up needing to do something like tracking the expected value of a payment and only fulfilling HTLCs that match the expected value (to the single-msat level).

The gossip_queries stuff is overcomplicated, requires zlib, and is hopefully gonna be replaced sooner or later, so probably dont want to bother implementing that, but we should at least support the initial-connection routing table dump in initial_routing_sync.

This is obviously required for #55 to be useful, but also we have to handle this stuff for simple TCP loss as well.