Repository navigation
multi: refuse splices after shutdown; review fixes for the next release - #16
Merged
Merged
Conversation
A splice of ours asks the link to quiesce, and the quiescer holds the request until our own updates are locked in before sending stfu. If the peer's shutdown arrived in that window, the stfu still went out after it, followed by splice_init once the peer answered. BOLT 2 allows no splice once shutdown is sent, and the Core Lightning port refused the splice with a warning and dropped the connection. The link now has ShutdownReceived, called as the peer's shutdown is read. The quiescer refuses our requests for quiescence from then on with ErrShutdownReceived, and withdraws one whose stfu hasn't gone out, answering it with that error. An stfu of ours already sent stays out, and the peer's own stfu is still answered. The refusal survives a resume and lasts as long as the link, which a reconnection replaces; the peer sends shutdown again then. Upstream's quiescer has no notion of shutdown: nothing there starts quiescence on its own yet.
The machine refused a splice once we had sent shutdown, which it reads
from the channel's state. The peer's shutdown wasn't known to it until
our answer, which the close sends only once the link is flushed: in a
lab run against the Core Lightning port, a splice requested half a
second after the peer's shutdown sent stfu and splice_init, which the
peer refused with a warning ("Splice refused: the channel is closing")
before disconnecting.
The machine now has ShutdownReceived, called as the peer's shutdown is
read. From then on:
- a splice of ours is refused with "closing", as is one whose
quiescence completes later, and Refusal reports "closing", so the
estimate and the channel's splice_status show it;
- a splice of ours still waiting for quiescence fails with "closing"
at once, its coins released; if the peer still answers our stfu, a
tx_abort ends the quiescence we won. A request the link withdraws
for the shutdown fails as "closing" rather than "not_quiescent";
- the peer's splice_init or tx_init_rbf gets a warning and the
connection is closed, as BOLT 2 requires of a node that received
shutdown.
With only our shutdown sent, the peer's splice_init is still answered
with tx_abort "closing": the peer may not have read our shutdown before
sending it, and BOLT 2 asks a warning only for a shutdown received.
tx_init_rbf now gets the same check.
The read handler canceled a splice of ours still waiting for quiescence only when the peer's shutdown arrived with a splice under way, and told nothing else of it. A splice requested between the peer's shutdown and our answer to it was let through. Every shutdown read now goes to the channel's splice machine and link first (spliceShutdown, replacing spliceYields): the machine refuses splices from then on and fails ours still waiting for quiescence, and the link withdraws an stfu of ours not yet sent. The check for a splice that hasn't locked follows as before.
Messages the link holds for a splice step were bounded by count alone, 1000 of them. The mailbox hands each over as it comes, so its 4 MiB budget stayed near empty while the link held them: a peer could have us hold about 998 update_add_htlc padded with an odd record to 61 KB each, some 58 MiB a channel, until the wait ended a minute later. The held messages are now charged against the mailbox's budget as well, with its own size function, and the link fails with a disconnect over either bound. A replay gives back what it takes up. TestLinkHeldBounded's new case hands over 70 such messages one at a time, the mailbox draining between them; without the change the link never fails.
The link takes any tx_abort while quiescent as the end of the peer's quiescence and holds the peer's updates until ours ends. The machine ended it for every tx_abort but an empty one with no splice of ours, which it only logged: the peer's updates were then held until the wait for them ran out and the link dropped the peer, and a peer could repeat that, stfu then the empty tx_abort, to have the link hold its messages a minute at a time. Such a tx_abort is still not echoed, but now ends our quiescence as a non-empty one does. TestTxAbortWithoutSplice checks it; without the change it waits for the resume in vain.
PayOffer kept the invoice it was given, trimmed, as the payment's request, failed attempts included, since the payment is recorded before any route is sought. bolt12.Decode takes strings up to 3.36 MB, with '+' continuations, either case, and unknown odd records, so a caller could have that much stored per payment hash. The invoice is now refused when its data is over what an onion message carries (sphinx.MaxOnionMessagePayloadSize), which bounds every invoice fetched, and kept in its canonical form: re-encoded from its data, lowercase and without continuations. The data is unchanged, so the signature still holds. The local variable that shadowed the bolt12 package is renamed. TestPayInvoiceStoredCanonical pays an uppercase invoice split with a continuation and a re-signed one padded past 32 KiB; without the change the first is stored as given and the second is paid.
InitPayment stored any payment request it was given. Callers bound theirs (a BOLT 11 request by its decoder, a BOLT 12 invoice by the size of an onion message), but nothing in the store did, so one caller that didn't could fill it. Both stores now refuse a request over MaxPaymentRequestSize with ErrPaymentRequestTooLarge, before touching the database. TestInitPaymentRequestSize, run against kv and SQL, refuses one a byte over the limit and stores one at it; without the change both are stored.
The KV to SQL migration recorded every payment request as a BOLT11 intent, while the native SQL store records a BOLT 12 invoice (lni1...) as intent type 1. A node paying offers on kv and later migrating would have had those payments labelled BOLT11. The migration now picks the type as the native store does, by the lni1 prefix in either case. TestMigratePaymentIntentType migrates BOLT11 and BOLT 12 requests and reads the type back; without the change the BOLT 12 ones come out as 0.
Keeping protocol.custom-init and protocol.custom-nodeann off the bits of option_independent_secrets, the feature manager refused every bit lnd defines. 0.21.4-beta-blake2b.21.1 took a known bit left unset, such as keysend (55) in init, splice (63) or payment_metadata (49), and refused only one already set, so a configuration that set any of them would now stop the node from starting. Only bits 266 and 267 are refused now, the reason for the check; the others are taken as before. TestCustomFeaturesRefuseKnownBits sets four known bits left unset, which fail without the change, and checks one already set is still refused.
The option's description said the database moves to version 39 once any channel has used the add-on, which reads as a channel that opened and ran. It moves as soon as such a channel is saved, which for the fundee is on funding_created, before any funding transaction exists, so a peer only starting an open is enough. Say that, and that older releases then refuse to open the database. sample-lnd.conf follows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes from an adversarial review of the next release, and lnd refusing splices once the peer has sent
shutdown.closingonce the peer'sshutdownhas been read; a pendingstfuis withdrawn and coin leases released; a peer'ssplice_initortx_init_rbfafter its ownshutdowngets a warning and a disconnect (BOLT 2). Found against the CLN port, which refused such a splice.tx_abortwith no splice ends quiescence instead of leaving messages held.InitPaymentrefuses a payment request over 64 KiB.protocol.independent-secretshelp says the database moves to version 39 as soon as a channel with the add-on is saved, even one a peer only starts opening.Each change has a test that fails without it.