htlcswitch: remove the bucket batchReplayBucket
#9929
Conversation
yyforyongyu
added
database
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| // failure error code. We infer that the offending error was due | ||
| // to a replayed packet because this index was found in the | ||
| // replay set. | ||
| if !reforward && replays.Contains(uint16(i)) { |
There was a problem hiding this comment.
but what if is a reforward and was a replay in the first run, we would now pass it through and forward it ?
There was a problem hiding this comment.
yeah i thought about it too, unless the peer can crash all the nodes down the path I don't think the attack can succeed tho. The first time a replayed htlc is received, the node will fail back and wait for the remote's revoke_and_ack. Unless the node crashes and restarts to reload the htlc pkg, the replayed htlc won't be forwarded. Even if this edge case happens, the next hop will fail it, so I think it's extremely unlikely? btw lnd_sweep_test.go is a good place to observe the overall flow.
There was a problem hiding this comment.
It does not need to crash know, the link only has to flap and then we are in that situation.
There was a problem hiding this comment.
this is called when a Link starts:
Lines 1327 to 1328 in 35102e7
There was a problem hiding this comment.
explained in this comment
There was a problem hiding this comment.
It seems like an acceptable way forward however we are accepting a case where we might flap on the incoming link and therefore would reforward a potential "replayed" HTLC (which very likely be detected by the next node as a replay if it is LND). It might be very unlikely that it happens so I am ok with going with this approach. The proper solution would be to add a new Relayed-Fwd-Filter into the forwarding logic and query it when the above happens.
| // failure error code. We infer that the offending error was due | ||
| // to a replayed packet because this index was found in the | ||
| // replay set. | ||
| if !reforward && replays.Contains(uint16(i)) { |
There was a problem hiding this comment.
It does not need to crash know, the link only has to flap and then we are in that situation.
| } | ||
| } | ||
|
|
||
| // If the fwdPkg has already been processed, it means we are | ||
| // reforwarding the packets again, which happens only on a restart. |
There was a problem hiding this comment.
On restart of the channel not on restart of the LND server, or do you mean the same ?
There was a problem hiding this comment.
restart of lnd, tho link restart is a superset?
There was a problem hiding this comment.
yes but what I wanted to say is, flaping of a link is way more likely than crashing a node, so just want to make sure we take this into account when talking about the edge case. But then can you say only on restart of the link ?
There was a problem hiding this comment.
Yes, if it's just the flapping of a link, it means it will go through the full cycle of start/stop, which has all the necessary data persisted. If a replay has been detected before, we will call sendMalformedHTLCError to store it in updateLogs, and update the commitment via AppendRemoteCommitChain, which will remove the Adds. Then, if the link restarts, it will restore the state, but the fwdPkg will have empty logs, as shown in the experiment here yyforyongyu#47. So as long as the link is going through normal restarts, we are protected. The only possible way is that somehow the remote node can crash our node, causing us to skip AppendRemoteCommitChain before the link shuts down, which is a different level of assumption I think, also don't think it's feasible.
There was a problem hiding this comment.
There is a time period between processing the incoming ADD and updating the Channel via AppendRemoteCommitChain which is called within the ChannelLink object where we might run in the scenario described above.
Scenario:
- Incoming replayed ADD comes in on the incoming, we process it (DecodeHopInterator etc.)
- We figure out that it is a Replay
- Call
sendMalformedHTLCError - Before we can call
AppendRemoteCommitChain(batchtimer hits => default 50ms) => Link flaps - MalFormed msg never persited on the State => meaning we will forward the Incoming Replayed ADD the next time we start-up the link.
There was a problem hiding this comment.
I think it will be a good argument if we assume the user will use a different batch commit interval as 50ms is a bit too short. That being said I think we do need to perform a commitment update, which is missing from this method when we fail some of the ADDs in processRemoteAdds.
The replay check also seems to be performed a bit late - we should instead perform this check when we receive an update_add_htlc, not revoke_and_ack. I'll see how easy it is to change that.
There was a problem hiding this comment.
I would keep it as is, I would propose that we add a comment that we are aware of this edge case and are willing to accept it (see my other comment #9929 (comment))
There was a problem hiding this comment.
Actually I don't think it relies on the batch ticker as there will be an immediate commitment update after calling processRemoteAdds,
Lines 2541 to 2568 in 35102e7
Think I will keep this PR as is and create a new one to address that we need to check replay when receiving update_add_htlc, maybe fix it in the dyn series.
yyforyongyu
force-pushed
the
fix-decalylog
branch
2 times, most recently
from
2ce3831 to
cd880a8
Compare
I think it's better than keeping the batch delay db then adding another key to bloat the disk. Also can you think of a case where a replay can happen without crashing the node? |
yyforyongyu
force-pushed
the
fix-decalylog
branch
from
cd880a8 to
46d28f9
Compare
| } | ||
| } | ||
|
|
||
| // If the fwdPkg has already been processed, it means we are | ||
| // reforwarding the packets again, which happens only on a restart. |
There was a problem hiding this comment.
yes but what I wanted to say is, flaping of a link is way more likely than crashing a node, so just want to make sure we take this into account when talking about the edge case. But then can you say only on restart of the link ?
htlcswitch/link.go
Outdated
| DecodeHopIterators func([]byte, []hop.DecodeHopIteratorRequest) ( | ||
| // are always presented for the same identifier. The last boolean is | ||
| // used to decide whether this is a reforwarding or not - when it's | ||
| // reforwarding, we skip the replay check enforced in our decaly log. |
There was a problem hiding this comment.
Here we should mention the edge case which we talked about, that in a very unlikely scenario an ADD can be reforwared and if it is a replay it will be forwarded to the next node.
| // PutBatch accepts a pending batch of hashed secret entries to write to disk. | ||
| // Each hashed secret is inserted with a corresponding time value, dictating | ||
| // when the entry will be evicted from the log. |
There was a problem hiding this comment.
I think we need to properly cleanup also the underlying replay set. No batch is written here anymore, also the tx.Commit() call does not commit anything anymore. So I think we should clean that up with either this PR or a followup.
There was a problem hiding this comment.
Discussed offline and @ziggie1984 will create a follow-up PR to clean this.
| // resulting and return it to ensure calls to put batch are | ||
| // idempotent. | ||
| replayBytes := batchReplayBkt.Get(b.ID) | ||
| if replayBytes != nil { |
There was a problem hiding this comment.
Just want to point out that we will for reforward have more db roundtrips compared to the previous implementation if we have more than 1 incoming ADD in the fwd package.
There was a problem hiding this comment.
Will be fixed in a follow up PR, I think we can skip the whole fetching of shared_secrets if it is a replay because we will mark them as replayed anyways in the refwd case,
so should probably be something link tx.Commit(reforward) => return all replays.
yyforyongyu
force-pushed
the
fix-decalylog
branch
from
46d28f9 to
b51b4d0
Compare
yyforyongyu
force-pushed
the
fix-decalylog
branch
from
b51b4d0 to
9b7699f
Compare
htlcswitch/link.go
Outdated
| DecodeHopIterators func([]byte, []hop.DecodeHopIteratorRequest) ( | ||
| // are always presented for the same identifier. The last boolean is | ||
| // used to decide whether this is a reforwarding or not - when it's | ||
| // reforwarding, we skip the replay check enforced in our decaly log. |
| @@ -3632,6 +3632,13 @@ func (l *channelLink) processRemoteSettleFails(fwdPkg *channeldb.FwdPkg) { | |||
| return | |||
| } | |||
|
|
|||
| // Exit early if the fwdPkg is already processed. | |||
| if fwdPkg.State == channeldb.FwdStateCompleted { | |||
There was a problem hiding this comment.
Cool. This is fine as a short circuit, as if the state is channeldb.FwdStateCompleted, then the check to fwdPkg.SettleFailFilter.Contains(uint16(i)) in the inner loop will always return true. Same for when we go to process adds.
| reforward := fwdPkg.State != channeldb.FwdStateLockedIn | ||
| // If the fwdPkg has already been processed, it means we are | ||
| // reforwarding the packets again, which happens only on a restart. | ||
| reforward := fwdPkg.State == channeldb.FwdStateProcessed |
There was a problem hiding this comment.
Just defined in this commit, but the useage in teh call site below was already present.
There was a problem hiding this comment.
This is actually introduced in a previous commit, where we added reforward := fwdPkg.State != channeldb.FwdStateLockedIn, and since FwdStateCompleted is skipped, we can now only check fwdPkg.State == channeldb.FwdStateProcessed.
| @@ -138,11 +132,6 @@ func (d *DecayedLog) initBuckets() error { | |||
| return ErrDecayedLogInit | |||
| } | |||
|
|
|||
| _, err = tx.CreateTopLevelBucket(batchReplayBucket) | |||
There was a problem hiding this comment.
Should we modify this to instead always try to delete the bucket on start up? Or will we reserve that for a distinct PR that handles the optional migration?
There was a problem hiding this comment.
Correct - the migration is done in #9945.
| replayBytes := batchReplayBkt.Get(b.ID) | ||
| if replayBytes != nil { | ||
| replays = sphinx.NewReplaySet() | ||
| return replays.Decode(bytes.NewReader(replayBytes)) |
There was a problem hiding this comment.
Why do we return replays at all, if we no longer decode the bytes from disk?
There was a problem hiding this comment.
Nvm, the sharedHashes bucket is just the primary source of truth.
Is there any situation wherein by only consulting sharedHashes we'd let through a replay that was already captured in the batchReplayBkt bucket?
IIRC historically, all onion replay processing was serial. We then introduced the batch replay bucket and processing to speed things up. Compared to the shared hashes, the batch replay bucket uses an ID derived from the source channel ID and the height of the outbound commitment that locked in those changes.
In retrospect, it's clear now that we were duplicating data across both sources, as replaying an HTLC is defined as using the very same shared secret, and assuming the same sender, that forces re-use of the payment hash as it's included in the associated data.
There was a problem hiding this comment.
Is there any situation wherein by only consulting sharedHashes we'd let through a replay that was already captured in the batchReplayBkt bucket?
Don't think so. As for a replay to happen it must use a new commitment height, which means it will create a new key to query batchReplayBkt, then find nothing, and continue to query sharedHashes. So in the end it's the sharedHashes as the source of truth. This batchReplayBkt is only helpful for a very specific case - the node receives replays, but before it persists its forwarding pkg, it goes offline. When it comes online again, it will reload the forwarding logs, then query batchReplayBkt instead of the sharedHashes to find the replays and fail the HTLCs back.
In retrospect, it's clear now that we were duplicating data across both sources, as replaying an HTLC is defined as using the very same shared secret, and assuming the same sender, that forces re-use of the payment hash as it's included in the associated data.
Totally - think eventually we can move the replay attack protection to be handled at the forwarding logs instead. Also given sharedHashes are GCed based on the CLTV values, which also means the replays are allowed to happen after CLTV blocks. So as long as we are within CLTV blocks we are protected.
We now rely on the forwarding package's state to decide whether a given packet is a reforwarding or not. If we know it's a reforwarding packet, there's no need to check for replays in the `sharedHashes` bucket, which behaves the same as if we are querying the `batchReplayBkt`.
This commit removes the `batchReplayBkt` as its only effect is to allow reforwarding htlcs during startup. Normally for every incoming htlc added, their shared secret is used as the key to be saved into the `sharedHashBucket`, which will be used for check for replays. In addition, the fwdPkg's ID, which is SCID+height is also saved to the bucket `batchReplayBkt`. Since replays of HTLCs cannot happen at the same commitment height, when a replay happens, `batchReplayBkt` simply doesn't have this info, and we again rely on `sharedHashBucket` to detect it. This means most of the time the `batchReplayBkt` is a list of SCID+height with empty values. The `batchReplayBkt` was previously used as a mechanism to check for reforwardings during startup - when reforwarding htlcs, it quries this bucket and finds an empty map, knowing this is a forwarding and skips the check in `sharedHashBucket`. Given now we use a bool flag to explicitly skip the replay check, this bucket is no longer useful.
We now move the check earlier in the loop so we don't even need to decode the hop for already processed ADDs.
yyforyongyu
force-pushed
the
fix-decalylog
branch
from
9b7699f to
7d05870
Compare
yyforyongyu
force-pushed
the
fix-decalylog
branch
from
7d05870 to
2defb11
Compare
This PR simplifies the htlc reforwarding logic by dropping the batch replay bucket.
Context
When an htlc is received, we call
processRemoteAdds, which invokesDecodeHopIteratorsto decode the onion.lnd/htlcswitch/link.go
Lines 3771 to 3773 in 92a5d35
We then create a
Txwhich has a memory batch inBeginTxn, and process it inProcessOnionPacket,lnd/htlcswitch/hop/iterator.go
Line 756 in 92a5d35
lnd/htlcswitch/hop/iterator.go
Line 786 in 92a5d35
This
ProcessOnionPacketthen call thisb.Putwhich saves the htlc:IsCommittedis true, error out.b.replayCacheand, use itsseqNumas key andbatchEntryas the value to mapb.entries. ThebatchEntryhas the fieldshashPrefixandcltv.seqNumto itsb.ReplaySet, which is a map.We then proceed to the
tx.Commit, which callsPutBatchunder the hood,lnd/htlcswitch/hop/iterator.go
Line 831 in 92a5d35
It will also be noop if
IsCommittedis true.Now, in this
PutBatch,ReplaySetbatchReplayBucket, ifb.IDis found, which is thefwdPkg.ID(), we return the savedReplaySeton the disk - most of the time it's an empty map. This is also the mechanism to skip checking replays when reforwarding htlcs during restart.b.ForEach. We will iterate the mapb.entriesthere,sharedHashesbucket, if we find an item there, we add this entry'sseqNumtoReplaySet. Note that when the item is found insharedHashesbucket, it means it's a replay.shareHashesbucket, we will save the item'shashPrefixand cltv to disk.ReplaySetwith the batch's internal replay set,b.ReplaySet.ReplaySeton disk inbatchReplayBucket.The actual replay attack protection comes in the last step in
ProcessOnionPacket, where we would check whether any of the decoded htlcs is found in theReplaySet.lnd/htlcswitch/hop/iterator.go
Lines 871 to 873 in 92a5d35
The Fix
We now drop this table and explicitly signal reforwarding during startup to skip the replay check. This means,
In combination with #9906 we can sunset this bucket completely.