The great msat purge #5306

rustyrussell · 2022-06-07T00:43:38Z

@ShahanaFarooqui urged me to complete the msat transition. This series does that, but some msat fields which were not named correctly leaked in, forcing broader deprecations.

All "msat" fields now end in _msat. This allows e.g. the Python code to turn them into Millisatoshi() class unconditionally
All "msat" fields will become raw numbers; this happens currently with allow-deprecated-apis=false. Code should handle both a "123msat" JSON string and 123 for the transition.
We rename some input arguments similarly for consistency.

cdecker · 2022-06-07T14:31:02Z

Yep, that compatibility mode for _msat fields broke cln-rpc and cln-grpc in interesting ways, since now the type of the field changed unexpectedly. I'll try to replicate the compat mode described above.

cdecker · 2022-06-07T14:41:21Z

@rustyrussell are there any raw amounts (just u64) that do not have msat as their unit? The problem is that we built a higher level Amount concept on top of non-bare amounts (i.e., with sat or msat suffix) when building the cln-rpc abstraction, and we don't have the field name readily available to differentiate them. My hope would be that the unit type would also be reflected in the schema, so that we can differentiate it that way when generating the code.

see ElementsProject/lightning#5306

rustyrussell · 2022-06-09T04:18:08Z

Yes, schema should be correct! We should add tooling on this (that all outgoing msat fields have names _msat, and all _msat field have msat values) though.

vincenzopalazzo

ACK f697809

This example predates the pay plugin! It's obsolete, unmaintained, and probably doesn't work. Signed-off-by: Rusty Russell <[email protected]>

Changelog-Fixed: lightningd: `log-level` `io` shows JSONRPC output, as well as input. Signed-off-by: Rusty Russell <[email protected]>

This is *much* easier to do inside parsing than in the caller! Signed-off-by: Rusty Russell <[email protected]>

We had json_add_amount_msat_only(), which was designed to be used to print out msat fields, if we had sats. However, we misused it, so split it into the three different cases: 1. json_add_amount_sat_msat: We are using it correctly, with a field called xxx_msat. 2. json_add_amount_sats_deprecated: We were using it wrong, so deprecate the old field and create a new one which does end in _msat. 3. json_add_sats: we were using it to hand sats as a JSON parameter to an interface, where "XXXsat". Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: Plugins: `rbf_channel` and `openchannel2` hooks `their_funding` (use `their_funding_msat`) Changelog-Deprecated: Plugins: `openchannel2` hook `dust_limit_satoshis` (use `dust_limit_msat`) Changelog-Deprecated: Plugins: `openchannel` hook `funding_satoshis` (use `funding_msat`) Changelog-Deprecated: Plugins: `openchannel` hook `dust_limit_satoshis` (use `dust_limit_msat`) Changelog-Deprecated: Plugins: `openchannel` hook `channel_reserve_satoshis` (use `channel_reserve_msat`) Changelog-Deprecated: Plugins: `channel_opened` notification `amount` (use `funding_msat`) Changelog-Deprecated: JSON-RPC: `listtransactions` `msat` (use `amount_msat`) Changelog-Deprecated: Plugins: `htlc_accepted` `forward_amount` (use `forward_msat`)

We don't always want to round down, sometimes we want to fail. Signed-off-by: Rusty Russell <[email protected]>

In general, we don't like to use `null` in JSON: simply omit the field. I found this one because it broke our 'msat' parsing (made stricter in followup) which doesn't allow `null`. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: `listconfigs` `plugins` `options` which are not set are omitted, not `null`.

The new msat fields are turned into Millisatoshi, so handle that correctly too in tests too. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: Plugins: `coin_movement` notification: `balance`, `credit`, `debit` and `fees` (use `balance_msat`, `credit_msat`, `debit_msat` and `fees_msat`)

