fix(pseudosettle): bound the first refreshment of a connection#5531
fix(pseudosettle): bound the first refreshment of a connection#5531mfw78 wants to merge 2 commits into
Conversation
The time-based allowance a node grants an incoming refreshment is computed as (now - lastSettlementTime) * refreshRate. On the first refreshment from a peer there is no stored settlement time, so the anchor defaulted to the zero Unix epoch, making the allowance (now - 0) * refreshRate. This lets the very first refreshment forgive an effectively unbounded debt, decoupled from how long the peer has actually been connected. Anchor the first-contact allowance to the peer's connection time instead, so the initial refreshment only forgives debt accrued over the elapsed connected time. Subsequent settlements are unchanged.
|
What about the payer side, should it match the peerAllowance? |
|
The payer derives its allowance expectation from the same defaulted timestamp as the recipient, so bounding only the recipient makes the payer expect more than is granted and blocklist it. Bound the first refreshment of a connection to a single refresh interval on both sides instead of anchoring at connection time, which would couple the bound to two independently measured clocks. Reconnection is treated as a first refreshment, so allowance no longer accrues while a peer is disconnected.
mfw78
changed the title
|
Good catch, it does have to match, and leaving it unmatched would have been worse than the original bug.
Pushed a change that applies the bound on both sides. I dropped the connection-time anchor while doing so: the amount being bounded is of the same order as the clock skew the protocol already tolerates, 2 seconds in That also covers reconnection, which the first version missed. The stored record survives a disconnect, so a peer returning after an hour was granted an hour of allowance against a connection seconds old. It now gets a single interval, same as first contact. On the test side, Two things worth your call: Rollout. An unpatched payer still expects the full attempted amount on first contact, so it will blocklist a patched recipient for roughly Magnitude. #5530 overstates this and I would rather correct it than have it read as more than it is. The allowance is capped by the peer's debt, which the recipient already caps at |
|
GitHub closed this on its own and refuses to reopen it: the head repository it was raised from is private and detached from the fork network, so the pull request is not recoverable. Continued in #5542, raised from a public fork with the same branch and commits. @gacevicljubisa your question is answered there. |
2 participants
Checklist
Description
peerAllowancecomputes the acceptable incoming payment as(now - lastSettlementTime) * refreshRate. With no stored settlement recordlastSettlementTimedefaults to0, so the first refreshment from a peer is granted the whole Unix epoch scaled by the refresh rate and forgives its entire outstanding debt in one go. The same holds on reconnection: the stored record survives the disconnect, so a peer returning after an hour is granted an hour of allowance for a connection that is seconds old.The bound cannot be applied to the recipient alone.
NotifyRefreshmentSentrecomputes the expectation asmin(allegedInterval * refreshRate, attemptedAmount)and blocklists the peer when the accepted amount falls short, andallegedIntervalis derived from the payer's own stored record, which defaults to0in exactly the same way. Both ends defaulting to the epoch is what makes them agree today, so tightening one side alone would make a patched recipient accept less than every peer expects and be blocklisted on the first refreshment.This bounds the first refreshment of a connection to a single refresh interval, applied identically on the recipient and the payer. Refreshments after the first use the stored timestamps as before, and reconnection is treated as a first refreshment so allowance no longer accrues while a peer is disconnected.
Anchoring at the connection time was considered and dropped. The quantity being bounded is of the same order as the clock skew the protocol already tolerates, 2 seconds in
ErrTimeOutOfSyncRecentagainst a default payment threshold of 3 seconds of refresh rate, so a bound derived from two independently measured clocks would be unreliable, and slack added to absorb the skew would be claimable on every refreshment. Treating an absent or pre-connection timestamp as one interval needs no clock comparison between peers and is identical on both ends by construction.On rollout: an unpatched payer still expects the full attempted amount on first contact, so it will blocklist a patched recipient for roughly
(debt + paymentThreshold) / refreshRateseconds on each new peer until the network has upgraded. The payer half is a pure relaxation and can never cause a blocklist, so it can be released on its own first with the recipient bound following later if you would rather not carry that. Happy to split it that way.One correction to #5530: the allowance is capped by the peer's debt, which the recipient already caps at
disconnectLimit, so the exposure is one payment threshold plus tolerance per new peer identity rather than an unbounded amount. Still worth closing off given identities are cheap, but the issue overstates the magnitude.Open API Spec Version Changes (if applicable)
None.
Motivation and Context (Optional)
See #5530.
Related Issue (Optional)
Closes #5530.
Screenshots (if appropriate):
N/A
AI Disclosure