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

Allow setting feerate on per-channel basis #2342

Merged
merged 6 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@m-schmoock
Copy link
Collaborator

commented Feb 11, 2019

PR for issue #2316

Progress:

  • Add db fields for per-channel feebase + ppm
  • Put these fields into struct channel
  • Use the fields in forward_htlc instead of global values
  • Show fields in listchannels
  • Plumb db fields through to gossipd for advertising.
  • Provide API eg. setchannelfee id <base> <ppm>
  • Create tests, CHANGELOG.md, man page.
  • Dance a happy dance.

Currently workin on: DONE! thank you all

Maybes:

  • Maybe constraint checks / warning when trying to set too high fees? If yes, which exactly?
  • Maybe allow wildcard '*' or 'all' as channel id in json command to set fees for all channels. Note: will be separate PR and is already done.

Issues:

  • None

Notes:

  • JSON: Leaving out values for ppm/base values will re-apply global config values.
  • COMMON: I added some amount_msat function in order to check and transform them to u32, if not wanted, I need other access method, or pointer magic, or use old style u32 number.
  • DB: the fix #2444 to the issue #2442 now raises a valgrind complaint which cause the travis to be red. See #2449 for details.

@m-schmoock m-schmoock requested a review from cdecker as a code owner Feb 11, 2019

@m-schmoock m-schmoock changed the title Allow setting feerate on per-channel basis #2316 Allow setting feerate on per-channel basis Feb 11, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch 4 times, most recently from 055d897 to 8a9a76e Feb 11, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from 8c0faad to c268ab7 Feb 19, 2019

@m-schmoock m-schmoock referenced this pull request Feb 20, 2019

Closed

Allow setting feerate on per-channel basis #2316

0 of 8 tasks complete
@cdecker
Copy link
Member

left a comment

Excellent work @m-schmoock, you picked up a lot of the c-lightning peculiarities right away. As is usual for us when a new contributor submits their first substantial PR I'm going through the commits with a fine-toothed comb and nag about every tiny detail, so don't be discouraged by the sheer size of the comments :-)

I'm about half-way through, but thought I submit what I have so far.

Overall the PR could use some more structure. I like that you laid out the work ahead in the form of comments, and bit off incremental chunks, but in many places you changed something back which you just introduced a few commits ago. I personally build up my PRs using git commit --fixup= to amend a commit that I want to go back on and make it cleaner (I also make heavy use of git diff-blame to actually find out which commit should be fixed up). Alternatively Rusty uses guilt which allows pushing and popping commits and therefore moving back and forth to the place that needs to be amended.

Show resolved Hide resolved lightningd/opening_control.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved tests/test_pay.py Outdated
Show resolved Hide resolved lightningd/peer_control.c
Show resolved Hide resolved tests/test_pay.py Outdated
Show resolved Hide resolved channeld/channeld.c Outdated
Show resolved Hide resolved lightningd/channel_control.c Outdated
@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 20, 2019

Thx for the initial feedback, I just wasnt expecting you to go through each commit seperately, but discuss the open issues and do a diff with master and squash :D

Ill redo the branch so we do it a second time, otherwise you see bugs or misassumptions that I fix later which doesn't make sense incrementally ;)

@cdecker

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Thx for the initial feedback, I just wasnt expecting you to go to each commit seperately, but discuss the open issues and do a diff with master and squash :D

I find it easier to walk through the commits since it allows me to follow the rationale, rather than trying to split the final diff into logical chunks 😉

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from b366ace to e6b117c Feb 21, 2019

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

@cdecker can we do another try for the review? I reorganized the branch, rebased and tested each subrevision on the current master.

Now I also understand why we are doing it this way, by isolating each commit I found out that there were inconsistencies in old my changes.

Note: As I wrote at the top of the PR, I have open questions regarding min/max constraint checks and zero values...

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch 3 times, most recently from be69586 to ca406e1 Feb 22, 2019

@rustyrussell
Copy link
Contributor

left a comment

