Skip to content

pay: Exempt a fee from the maxfeepercent rule it is still tiny#1765

Merged
rustyrussell merged 3 commits into
ElementsProject:masterfrom
cdecker:issue-1746
Jul 30, 2018
Merged

pay: Exempt a fee from the maxfeepercent rule it is still tiny#1765
rustyrussell merged 3 commits into
ElementsProject:masterfrom
cdecker:issue-1746

Conversation

@cdecker
Copy link
Copy Markdown
Member

@cdecker cdecker commented Jul 27, 2018

Several users have noticed that they cannot pay satoshis.place or similar places
that have tiny payment amounts if they are not directly connected. This is due
to the forwarding fee dominating the transferred amount.

This commit adds a new option, exempting tiny fees (up to 5 satoshis by default)
from having to pass the maxfeepercent flag. While we could have told users to
tweak maxfeepercent I think it is usefull to have a default exemption.

Not sure about the name of the option just yet, the current naming is based on
exempting it from the check, which feels clunky. Maybe someone else has a
good idea?

Fixes #1746

Ping @SerafinTech, @renepickhardt, @jonathancross

Comment thread lightningd/payalgo.c
p_opt_def("retry_for", json_tok_number, &retryfor, 60),
p_opt_def("maxdelay", json_tok_number, &maxdelay,
cmd->ld->config.locktime_max),
p_opt_def("exemptfee", json_tok_number, &exemptfee, 5000),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that also make the argument available as an option in the config file?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This only deals with json parameters. config and command line parameters are handled in lightningd/options.c.

@renepickhardt
Copy link
Copy Markdown
Collaborator

thanks for pinging me! that really helped (especially after I already started looking at pay.c and payalgo.c. besides not being able to judge for side effects (which I find highly unlikely this particular change) this patch looks valid to me.

Comment thread lightningd/payalgo.c Outdated
* cast u64 to double for feepercent calculation. */
feepercent = ((double) fee) * 100.0 / ((double) pay->msatoshi);
fee_too_high = (feepercent > pay->maxfeepercent);
fee_too_high = (fee <= pay->exemptfee && feepercent > pay->maxfeepercent);
Copy link
Copy Markdown

@SerafinTech SerafinTech Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fee_too_high = (fee > pay->exemptfee && feepercent > pay->maxfeepercent);

fee_too_high should only be true if the fee is more than the pay->exemptfee

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh 👍

Several users have noticed that they cannot pay satoshis.place or similar places
that have tiny payment amounts if they are not directly connected. This is due
to the forwarding fee dominating the transferred amount.

This commit adds a new option, exempting tiny fees (up to 5 satoshis by default)
from having to pass the maxfeepercent flag. While we could have told users to
tweak maxfeepercent I think it is usefull to have a default exemption.
@wythe
Copy link
Copy Markdown
Contributor

wythe commented Jul 27, 2018

Also add the description to the static const struct json_command pay_command help.

Copy link
Copy Markdown
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 252203d

@rustyrussell
Copy link
Copy Markdown
Contributor

Ack dddf5ca

@rustyrussell rustyrussell merged commit 55d450f into ElementsProject:master Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants