Add optional user specified channel limits of bolt 2 #169

SWvheerden · 2018-09-12T10:13:02Z

Added a config struct to use when passing configurations into the channel.

Before accepting a channel the channel will test optional limits as specified by BOLT 2.
The default values of these test are set to not test the limits, but the user can supply other values.

TheBlueMatt

There's a few whitespace things I didn't mark, but if you do git show it should highlight them for you. Also, best to avoid merge commits in pull requests, its easier to keep track of where things are in the eventual git log if you rebase instead.

src/util/mod.rs

src/util/configurations.rs

src/util/mod.rs

src/ln/router.rs

src/ln/channelmanager.rs

src/ln/channel.rs

src/util/configurations.rs

TheBlueMatt

Needs rebase. Note that you may also want to set your editor to highlight extra whitespace at EOLs, since you've got that in a few places, though I'm generally fine cleaning up whitespace errors during merge.

src/ln/channel.rs

fuzz/fuzz_targets/channel_target.rs

src/ln/channelmanager.rs

TheBlueMatt · 2018-09-29T19:27:58Z

Why not something like TheBlueMatt@96889fd instead of all of the per-Channel storage just to get the announced_channel flag?

SWvheerden · 2018-10-01T05:36:54Z

mmm that could work as well. This is why I put the annouce channel and fee_proportional_millionths in the config: #169 (comment)

I liked the idea of storing as much as possible in a "config" as I believe this makes the code easier to read, but both will work just as well.

But not even storing a reference to the limits removes the issue of using an ARC or lifetimes which solves one problem we have.

SWvheerden · 2018-10-15T11:59:19Z

#129

TheBlueMatt

There's a bunch of out-of-date docs in configurations.rs that should be rewritten for the latest version of this PR. Also, it may make sense to shorten some names as Configuration/Configurations in needlessly long - Config is equally descriptive and much shorter, generally.

src/ln/channelmanager.rs

src/util/configurations.rs

src/util/mod.rs

src/util/configurations.rs

src/ln/channelmanager.rs

TheBlueMatt · 2018-10-19T18:08:09Z

src/ln/channel.rs

 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
 // inbound channel.
 pub(super) struct Channel {
+	pub config :  ChannelConfig,


Hmm, I feel awkward making this pub - we cant change announce_publicly at runtime and it would also be the only field not hidden behind accessors.

src/ln/channel.rs

src/ln/channelmanager.rs

TheBlueMatt

Cool, code looks good, but the docs here aren't really at all informative, they read more just like "had to add a doc, so wrote the variable name" in a few places, whereas especially for configuration nobs we really need to be descriptive and tell users what is going on.

src/util/mod.rs

src/util/configurations.rs

TheBlueMatt · 2018-10-31T19:02:38Z

Will take as #234.

TheBlueMatt reviewed Sep 12, 2018

View reviewed changes

TheBlueMatt reviewed Sep 14, 2018

View reviewed changes

src/ln/channel.rs Show resolved Hide resolved

src/ln/channel.rs Outdated Show resolved Hide resolved

fuzz/fuzz_targets/channel_target.rs Outdated Show resolved Hide resolved

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch from 653f418 to 9a3e171 Compare September 17, 2018 06:35

TheBlueMatt reviewed Sep 18, 2018

View reviewed changes

src/ln/channelmanager.rs Outdated Show resolved Hide resolved

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch from f423a15 to 99b1c24 Compare September 19, 2018 09:15

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch 5 times, most recently from ecaa1a2 to 86c84ed Compare September 26, 2018 13:31

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch 4 times, most recently from 7b75169 to 6c72a35 Compare October 2, 2018 08:09

SWvheerden mentioned this pull request Oct 15, 2018

Work through each BOLT and fill out error conditions #129

Closed

8 tasks

TheBlueMatt reviewed Oct 16, 2018

View reviewed changes

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch from 6c72a35 to 0bb08f5 Compare October 17, 2018 06:54

TheBlueMatt reviewed Oct 18, 2018

View reviewed changes

src/util/configurations.rs Outdated Show resolved Hide resolved

src/ln/channelmanager.rs Outdated Show resolved Hide resolved

SWvheerden changed the title ~~Add optional user spesified channel limits of bolt 2~~ Add optional user specified channel limits of bolt 2 Oct 19, 2018

TheBlueMatt reviewed Oct 19, 2018

View reviewed changes

yuntai mentioned this pull request Oct 20, 2018

Add API to change HTLC relay parameters #216

Closed

TheBlueMatt reviewed Oct 23, 2018

View reviewed changes

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch 5 times, most recently from 26a8e5f to 39c2e7b Compare October 26, 2018 06:04

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch 4 times, most recently from b163b89 to 17b7611 Compare October 30, 2018 11:49

Added config file to allow users to specify channel limits as per bolt 2

b57407a

SWvheerden force-pushed the add-optional-user-spesified-channel-limits-of-BOLT-2- branch from 35b1eb2 to b57407a Compare October 31, 2018 11:03

TheBlueMatt mentioned this pull request Oct 31, 2018

169 Tweaked #234

Merged

TheBlueMatt closed this Oct 31, 2018

SWvheerden deleted the add-optional-user-spesified-channel-limits-of-BOLT-2- branch November 1, 2018 10:09

Add optional user specified channel limits of bolt 2 #169

Add optional user specified channel limits of bolt 2 #169

Uh oh!

Conversation

SWvheerden commented Sep 12, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Sep 29, 2018

Uh oh!

SWvheerden commented Oct 1, 2018

Uh oh!

SWvheerden commented Oct 15, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Oct 31, 2018

Uh oh!

Uh oh!