-
Notifications
You must be signed in to change notification settings - Fork 41
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
Disable mpp until LND race condition is resolved #323
Disable mpp until LND race condition is resolved #323
Conversation
Ideally it should be linked to a settings with default = allow mpp. This way it is non breaking and allows user to disable MPP if they desire. |
rebalancer.py
Outdated
@@ -58,7 +58,7 @@ async def run_rebalancer(rebalance, worker): | |||
timeout = rebalance.duration * 60 | |||
invoice_response = stub.AddInvoice(ln.Invoice(value=rebalance.value, expiry=timeout)) | |||
print(f"{datetime.now().strftime('%c')} : {worker} starting rebalance for: {rebalance.target_alias} {rebalance.last_hop_pubkey=} {rebalance.value=} {rebalance.duration=} {chan_ids=}") | |||
async for payment_response in routerstub.SendPaymentV2(lnr.SendPaymentRequest(payment_request=str(invoice_response.payment_request), fee_limit_msat=int(rebalance.fee_limit*1000), outgoing_chan_ids=chan_ids, last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey), timeout_seconds=(timeout-5), allow_self_payment=True), timeout=(timeout+60)): | |||
async for payment_response in routerstub.SendPaymentV2(lnr.SendPaymentRequest(payment_request=str(invoice_response.payment_request), fee_limit_msat=int(rebalance.fee_limit*1000), outgoing_chan_ids=chan_ids, last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey), timeout_seconds=(timeout-5), allow_self_payment=True, max_parts=1), timeout=(timeout+60)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it
if disable_mpp = True:
// set max_parts=1
else:
// keep existing functionality
you can set disable_mpp based on LocalSettings object at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I'm wrong, but how would that look like in detail?
I would set LND_MPP_DISABLED = False
in lndg.settings.py and
if settings.LOGIN_REQUIRED.LND_MPP_DISABLED = True:
async for payment_response in routerstub.SendPaymentV2(lnr.SendPaymentRequest(payment_request=str(invoice_response.payment_request), fee_limit_msat=int(rebalance.fee_limit*1000), outgoing_chan_ids=chan_ids, last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey), timeout_seconds=(timeout-5), allow_self_payment=True, max_parts=1), timeout=(timeout+60)):
# Add max_parts=1 for disable_mpp=True
else:
async for payment_response in routerstub.SendPaymentV2(lnr.SendPaymentRequest(payment_request=str(invoice_response.payment_request), fee_limit_msat=int(rebalance.fee_limit*1000), outgoing_chan_ids=chan_ids, last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey), timeout_seconds=(timeout-5), allow_self_payment=True), timeout=(timeout+60)):
# no max_parts=1 for disable_mpp=False
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BhaagBoseDK I'm not sure I got the LocalSettings Header part right, would you mind a review and adjust based on how you usually do it in the LNDg codebase, please?
# Define LocalSettings
class LocalSettings:
def __init__(self):
self.enable_mpp = True # Default value is True
# Check if LocalSetting LND-EnableMPP exists and set enable_mpp accordingly
if LocalSettings.objects.filter(key='LND-EnableMpp').exists():
if int(LocalSettings.objects.filter(key='LND-EnableMpp')[0].value) > 0:
enable_mpp = True
else:
LocalSettings(key='LND-EnableMpp', value='0').save() # Set with 0 value for the future
enable_mpp = False
else:
enable_mpp = True # Default value is True. Not sure we need it again
if not enable_mpp:
async for payment_response in routerstub.SendPaymentV2(
lnr.SendPaymentRequest(
payment_request=str(invoice_response.payment_request),
fee_limit_msat=int(rebalance.fee_limit * 1000),
outgoing_chan_ids=chan_ids,
last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey),
timeout_seconds=(timeout - 5),
allow_self_payment=True,
max_parts=1
),
timeout=(timeout + 60)
):
# enable_mpp=False
else:
async for payment_response in routerstub.SendPaymentV2(
lnr.SendPaymentRequest(
payment_request=str(invoice_response.payment_request),
fee_limit_msat=int(rebalance.fee_limit * 1000),
outgoing_chan_ids=chan_ids,
last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey),
timeout_seconds=(timeout - 5),
allow_self_payment=True
),
timeout=(timeout + 60)
):
# enable_mpp=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping this for your kind review @BhaagBoseDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I would try to condense this to a single SendPaymentRequest
by making max_parts=None
when MPP is not disabled. This seems to still send payments when set to None
but I wasn't able to verify that MPP payments were still being made yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cryptosharks131
How about this consolidating the SendPaymentRequest
.
@ziggie1984 would you know whether we still send it when max_parts=None
?
# Define LocalSettings
class LocalSettings:
def __init__(self):
self.enable_mpp = True # Default value is True
# Check if LocalSetting LND-EnableMPP exists and set enable_mpp accordingly
if LocalSettings.objects.filter(key='LND-EnableMpp').exists():
if int(LocalSettings.objects.filter(key='LND-EnableMpp')[0].value) > 0:
enable_mpp = True
else:
LocalSettings(key='LND-EnableMpp', value='0').save() # Set with 0 value for the future
enable_mpp = False
else:
enable_mpp = True # Default value is True. Not sure we need it again
max_parts = 1 if enable_mpp else None # Adjust max_parts based on the enable_mpp value
async for payment_response in routerstub.SendPaymentV2(
lnr.SendPaymentRequest(
payment_request=str(invoice_response.payment_request),
fee_limit_msat=int(rebalance.fee_limit * 1000),
outgoing_chan_ids=chan_ids,
last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey),
timeout_seconds=(timeout - 5),
allow_self_payment=True,
max_parts=max_parts # Set max_parts based on the enable_mpp value
),
timeout=(timeout + 60)
):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit of background:
The lightning network has either MPP Payment or AMP payments (lnd only). An MPP payment can also be of size 1. So when selecting None
here, this will trigger the default behaviour of 16 shards (default) on the lnd side.
I would rather name this setting MaxShards and default it to 1, and allow the user to set it to the default value if he likes?
Btw, value none
works but ChatGPT said it won't, never trust an AI !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's safer to adhere to the standard-payment algo LND already uses, and not define MaxShards arbitrarily.
So we either set it to 1 (which effectively is MPP deactivated nonetheless), or none
by default to follow the LND standards. In case LND changes it to another number than 16, we have code tax which needs to be manually adjusted.
I think I had a typo in the max_parts =
algo, it should be None
if allow_multishards = True
, not the other way around as stated above.
Please review
# Define LocalSettings
class LocalSettings:
def __init__(self):
self.allow_multishards = True # Default value is True
# Check if LocalSetting LND-EnableMPP exists and set allow_mpp accordingly
if LocalSettings.objects.filter(key='LND-MaxShards').exists():
if int(LocalSettings.objects.filter(key='LND-MaxShards')[0].value) > 0:
allow_multishards = True
else:
LocalSettings(key='LND-MaxShards', value='0').save() # Set with 0 value for the future
allow_multishards = False
else:
allow_multishards = True # Default value is True. Not sure we need it again
max_parts = None if allow_multishards else 1 # Adjust max_parts based on the allow_multishards value
async for payment_response in routerstub.SendPaymentV2(
lnr.SendPaymentRequest(
payment_request=str(invoice_response.payment_request),
fee_limit_msat=int(rebalance.fee_limit * 1000),
outgoing_chan_ids=chan_ids,
last_hop_pubkey=bytes.fromhex(rebalance.last_hop_pubkey),
timeout_seconds=(timeout - 5),
allow_self_payment=True,
max_parts=max_parts # Set max_parts based on the allow_multishards value
),
timeout=(timeout + 60)
):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds reasonable let's do it like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, the self declarations are not needed at the top. Maybe we could rename the variable since its more of an on/off here than a max shards setting. I think something like LND-DisableMPP
makes sense, since the user is deviating from default LND behavior.
I'm running non-MPP for 48h on my main node and it's working fine.
This proposed change intends to reduce the number of HTLC shards LNDg fires to rebalance.
Up for review, thanks