plugins icon indicating copy to clipboard operation
plugins copied to clipboard

feeadjuster: fix initial feeadjustment loops

Open m-schmoock opened this issue 4 years ago • 2 comments

I discovered some unexpected behavior in the initial fee adjustment loops. This currently just add the xfail test. We can add proper fix in this PR as we go...

feeadjuster/test_feeadjuster.py::test_initial_updates


Update: I added the plugin channel_state_changed notification. When a channel changes to CHANNELD_NORMAL this will now also trigger a maybe_adjust_fees.

Update2: I fixed remaining tests but it seems like I discovered a bug in clightning's code. When a channels gossip gets updated directly after turning to CHANNELD_NORMAL (i.e. by setchannelfee called from a channel_state_changed_hook) the update first gets (correctly) delayed, but then gets missing so that the gossip is actually never updated. I marked the affected testcode lines with a FIXME comment. The two testcases currently fail just because of that.

m-schmoock avatar Jun 26 '21 11:06 m-schmoock

Update2: I fixed remaining tests but it seems like I discovered a bug in clightning's code. When a channels gossip gets updated directly after turning to CHANNELD_NORMAL (i.e. by setchannelfee called from a channel_state_changed_hook) the update first gets (correctly) delayed, but then gets missing so that the gossip is actually never updated. I marked the affected testcode lines with a FIXME comment. The two testcases currently fail just because of that.

I cannot reproduce this, BTW, in c-lightning master, using the following test (though very slow under !DEVELOPER).

def test_setchannelfee_delayed(node_factory, bitcoind):
    l1, l2 = node_factory.line_graph(2)

    l1.rpc.setchannelfee("all", 0xDEAD, 0xBEEF)
    l1.daemon.wait_for_log('delaying .* secs')

    wait_for(lambda: only_one(l2.rpc.listchannels(source=l1.info['id'])['channels'])['base_fee_millisatoshi'] == 0xDEAD)

However, you may be hitting the other effect: we ratelimit everyone's updates, including our own, when we process them (we allow 1 per day average, with a burst of up to 4 per day). That's more strict than this, and can cause us to throw away our own updates!

Perhaps we should give our own updates a free pass?

rustyrussell avatar Jul 25 '21 04:07 rustyrussell

Needs rebase, feeadjuster has been removed from the actively maintained plugins list to make the CI pass again, feel free to add it back :)

chrisguida avatar Feb 07 '24 16:02 chrisguida