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

Update to handle JSON-RPC amounts in either the old or new conventions #162

Merged
merged 1 commit into from
Aug 19, 2023
Merged

Update to handle JSON-RPC amounts in either the old or new conventions #162

merged 1 commit into from
Aug 19, 2023

Conversation

ksedgwic
Copy link
Collaborator

@ksedgwic ksedgwic commented May 3, 2023 •
edited

The JSON-RPC interface has changed the type of amounts from string w/ "msat" suffix to a plain number.

The backtrace hack and removal of the Amount constructor-from-string are useful during debugging but we might want to drop them ...

@ksedgwic ksedgwic mentioned this pull request May 3, 2023
Closed
@tsjk
Copy link

tsjk commented May 9, 2023

What does this fix?
Does it make clboss compatible with cln v23?

@ksedgwic
Copy link
Collaborator Author

ksedgwic commented May 9, 2023

Yes, this PR makes CLBOSS compatible with v23.05.

However, it is not currently backwards compatible; if you run this PR against older versions of CLN it won't work. It should be possible to make it backwards compatible, I will have a look at that next ...

@ksedgwic ksedgwic changed the title Update amounts to msat Update to handle JSON-RPC amounts in either the old or new conventions May 12, 2023
@ksedgwic
Copy link
Collaborator Author

@tsjk I've reworked the CLBOSS amount PR so it should be able to handle either the old or new conventions.

@ksedgwic
Copy link
Collaborator Author

It's running successfully on CLN v23.05, but I have not verified it on an older CLN.

@tsjk
Copy link

tsjk commented May 12, 2023

I'll try to find time to test this soon.

@ksedgwic ksedgwic marked this pull request as draft May 12, 2023 20:08
@ksedgwic
Copy link
Collaborator Author

@tsjk switching back to Draft, I'm seeing bogus swap reports in clboss-status:

         {
            "time": 1.68391e+09,
            "time_human": "UTC 2023-05-12 17:29:15.552",
            "amount_sent": "2msat",
            "amount_received": "18446744073709551456msat",
            "amount_lost": "0msat",
            "provider": "Boltz::Service(\"https://testnet.boltz.exchange/api\")"
         },

The amount_sent and amount_received are obviously wrong.

The values in the data.clboss db are also bad after my changes:

1679187607.98172|299528669|297715000|Boltz::Service("https://testnet.boltz.exchange/api")
1679200208.08367|100094092|99148000|Boltz::Service("https://testnet.boltz.exchange/api")
1679211008.25419|398096693|395157000|Boltz::Service("https://testnet.boltz.exchange/api")
1679291804.75702|108338105|105921000|Boltz::Service("https://testnet.boltz.exchange/api")
1679302004.89865|100000095|92722000|Boltz::Service("https://testnet.boltz.exchange/api")
1680386607.52125|4096999375|4077496000|Boltz::Service("https://testnet.boltz.exchange/api")
1681898663.87814|312002872|309660000|Boltz::Service("https://testnet.boltz.exchange/api")
1683855569.11341|2|94067341782384|Boltz::Service("https://testnet.boltz.exchange/api")
1683867701.28066|2|94842369897008|Boltz::Service("https://testnet.boltz.exchange/api")
1683878085.26563|2|-160|Boltz::Service("https://testnet.boltz.exchange/api")
1683885285.61652|2|-160|Boltz::Service("https://testnet.boltz.exchange/api")
1683893817.04882|2|-160|Boltz::Service("https://testnet.boltz.exchange/api")
1683902355.21184|2|-160|Boltz::Service("https://testnet.boltz.exchange/api")
1683912555.55297|2|-160|Boltz::Service("https://testnet.boltz.exchange/api")

Still looking, clues welcome ...

@tsjk
Copy link

tsjk commented May 13, 2023

The negative numbers don't look too good. :)
I have not detailed knowledge of the code, but I have browsed through it on several occasions.
The amount received is on-chain, right? Are on-chain transactions effected in the same way by the api-change in cln?
If you compare what the numbers should be with what they are - does that give clues? How much did you actually send and receive?

@ksedgwic
Copy link
Collaborator Author

ksedgwic commented May 18, 2023 •
edited

@tsjk I force pushed a new version which uses a class method instead of a conversion constructor for the JSON -> Amount method. I have seen one swap since and it was correctly reported. Possibly the problem is fixed.