OK, I also went though this with a fine tooth comb, too. It's nice work! I think the magic 0 here is the main API quirk I'd like changed. The global values are nothing magic, and should not be treated specially IMHO. (I've been thinking recently that the default should be to make them slightly random!).

Show resolved Hide resolved lightningd/opening_control.c Outdated
Show resolved Hide resolved wallet/db.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved channeld/channel_wire.csv Outdated
Show resolved Hide resolved channeld/channel_wire.csv Outdated
Show resolved Hide resolved lightningd/peer_control.c Outdated
Show resolved Hide resolved lightningd/peer_control.c Outdated
Show resolved Hide resolved lightningd/peer_control.c Outdated
Show resolved Hide resolved doc/lightning-setchannelfee.7.txt Outdated

@rustyrussell rustyrussell added this to the 0.7.1 milestone Feb 23, 2019

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 24, 2019

@cdecker @rustyrussell Thanks for all your your input. I will come back with an update.

No Magic Zero
In order to support '0' fees without disabling it (which currently I also find very uncool), I think I need one of the following databse changes:

  1. Allow -1 in the database to indicate that channel specific fee is turned of, which is kinda iffy, but probably the least invasive solution.
  2. OR Add one or two bool (database Integers) that indicate the activation of channel specific fees. Two are needed if one of the args base/ppm can be optional, which I find good.
  3. OR Add one Integer field that is interpreted bitwise to check if channel specific values ought to be applied or not.

Return global values if unset/unchanged/defaulted
As written above, I need to add code to access global ln->config within the lightningd/peer_control.c. Should I add it?

Min/Max constraint checks
Should I add protection for the user against himself, trying to set too low/high values? If so, what values and should it just return a warning or prevent usage?

No specific fees, just initialize all channels after update with current global settings
As suggested by rusty we could initilize all channels base/ppm values to the current global settings on database migration instead of setting them to 0 or -1 (specific fees disabled). This would result in the situation that a change of the global settings will not affect any existing channels, is this really intended? Or do we want to have the option to disable channel specific fees again, i.e. by leaving out base/ppm value in setchannelfee. Since my PR was initially designed for the latter, I tend to have specific fees that can be disabled again.

Can you please give me an indication which of the solutions you find appropriate?

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch 9 times, most recently from b66ec30 to bc0b01b Feb 25, 2019

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from a2a10c4 to 255daeb Mar 8, 2019

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 8, 2019

I cherrypicked

fix: a pr2444 introduced valgrind complaint 4c0ecec
fix: also set testtub config for wallet test 145f910

To get around travis CI valgrind check. Probably not final. But rest is final.

@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 8, 2019

Looks like valgrind and all the tests are happy now, are you as well?
I can update the PR without the cherry picks (if merged via #2449) ...

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from 255daeb to aac64e5 Mar 8, 2019

@niftynei
Copy link
Collaborator

left a comment

couple formatting/spelling change suggestions. also noticed there's no tests for the channel in a state other than CHANNEL_NORMAL.

Show resolved Hide resolved lightningd/peer_control.c Outdated
Show resolved Hide resolved lightningd/peer_control.c
Show resolved Hide resolved tests/test_pay.py Outdated
Show resolved Hide resolved tests/test_pay.py Outdated
Show resolved Hide resolved tests/test_pay.py
@m-schmoock

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 8, 2019

@niftynei thanks for your input, I will push an update this weekend

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch 8 times, most recently from 03f7364 to 08af648 Mar 9, 2019

Show resolved Hide resolved lightningd/peer_control.c Outdated

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from 08af648 to 212ebbf Mar 14, 2019

m-schmoock added some commits Feb 21, 2019

adds: new db fields and struct variables
- Intrduce DB update `channel` values: `feerate_base` and `feerate_ppm`
- Make fist use of now context realted DB migration
- Add `struct channel` members of the same name
- Use struct values instead of config when commiting new channels
json: add cmd setchannelfee and wire to channeld
* adds the channeld_specific_feerates wire message 1029
* adds a json command handler
* adds u32 access methods for amount_msat
test: add tests for setchannelfee command
Goissp related tests are disabled for non-developers.
New setchannelfee testscases

* py.test -v tests/test_pay.py -k test_setchannelfee_usage
* py.test -v tests/test_pay.py -k test_setchannelfee_routing
* py.test -v tests/test_pay.py -k test_setchannelfee_state
* py.test -v tests/test_pay.py -k test_setchannelfee_zero
* py.test -v tests/test_pay.py -k test_setchannelfee_restart

@m-schmoock m-schmoock force-pushed the m-schmoock:feerate-per-channel branch from 212ebbf to c8b8f80 Mar 14, 2019

@rustyrussell
Copy link
Contributor

left a comment

Ack c8b8f80

void amount_msat_from_u32(struct amount_msat *msat, u32 value)
{
msat->millisatoshis = (u64)value;
}

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Mar 15, 2019

Contributor

We don't create these primitives for very good reasons; there are times to use direct access, but they should be carefully checked, hence the requirement to annotate them with an excuse. Otherwise people can start making mistakes with amounts (eg. doing arith on them!).

But I'm going to propose this change as a separate fixup, since I don't want to delay approving this excellent work further!

This comment has been minimized.

Copy link
@m-schmoock

m-schmoock Mar 15, 2019

Author Collaborator

Thanks. Yes, I was also not happy with that direct access.

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Ack c8b8f80

Ackbot, are you there? :)

@rustyrussell rustyrussell merged commit c7ab510 into ElementsProject:master Mar 15, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@m-schmoock m-schmoock deleted the m-schmoock:feerate-per-channel branch Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.