This is consistent with our output changes, and increases consistency. It also keeps future sanity checks happy, that we only use JSON msat helpers with '_msat' fields. Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: JSON-RPC: `invoice`, `sendonion`, `sendpay`, `pay`, `keysend`, `fetchinvoice`, `sendinvoice`: `msatoshi` argument is now called `amount_msat` to match other fields. Changelog-Deprecated: JSON-RPC: `invoice`, `sendonion`, `sendpay`, `pay`, `keysend`, `fetchinvoice`, `sendinvoice` `msatoshi` (use `amount_msat`)

Changelog-Added: Plugins: `htlc_accepted_hook` `amount_msat` field. Changelog-Deprecated: Plugins: `htlc_accepted_hook` `amount` field (use `amount_msat`) Signed-off-by: Rusty Russell <[email protected]>

Nobody really parses this though, fortunately. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `pay` `attempts` `amount_msat` field. Changelog-Deprecated: JSON-RPC: `pay` `attempts` `amount` field (use `amount_msat`).

The name in the spec is `msat`, but I don't want to make an API exception. Signed-off-by: Rusty Russell <[email protected]> Changelog-Added: JSON-RPC: `fetchinvoice` `changes` `amount_msat` Changelog-Deprecated: JSON-RPC: `fetchinvoice` `changes` `msat` (use `amount_msat`)

Now we've fixed them all, make sure no new ones slip in! Signed-off-by: Rusty Russell <[email protected]>

This prepares for when they start being u64, not strings with msat appended. This has a strange side effect on our schema: despite the name, decodepay's `fee_base_msat` is actually a u64, which we now convert to msat on decode. Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

A small change in one routine creates a lot of changes! We actually recommended moving away from these in v0.7.0 (2019-02-28), but never deprecated them formally. Changelog-Deprecated: JSON-RPC: `pay`, `decode`, `decodepay`, `getroute`, `listinvoices`, `listpays` and `listsendpays` `msatoshi` fields (use `amount_msat`). Changelog-Deprecated: JSON-RPC: `getinfo` `msatoshi_fees_collected` field (use `fees_collected_msat`). Changelog-Deprecated: JSON-RPC: `listpeers` `channels`: `msatoshi_to_us`, `msatoshi_to_us_min`, `msatoshi_to_us_max`, `msatoshi_total`, `dust_limit_satoshis`, `our_channel_reserve_satoshis`, `their_channel_reserve_satoshis`, `spendable_msatoshi`, `receivable_msatoshi`, `in_msatoshi_offered`, `in_msatoshi_fulfilled`, `out_msatoshi_offered`, `out_msatoshi_fulfilled`, `max_htlc_value_in_flight_msat` and `htlc_minimum_msat` (use `to_us_msat`, `min_to_us_msat`, `max_to_us_msat`, `total_msat`, `dust_limit_msat`, `our_reserve_msat`, `their_reserve_msat`, `spendable_msat`, `receivable_msat`, `in_offered_msat`, `in_fulfilled_msat`, `out_offered_msat`, `out_fulfilled_msat`, `max_total_htlc_in_msat` and `minimum_htlc_in_msat`). Changelog-Deprecated: JSON-RPC: `listinvoices` and `pay` `msatoshi_received` and `msatoshi_sent` (use `amount_received_msat`, `amount_sent_msat`) Changelog-Deprecated: JSON-RPC: `listpays` and `listsendpays` `msatoshi_sent` (use `amount_sent_msat`) Changelog-Deprecated: JSON-RPC: `listforwards` `in_msatoshi`, `out_msatoshi` and `fee` (use `in_msat`, `out_msat` and `fee_msat`) Changelog-Deprecated: JSON-RPC: `listfunds` `outputs` `value` (use `amount_msat`) Signed-off-by: Rusty Russell <[email protected]>

We should be using amount_msat always. Many tests were not. Plus, deprecating it simplifies the code. Signed-off-by: Rusty Russell <[email protected]> Changelog-Deprecated: JSONRPC: `sendpay` `route` elements `msatoshi` (use `amount_msat`)

…l have "msat" appended. After next patch, it's a raw u64, and this code broke. Signed-off-by: Rusty Russell <[email protected]>

Let the parsing handle if they're the wrong type (soon they'll be u64). Signed-off-by: Rusty Russell <[email protected]>

