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.

Sign up for GitHub

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

Jump to bottom

Restore payment amount fuzzing #3212

Merged
merged 5 commits into from
Nov 8, 2019
Merged

Restore payment amount fuzzing #3212

merged 5 commits into from
Nov 8, 2019

Conversation

darosior
Copy link
Collaborator

niftynei
niftynei reviewed Oct 29, 2019
View reviewed changes
plugins/pay.c Outdated
@@ -1061,6 +1067,11 @@ static struct command_result *json_pay(struct command *cmd,
pc->msat = *msat;
}

if (*fuzzpercent > 0.0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we cap the max allowable fuzzpercent? 100% maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

though there's a comment on the old version of fuzzing that mentions that the total amount doesn't exceed the allowed limit for a payment. i wonder if this is still implemented also

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm yeah I wondered to make it a do {} while (overpayment > maxfeepercent * amount). But the default fuzzpercent <= maxpercent and since fuzzpercent and maxpercent are optional, I think someone providing them knows what they doing ?

@rustyrussell
Copy link
Contributor

I wonder if we should, instead, use the shadow route to fuzz? We use that to fuzz the cltv, should we use that to fuzz the amount?

I'd like to generally make our shadow route stronger, and this would be a good start...

@darosior
Copy link
Collaborator Author

darosior commented Nov 1, 2019 •
edited
Loading

I wonder if we should, instead, use the shadow route to fuzz? We use that to fuzz the cltv, should we use that to fuzz the amount?

Reading this, I said myself it would be harder to test , so it might be harder to analyze as well. I'll give it a try!

@darosior darosior force-pushed the amount_fuzzing branch 2 times, most recently from ac6705c to a1e551d Compare November 2, 2019 20:28
@darosior
Copy link
Collaborator Author

darosior commented Nov 2, 2019 •
edited
Loading

Did it through shadow routing, added a FIXME in pay because the cltv delta in route_info is an u16 (why?) and I didn't want to go down the rabbit hole to change it in this PR. Added an other one in the shadow routing test as testing the cltv delta needs the pay command to return it and this addition seems outside the scope of this PR.

@darosior darosior force-pushed the amount_fuzzing branch 7 times, most recently from 21e98c9 to e458272 Compare November 4, 2019 17:26
@darosior darosior changed the title Restore payment amount fuzzing [WIP] Restore payment amount fuzzing Nov 4, 2019
@darosior darosior changed the title [WIP] Restore payment amount fuzzing Restore payment amount fuzzing Nov 4, 2019
rustyrussell
rustyrussell reviewed Nov 5, 2019
View reviewed changes
Copy link
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.

Nice work, as always! Some minor changes, esp. with adding even more arguments to pay; I'm trying to avoid that as long as possible.

CHANGELOG.md Outdated Show resolved Hide resolved
plugins/pay.c Outdated Show resolved Hide resolved
plugins/pay.c
@@ -103,6 +103,9 @@ struct pay_command {
/* Any remaining routehints to try. */
struct route_info **routehints;

/* Disable the use of shadow route ? */
double use_shadow;

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this dev-only please? I don't want normal people to disable shadow routing, it's a terrible idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

@darosior
Copy link
Collaborator Author

darosior commented Nov 6, 2019

  • Dropped the first commit (fuzzpercent parameter).
  • Added a json helper (basically a copy of the above one, json_to_number) for u16 and used it for cltv (that fixed the previously introduced FIXME).
  • Made use_shadow dev-only, which introduced a some changes since it's a parameter of a non-dev command. Users are now shadow-safe !
    (and rebased on master)

Thanks for the review !

@darosior
Copy link
Collaborator Author

darosior commented Nov 6, 2019

Ah I forgot the CHANGELOG trick, will do now

@darosior
pay: restore payment value randomization through shadow routing
7e4ea52 
When doing the random walk through the channel, we now add the fees
(both the base and the proportional one) for that channel in addition to
the cltv delta.

Changelog-Added: Payment amount fuzzing is restored, but through shadow routing.
@darosior
common/json: add a helper for json to u16
247c2c9 
As Rusty pointed out to me, the gossip protocol restricts cltvs to u16
so at least we'll use this helper for them.
@darosior darosior force-pushed the amount_fuzzing branch 3 times, most recently from 27fcf2d to 642e819 Compare November 7, 2019 08:26
@darosior
pay: add a dev-only parameter to deactivate shadow routing
db00602 
Had to make a special pylightning function to avoid rewriting all calls
to 'pay()' with 'rpc.call()' in the next commit..
@darosior
pytest: deactivate shadow routing for some tests that use 'pay'
608d868 
So that we can assert payment values
@darosior
pytest: test shadow routing in pay plugin
216dd35
rustyrussell
rustyrussell approved these changes Nov 8, 2019
View reviewed changes
Copy link
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 216dd35

@rustyrussell rustyrussell merged commit 2081237 into ElementsProject:master Nov 8, 2019
@darosior darosior deleted the amount_fuzzing branch November 8, 2019 09:52
@darosior darosior mentioned this pull request May 2, 2020
Closed
@kirelagin kirelagin mentioned this pull request May 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niftynei niftynei niftynei left review comments

@rustyrussell rustyrussell rustyrussell approved these changes

@cdecker cdecker Awaiting requested review from cdecker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darosior @rustyrussell @niftynei