Add options to disable clboss features which cost money #212
Conversation
Merge upstream changes
|
Thanks for working on this! I kicked off the CI, I think there might be some interesting failures. Some thoughts on other parts follow ... |
|
When looking at (I'm tempted to see what ChatGPT proposes ... ;-) ) |
|
You've modified the I wonder if it would make more sense to inhibit the actions of the I haven't looked into what interactions the |
|
I think the |
|
We should make sure that inhibiting the Your implementation here looks more likely to cut off all the desired activity. |
|
Thanks Ken. I will have a look at the test failures (I must admit I overlooked the test suite completely) and see what I can do about the other points you've raised. Thanks very much for taking a look! If you do feel interested to pursue the code duplication thing with ChatGPT assistance obviously feel free to go ahead with that. I didn't touch JitRebalancer because I think it's disabled by default, although it may well be good to handle this case too. InitialRebalancer seems to be limited to spending 0.04% of the channel balance and is a one-off thing so I thought it might be best to allow that. Your suggestion of moving the check into FundsMover (subject to verifying it doesn't break anything else) seems superficially reasonable, though I haven't looked into this in any detail yet. I will be away a bit over the next week or so and while I might be able to work on this, I might not, so apologies in advance if I'm a bit unresponsive. |
|
Incidentally is there any way to convert these diagrams like channel_creation.md into a high-resolution bitmap file for viewing offline? I have experimented a bit and I haven't been able to convert the whole diagram into a bitmap using the upstream website - I could only get a bitmap showing what was currently on my screen. I find panning and zooming around them pretty uncomfortable in github and in the upstream website viewer and they would probably be a lot more usable if I could use a local graphics viewer on them. |
|
Yeah, the github viewer is not great. The best I can do with the github viewer is:
A much better viewer that might be able to export is here: https://mermaid.live/ I'd love to figure out how to have a link to click to preload the CLBOSS graphs into that editor but haven't figured it out yet ... |
|
Apologies for the delay in working on this, Ken, and thanks again for taking the time to give such detailed comments. I've now been over them in more detail. Thanks also for the advice about viewing the charts in mermaid.live, that's definitely more usable.
That diagram suggests that ActiveProber uses FundsMover, which doesn't seem obvious to me when looking at the code. Am I getting confused? It looks to me as though ActiveProber::sendpay() makes a direct "sendpay" RPC. FundsMover appears to be invoked in general by Msg::RequestMoveFunds, which is raised by MoveFundsCommand, InitialRebalancer, JitRebalancer and EarningsRebalancer. So I think the main concern with moving the "inhibit circular rebalance" behavior into FundsMover would be the possibility of breaking MoveFundsCommand. That is debug-only, so maybe it doesn't really matter. Otherwise, I think it would work fairly well to move the inhibit logic into FundsMover/Main.cpp's Msg::RequestMoveFunds subscription handler. I was worried that inhibiting rebalances might cause problems where we think we've sent a payment and it never arrives and we blame one of our peers, but I think that's fine. This appears to be handled by monitoring "sendpay" RPC calls, so if we simply never make a "sendpay" call in the first place there is no risk of anyone getting blamed.
I think you're right about NeedsOnchainFundsSwapper needing to be included in the restriction. I'm not so keen on inhibiting in SwapManager or BoltzSwapper. For BoltzSwapper specifically, as this is a general "no auto swap" restriction it doesn't seem to belong in a Boltz-specific class. SwapManager feels (admittedly based on my imperfect understanding of the clboss architecture) more like code to do a job than code to implement policy like this. My inclination would be to move this check from NodeBalanceSwapper into trigger_swap(), which is used by both NodeBalanceSwapper and NeedsOnchainFundsSwapper (and triggers SwapManager). Incidentally, trigger_swap() is one of the two places the existing GetOnchainIgnoreFlag is checked, which surprised me at first but does make sense the more I think about it - there is no point triggering a swap to get on-chain funds if we will just refuse to do anything with them. (The other place is OnChainFundsAnnouncer.) I mention this mainly as supporting evidence that trigger_swap() is a reasonable place to add "policy" code like this inhibit behavior.
I've had a look over the code and your diagram and it looks to me like ChannelCreationDecider is the right place for this logic - it is the only code that can raise Msg::RequestChannelCreation messages. If I've missed something here could you please let me know what?
I haven't had a look at this yet, although I agree it would be good to reduce the code duplication. I will see if I can think of anything - I'll hold off making any changes based on the above analysis for at least a couple of days in case you or anyone else has any comments. |
I think |
Ah, I missed the Thank you! |
Thanks Ken, I think I missed the label on the arrow somehow. I agree this is not using the FundsMover to move anything. That said, I may be getting confused but it looks to me as though both ActiveProber and FundsMover subscribe to the SolicitDeletablePaymentLabelFilter message but it is only raised in PaymentDeleter::Impl::initialize(), so ActiveProber doesn't use FundsMover via this message, it is just that they both respond to this message to allow PaymentDeleter to delete their payments. |
I have pushed a commit to the low-cost-v1 branch which tries to address this. It's a fairly obvious template-based approach but while slightly fiddly (because you can't use literal strings as template arguments) it does save code duplication. |
|
I've pushed another commit to low-cost-v1 which moves the swap and circular rebalance inhibit logic as discussed above. This has had minimal testing on signet only. I believe this addresses all the feedback so far. It definitely needs some testing before it can be considered for merging, and of course if anyone has any further feedback please let me know. |
|
@sj-fisher are you on discord? Possibly related thoughts here: https://discord.com/channels/899980449231814676/907338137041256509/1273205821098037279 |
|
I just signed up for discord but when I click that link I get a page which says "No text channels". Do I need to join a particular server or channel first? I'm not very familiar with how this works, but I am able to browse some random discoverable servers and see messages. |
|
That link still works for me ... maybe you need an invite? Try this: The comment is in the CLBOSS channel on the core lightning discord server, all very public ... |
|
Thanks Ken, I can see it now I've accepted your invitation. Maybe this was necessary or maybe I just missed something before. I am not yet running 0.13.3 so I have not seen this fundsmover activity or how/if it interacts with the changes on this pull request. Now I know about 0.13.3 I will look at upgrading, but it's not likely to be for a few weeks as I want to make sure I have spare time to try to recover things if it all goes bad. I'll also try to keep an eye on the discord now I know about it, thanks. |
|
@sj-fisher we're trying to setup a conf call to brainstorm CLBOSS futures: |
|
Thanks Ken, I sent you a message on discord about the call. On a different note and speaking to anyone reading, I have created a branch https://github.com/sj-fisher/clboss/tree/low-cost-v1-rebased which contains the same changes as low-cost-v1 but on top of clboss 0.13.3. I have given this a very quick test on signet and will probably try it on my mainnet node soon-ish. Testing welcome but please use caution if you're on mainnet! |
|
Should I test this? |
|
Apologies for not replying earlier... It may be best to see what Ken thinks. I implemented this because I had a small node I wanted to run clboss on but didn't want it splurging on Boltz fees to get inbound liquidity I mostly didn't care about. That said, I completely get (what I think is) Ken's position from the recent Friday calls that it's better to tune clboss to work without manual options like this as far as possible. Given that, I can't see this being merged into a release any time soon - it would probably only get merged after the ongoing tuning and discoverability work nears completion and if it turns out there is still value in these options. I don't think these changes are likely to break things badly, so if you want these options to work around clboss wasting money right now, please do go ahead and test. But if you're just testing to help these changes moving towards inclusion, that would be appreciated and a step forward but may not actually result in them moving much closer to inclusion as I suspect the tuning/discoverability work is the main blocker. (By "discoverability" I mean the development of tools and wiki pages and so on which allow users to have a better idea what's going on inside their node.) |
|
If you do decide to test, please note the new branch https://github.com/sj-fisher/clboss/tree/low-cost-v1-rebased where I rebased this change onto 0.13.3. It wasn't a difficult merge so you'd probably be fine taking the low-cost-v1 branch mentioned on the actual pull request, but you might as well take the one where I already did it. |
This attempts to address #202.
This is a draft pull request - feedback would be appreciated!
This has been lightly tested on signet and nothing more. Please be very cautious if using this on mainnet!
Overview
This adds three new boolean options:
The new options can be specified via config file entries, e.g.
and can also be controlled via lightning-cli, e.g.:
If present, config file entries are applied on startup, overriding any changes made earlier using lightning-cli commands. Otherwise, changes made with lightning-cli are persisted in the database across restarts. This feels a bit over-complex, but it's also the most flexible approach.
If nothing is ever specified, all three default to "true" for backwards compatibility.
Each of the options can also be temporarily set using lightning-cli, e.g.:
will temporarily allow auto-opening of channels for 3 hours. If this command uses the same boolean value as the base state, it is a no-op.
A temporary setting can be removed before it times out by using the non-temporary form of the command. For example, to cancel the previous temporary permission to auto-open channels:
You can see the current state using:
Possible problems
Overlap between auto-open-channels and ignore-onchain
The new auto-open-channels/temporarily-auto-open-channels commands have a lot of overlap with the existing notice/ignore-onchain commands. They are not quite the same:
It would obviously be possible to check in the same places in the code if desired. I do like the ability to have "default false", which is why I added this in the first place. I think there is a theoretical difference between the two which doesn't apply at the moment - if (for example) we had support for advertising our willingness to dual-fund channels, ignore-onchain should probably prevent that, whereas "auto-open-channels false" would not.
I'm reluctant to break backwards compatibility by removing the notice/ignore-onchain options completely, but they could be handled as special-case syntax for the new auto-open-channels behavior.
Implementation notes
Most of the new code is in Boss/ModG/TimedBoolean.hpp, which adds a TimedBoolean helper class which deals with all the messy user interface and state maintenance for "timed booleans", i.e. booleans which have a base state and which can be set to a different state for a fixed period of time. This has its origins in the existing OnchainFundsIgnorer code.
Three new controllers (AutoOpenController, AutoSwapController and CircularRebalanceController, all in Boss/Mod) use TimedBoolean internally and expose a pair of Request/Response messages to allow their current state to be queried.
Logic has then been added (in ChannelCreationDecider, NodeBalanceSwapper and EarningsRebalancer) to disable the existing behavior where the relevant timed boolean is currently false.