TODO:

  • observe additional swap reports with v23.05
  • test on older versions which still use deprecated amount msat string format.

@jonasnick jonasnick mentioned this pull request May 22, 2023
Merged
@ksedgwic ksedgwic mentioned this pull request May 22, 2023
Closed
@ksedgwic
Copy link
Collaborator Author

@tsjk I've observed more swaps with well-formed reports. I'm checking the first TODO box. Any chance you could try on an older CLN version to validate backwards compatibility?

@ksedgwic ksedgwic marked this pull request as ready for review May 23, 2023 22:36
@ksedgwic ksedgwic mentioned this pull request May 23, 2023
Closed
@tsjk
Copy link

tsjk commented May 30, 2023

Sorry. I've been busy with other things.
I guess I could test this. Haven't ran cln on a testnet but can imagine to try it and let clboss go wild.
Not sure testing it on my mainnet node would make a lot of sense.

@JavierRSobrino
Copy link

When is the solution going to be merged with master? Is it ok to wait?

@ksedgwic
Copy link
Collaborator Author

ksedgwic commented Jun 5, 2023

It still needs testing on older systems with the legacy msat format, modulo that it has been working great for us on v23.05

@tsjk
Copy link

tsjk commented Jun 6, 2023

I ended up upgrading my mainnet node, because the version I was using (v22.11.1 with some back-ported patches) actually back-ported the msat removal - voiding my reason for stay on that version. For now my build recipe for Gentoo only applies this patch if the installed version of core lightning is v23.05 or higher.
But, we should definitely try to test this. My idea was to setup at testnet node with a testnet bitcoind. I realise that's quite an effort. So, perhaps this should be tested by someone still on earlier version - or we could just go high chaparral and requiring that the latest clboss requires core-lightning v23.05 or later...

@ksedgwic
Copy link
Collaborator Author

ksedgwic commented Jun 6, 2023

or we could just go high chaparral and requiring that the latest clboss requires core-lightning v23.05 or later...

Over time this will become more and more reasonable. Not sure if it is quite reasonable yet though (especially for mainnet where folks run more mature versions ...)

@JavierRSobrino
Copy link

Well, people using old versions of Core Lightning and not intending to upgrade have zero reasons to try this patch. So I think it is better to just release this and pass an important notice like "Not compatible with versions previous to v23.05". You may want to even test version in code and return an error if version is not equal or higher than v23.05..

@tsjk
Copy link

tsjk commented Jun 13, 2023

Yes, checking for version sounds sensible.
On another note, I've been running this for a while and it seems ok. No submarine swaps performed as of yet, however.

@JavierRSobrino
Copy link

Would you please check if #147 is related to this?

@ghost ghost mentioned this pull request Jun 30, 2023
Closed
@openoms openoms mentioned this pull request Aug 16, 2023
Closed
@chrisguida
Copy link
Contributor

Testing this on 23.02 with a raspiblitz

vincenzopalazzo
vincenzopalazzo approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK a4bb019

@chrisguida
Copy link
Contributor

Update: ran overnight on raspiblitz with CLN 23.02, looks great. Let's merge!

ACK a4bb019

@ksedgwic
Copy link
Collaborator Author

I'm attempting resolve a conflict with recent commits on master.

Having a build problem with master before any rebasing/merging: #166

@ksedgwic
Copy link
Collaborator Author

rebasing on master

@ksedgwic
Copy link
Collaborator Author

I'm going to smoke test this in my test nodes, will merge if problem free

@tsjk
Copy link

tsjk commented Aug 18, 2023 •
edited

Was just about to ask whether this was tested on any pre-msat-drop nodes in the end.

@ksedgwic
Copy link
Collaborator Author

@tsjk #162 (comment)

@ksedgwic
Copy link
Collaborator Author

Looks stable on my testnet nodes, merging

@ksedgwic ksedgwic merged commit 60c07a7 into ZmnSCPxj:master Aug 19, 2023
1 check passed
@kn0wmad
Copy link

kn0wmad commented Aug 19, 2023

Any ETA on a release that might include this fix?

@ksedgwic
Copy link
Collaborator Author

@kn0wmad #172

@ksedgwic ksedgwic mentioned this pull request Sep 11, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisguida chrisguida chrisguida approved these changes

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@ksedgwic @tsjk @JavierRSobrino @chrisguida @kn0wmad @vincenzopalazzo