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

Amounts part III: Internal conversion #2365

Merged

Conversation

Projects
None yet
2 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Feb 19, 2019

Follows on from #2360.

Probably best to wait until that's merged, then skim through as one giant change to avoid reviewing all the back-and-forth changes!

@rustyrussell rustyrussell requested review from cdecker and ZmnSCPxj as code owners Feb 19, 2019

@niftynei
Copy link
Collaborator

niftynei left a comment

cool. looks great! i love how this adds a nice layer of over/underflow protection. i left a couple of comments that are obsolete (but that i can't find now). i'll delete them after i post this.

@@ -658,11 +658,11 @@ static void channel_config(struct lightningd *ld,
* - set `dust_limit_satoshis` to a sufficient value to allow
* commitment transactions to propagate through the Bitcoin network.
*/
ours->dust_limit_satoshis = 546;
ours->max_htlc_value_in_flight_msat = UINT64_MAX;
ours->dust_limit = AMOUNT_SAT(546);

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

should we grab this constant from the chainparams instead?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

Yes!

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

(patch welcome ;)

return CHANNEL_ERR_CHANNEL_CAPACITY_EXCEEDED;
}
if (!amount_msat_greater_eq_sat(balance, reserve)) {
status_trace("Cannot fee %s would make balance %s"

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

i think you're missing a verb here Cannot afford...?

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

I accidentally the verb. Will fix!

@@ -687,7 +769,8 @@ static int change_htlcs(struct channel *channel,
u32 approx_max_feerate(const struct channel *channel)

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

should this return a amt_type?

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 21, 2019

Collaborator

i guess feerate isn't exactly an amount...

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Feb 21, 2019

Author Contributor

Yeah, we still use u32 for feerate; it hasn't been a problem so far, since it's much rarer.


if (!ok)
status_failed(STATUS_FAIL_INTERNAL_ERROR,
"Overflow in updating range");

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

maybe add that this is the fee range? i.e. Overflow in updating fee range.

chan->half[idx].htlc_maximum_msat = satoshis * 1000;
chan->half[idx].htlc_minimum = AMOUNT_MSAT(0);
if (!amount_sat_to_msat(&chan->half[idx].htlc_maximum, satoshis))
abort();;

This comment has been minimized.

Copy link
@niftynei

niftynei Feb 20, 2019

Collaborator

ubernit: two ;s?

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-internal-conversion branch 2 times, most recently from 6df5a12 to 929f4da Feb 21, 2019

rustyrussell added some commits Feb 21, 2019

plugins/pay: use struct amount_msat.
This is particularly interesting because we handle overflow during route
calculation now; this could happen in theory once we wumbo.

It fixes a thinko when we print out routehints, too: we want to print
them out literally, not print out the effect they have on fees (which
is in the route, which we also print).

This ABI change doesn't need a CHANGELOG, since paystatus is new since
release.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
struct channel_config: use amount_sat / amount_msat.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
bitcoin/chainparams: use amount_sat / amount_msat
Simple changes, but ripples through the code.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld: use amount_msat for struct htlc amount.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
commit_tx & htlc_tx: use amount_sat/amount_msat.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
bitcoin: use amount_sat/amount_msat.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld: use amount_sat/amount_msat.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
wallet: use amount_sat/amount_msat.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
channeld: avoid overflow in reloading of channel from db.
We used to just throw htlcs into the channel with a flag to tell it to
ignore overflow.  Instead, we can insert them in order (which is the same as
id order) which always must be valid.

This helps when we turn the balance into a struct amount_msat which will get
upset with overflows.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
daemons: use amount_msat/amount_sat in all internal wire transfers.
As a side-effect of using amount_msat in gossipd/routing.c, we explicitly
handle overflows and don't need to pre-prune ridiculous-fee channels.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tools/generate-wire: use amount_msat / amount_sat for peer protocol.
Basically we tell it that every field ending in '_msat' is a struct
amount_msat, and 'satoshis' is an amount_sat.  The exceptions are
channel_update's fee_base_msat which is a u32, and
final_incorrect_htlc_amount's incoming_htlc_amt which is also a
'struct amount_msat'.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Makefile: check for direct amount_sat/amount_msat access.
We need to do it in various places, but we shouldn't do it lightly:
the primitives are there to help us get overflow handling correct.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:amount-internal-conversion branch from 929f4da to fa26fb1 Feb 21, 2019

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Feb 21, 2019

Ack fa26fb1

@rustyrussell rustyrussell merged commit 38e7d19 into ElementsProject:master Feb 21, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.