We would otherwise multiply them by 1000. Signed-off-by: Rusty Russell <[email protected]>

This code was buggy: handing "1000" as a parameter to min_their_funding_msat, don't turn that into "1000sat"! Signed-off-by: Rusty Russell <[email protected]>

We would otherwise multiply them by 1000. Signed-off-by: Rusty Russell <[email protected]>

@ShahanaFarooqui

This changes many fields: in non-deprecated mode, they're now raw integers. This was always the intention, but the transition was never completed. Suggested-By: @ShahanaFarooqui Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: JSON-RPC: "_msat" fields can be raw numbers, not "123msat" strings (please handle both!) Changelog-Deprecated: JSON-RPC: "_msat" fields as "123msat" strings (will be only numbers)

Signed-off-by: Rusty Russell <[email protected]>

Since we want to transition to raw `u64` values we need to accept both for some time.

rustyrussell · 2022-06-20T10:29:01Z

Trivial rebase to address conflicts, and update examples in PLUGINS.md.

Ack 6132e9f

rustyrussell · 2022-06-20T21:22:02Z

Ack 6132e9f
(In case ackbot is waiting for a separate message, not an edited one?)

jsarenik · 2022-06-30T12:32:00Z

Please do not release a stable version until lightningd/plugins#369 is fixed (here in pyln). Thanks.

jsarenik · 2022-06-30T12:32:45Z

Or rother there in the summary plugin? OK.

8461e3f fix(rpc): MSat accepts both strings ending with msat and u64 (Riccardo Casatta) Pull request description: According to ElementsProject/lightning#5306 the fields ending with msat will be raw numbers "Code should handle both a "123msat" JSON string and 123 for the transition." I am not 100% sure if it's allowed somewhere to have negative numbers, I thought there aren't based on the `MSat` type on branch master which contains a `u64` Top commit has no ACKs. Tree-SHA512: 734c644a42a30e9985e6da8891912ca6754d924a3a55562e9a670e482aaefe71d89c865160da356944f43e29d829d192b0b91733b5331d9698051c2f159bef2a

rustyrussell added the json-rpc label Jun 7, 2022

rustyrussell added this to the v0.12 milestone Jun 7, 2022

rustyrussell requested review from niftynei and ShahanaFarooqui June 7, 2022 00:43

rustyrussell requested a review from cdecker as a code owner June 7, 2022 00:43

fiatjaf added a commit to nbd-wtf/poncho that referenced this pull request Jun 8, 2022

prepare for the great msat purge.

299b77c

see ElementsProject/lightning#5306

rustyrussell force-pushed the guilt/the-great-msat-purge branch 2 times, most recently from 8037b56 to f697809 Compare June 18, 2022 04:30

vincenzopalazzo approved these changes Jun 18, 2022

View reviewed changes

rustyrussell added 18 commits June 19, 2022 16:43

contrib/pylightning: remove lightning-pay helper.

d249707

This example predates the pay plugin! It's obsolete, unmaintained, and probably doesn't work. Signed-off-by: Rusty Russell <[email protected]>

lightningd: fix outgoing IO logging for JSONRPC.

f6c11d3

Changelog-Fixed: lightningd: `log-level` `io` shows JSONRPC output, as well as input. Signed-off-by: Rusty Russell <[email protected]>

common/param: support renaming options using "|<deprecatedname>".

e8bb344

This is *much* easier to do inside parsing than in the caller! Signed-off-by: Rusty Russell <[email protected]>

common: add msat to sat convert helper.

2907732

We don't always want to round down, sometimes we want to fail. Signed-off-by: Rusty Russell <[email protected]>

lightningd: use amount_msat not amount in htlc_accepted_hook.

1f84eb3

Changelog-Added: Plugins: `htlc_accepted_hook` `amount_msat` field. Changelog-Deprecated: Plugins: `htlc_accepted_hook` `amount` field (use `amount_msat`) Signed-off-by: Rusty Russell <[email protected]>

common: enforce that msat fields are called "xxx_msat".

c6c794b

