-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Persist MPP resolution status in wallet file. #9083
base: master
Are you sure you want to change the base?
Conversation
class RecvMPPResolution(Enum): | ||
WAITING = enum.auto() | ||
EXPIRED = enum.auto() | ||
ACCEPTED = enum.auto() | ||
FAILED = enum.auto() |
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.
We should add a comment here about these values being persisted in the wallet file, and hence being sensitive to changes. As in:
electrum/electrum/lnchannel.py
Lines 77 to 79 in 02a9ab8
# Note: these states are persisted by name (for a given channel) in the wallet file, | |
# so consider doing a wallet db upgrade when changing them. | |
class ChannelState(IntEnum): |
Btw, like with the channel states, we could potentially persist them by name instead of value. It only matters if we end up changing this enum, and how often. The ChannelState enum has been changed several times and hence in that case it was quite helpful to use the names.
e83a981
to
04de266
Compare
mpp_status = self.received_mpp_htlcs[payment_key] | ||
self.received_mpp_htlcs[payment_key] = mpp_status._replace(resolution=resolution) | ||
mpp_status = self.received_mpp_htlcs[payment_key.hex()] | ||
self.logger.info(f'set_mpp_resolution {resolution.name} {len(mpp_status.htlc_set)}') |
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.
Log line not very useful in non-trivial scenarios. It should also contain the payment_key or the payment_hash, or sth ~id-like.
Same goes for other log lines touched.
if not mpp_status: | ||
return int(time.time()) | ||
return min([_htlc.timestamp for scid, _htlc in mpp_status.htlc_set]) | ||
|
||
def maybe_cleanup_forwarding( | ||
def maybe_cleanup_mpp( |
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.
What about channels that get closed, do we also want to clean-up the received_mpp_htlcs
dict for those?
If we accept a MPP and we forward the payment (trampoline or swap), we need to persist the payment accepted status, or we might wrongly release htlcs on the next restart. lnworker.received_mpp_htlcs used to be cleaned up in maybe_cleanup_forwarding, which only applies to forwarded payments. However, since we now persist this dict, we need to clean it up also in the case of payments received by us. This part of maybe_cleanup_forwarding has been migrated to lnworker.maybe_cleanup_mpp
04de266
to
7e38237
Compare
If we accept a MPP and we forward the payment (trampoline or swap), we need to persist the payment accepted status, or we might wrongly release htlcs on the next restart.
lnworker.received_mpp_htlcs used to be cleaned up in maybe_cleanup_forwarding, which only applies to forwarded payments. However, since we now persist this dict, we need to clean it up also in the case of payments received by us. This part of maybe_cleanup_forwarding has been migrated to lnworker.maybe_cleanup_mpp