Now we've fixed them all, make sure no new ones slip in! Signed-off-by: Rusty Russell <[email protected]>

pytest: don't use deprecated amount fields

d44eb0d

Signed-off-by: Rusty Russell <[email protected]>

fundchannel, txprepare, multiwithdraw: don't assume "excess_msat" wil…

6e3bd02

…l have "msat" appended. After next patch, it's a raw u64, and this code broke. Signed-off-by: Rusty Russell <[email protected]>

libplugin: don't be so strict on msat fields.

9e36911

Let the parsing handle if they're the wrong type (soon they'll be u64). Signed-off-by: Rusty Russell <[email protected]>

rustyrussell and others added 6 commits June 20, 2022 19:52

funder: prepare for msats fields as raw numbers.

0bb25da

We would otherwise multiply them by 1000. Signed-off-by: Rusty Russell <[email protected]>

plugins/funder: fix parameter parsing.

4c8bbc1

This code was buggy: handing "1000" as a parameter to min_their_funding_msat, don't turn that into "1000sat"! Signed-off-by: Rusty Russell <[email protected]>

lease_rates: prepare for msats fields as raw numbers.

a56ef47

We would otherwise multiply them by 1000. Signed-off-by: Rusty Russell <[email protected]>

doc/PLUGINS: update to remove deprecated fields, formats.

f491ad8

Signed-off-by: Rusty Russell <[email protected]>

clnrs: Implement backwards compatible mode for Amount as bare u64

6132e9f

Since we want to transition to raw `u64` values we need to accept both for some time.

rustyrussell force-pushed the guilt/the-great-msat-purge branch from f697809 to 6132e9f Compare June 20, 2022 10:28

ShahanaFarooqui approved these changes Jun 20, 2022

View reviewed changes

rustyrussell merged commit 9a880a0 into ElementsProject:master Jun 20, 2022

vincenzopalazzo mentioned this pull request Jun 20, 2022

retype the filed from msat from String to int clightning4j/JRPClightning#74

Open

vincenzopalazzo mentioned this pull request Jun 30, 2022

summary: NO PUBLIC ADDRESSES lightningd/plugins#369

Open

vincenzopalazzo mentioned this pull request Jul 7, 2022

track field of amount of msat in invoice dart-lightning/lndart.clnapp#24

Closed

This was referenced Jul 8, 2022

Sendpay: providing amount_msat as well as msatoshi fails #5386

Closed

CLN: msatoshi in sendpay route got deprecated ElementsProject/peerswap#92

Closed

RCasatta mentioned this pull request Sep 3, 2022

MSat accepts both strings ending with msat and u64 laanwj/cln4rust#53

Merged

roeierez mentioned this pull request Sep 13, 2022

support zero conf funding and renaming fields niftynei/glightning#41

Open

thesimplekid mentioned this pull request Mar 25, 2023

improve: use serde with elsirion/clnurl#9

Merged

sha-265 mentioned this pull request May 18, 2023

Fix compatibility issues with CLN 23.05 in activity ZeusLN/zeus#1465

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The great msat purge #5306

The great msat purge #5306

rustyrussell commented Jun 7, 2022

cdecker commented Jun 7, 2022

cdecker commented Jun 7, 2022

rustyrussell commented Jun 9, 2022

vincenzopalazzo left a comment

rustyrussell commented Jun 20, 2022 •

edited

Loading

rustyrussell commented Jun 20, 2022

jsarenik commented Jun 30, 2022

jsarenik commented Jun 30, 2022

The great msat purge #5306

The great msat purge #5306

Conversation

rustyrussell commented Jun 7, 2022

cdecker commented Jun 7, 2022

cdecker commented Jun 7, 2022

rustyrussell commented Jun 9, 2022

vincenzopalazzo left a comment

Choose a reason for hiding this comment

rustyrussell commented Jun 20, 2022 • edited Loading

rustyrussell commented Jun 20, 2022

jsarenik commented Jun 30, 2022

jsarenik commented Jun 30, 2022

rustyrussell commented Jun 20, 2022 •

edited

Loading