Accountable HTLCs #3217
Accountable HTLCs #3217
Conversation
thomash-acinq
force-pushed
the
accountability
branch
4 times, most recently
from
c44e38f to
7e21640
Compare
thomash-acinq
force-pushed
the
accountability
branch
6 times, most recently
from
f8160e5 to
d807c65
Compare
Add accountability signal for HTLCs, it replaces endorsement. See lightning/bolts#1280
thomash-acinq
force-pushed
the
accountability
branch
from
d807c65 to
1a162d1
Compare
t-bast
left a comment
t-bast
left a comment
There was a problem hiding this comment.
This is simpler and less invasive than the previous endorsement mechanism, I like it. Let's get that on master soon to avoid dealing with merge conflicts since a lot of tests are being updated (hopefully for the last time).
I think it's fine to set the accountability bit in the route blinding data even though it isn't in the spec yet, we can still change it on master in a follow-up PR if the final decision is slightly different than what we're doing.
| case WrappedReputationScore(score) => | ||
| new ChannelRelay(nodeParams, register, channels, r, upstream, score, context).start() | ||
| val accountable = r.add.accountable || incomingChannelOccupancy > 1 - nodeParams.relayParams.reservedBucket | ||
| if (accountable && !r.payload.upgradeAccountability) { |
There was a problem hiding this comment.
If we include this change, this isn't a "shadow deployment" anymore: we will reject payments that don't use the accountable TLV (existing nodes on the network) whenever the incoming channel is more than half full. We should only log this information for now but not reject HTLCs yet. I've fixed that in #3229.
| val payment = if (useMultiPart) { | ||
| SendMultiPartPayment(payFsmAdapters, recipient, maxPaymentAttempts, routeParams) | ||
| val accountable = upstream.accountable || upstream.incomingChannelOccupancy > 1 - nodeParams.relayParams.reservedBucket | ||
| if (accountable && !upgradeAccountability) { |
There was a problem hiding this comment.
Same comment here about keeping it a "shadow deployment" for now (also fixed in #3229).
| // @formatter:off | ||
| sealed trait Command | ||
| case class GetConfidence(replyTo: ActorRef[Reputation.Score], upstream: Upstream.Hot, downstream_opt: Option[PublicKey], fee: MilliSatoshi, currentBlockHeight: BlockHeight, expiry: CltvExpiry) extends Command | ||
| case class GetConfidence(replyTo: ActorRef[Reputation.Score], downstream_opt: Option[PublicKey], fee: MilliSatoshi, currentBlockHeight: BlockHeight, expiry: CltvExpiry, accountable: Boolean) extends Command |
There was a problem hiding this comment.
The downstream node_id is always known at that point, we don't need this to be an Option. I've removed the Option in #3229.
| private val pathKey: Codec[PathKey] = (("length" | constant(hex"21")) :: ("pathKey" | publicKey)).as[PathKey] | ||
|
|
||
| case class Endorsement(level: Int) extends UpdateAddHtlcTlv | ||
| case object Accountable extends UpdateAddHtlcTlv |
There was a problem hiding this comment.
We've had issues in the past with case objects and scodec, I couldn't find the exact reason for it in the git history, but I think it's better to be consistent with other similar TLVs (e.g. RequireConfirmedInputs) and use a case class, which allows using the tlvField() helper codec and the TlvStream.get[T] helper method.
I've changed that in the first commit of #3229.
| sealed trait StandardPayload extends PerHopPayload { | ||
| def upgradeAccountability: Boolean = records.records.contains(UpgradeAccountability) | ||
| } |
There was a problem hiding this comment.
This isn't necessary, we can put this field directly in PerHopPayload and override it in BlindedPayload, which simplifies the trait hierarchy and reduces the diff with master. Removed in #3229.
| assert(r.getConfidence(1000 msat, 0, BlockHeight(0), CltvExpiry(1), TimestampMilli(0) + 1.hour) < 0.000001) | ||
| } | ||
|
|
||
| test("multiple endorsement levels") { |
There was a problem hiding this comment.
We should remove the mention of endorsement, it's confusing since the spec only talk about accountability now. I've renamed those tests in #3229. They're a bit confusing because they get exactly the same result regardless of whether we set accountable = true or accountable = false, but that's because we're reusing the previous bucketing strategy that extends to more than just 2 buckets.
| } | ||
| walletNodeId_opt match { | ||
| case Some(walletNodeId) if shouldAttemptOnTheFlyFunding(remoteFeatures_opt, previousFailures) => RelayNeedsFunding(walletNodeId, cmdFail) | ||
| case Some(walletNodeId) if shouldAttemptOnTheFlyFunding(remoteFeatures_opt, previousFailures) && !accountable => RelayNeedsFunding(walletNodeId, cmdFail) |
There was a problem hiding this comment.
I'm not sure why you're adding the accountable check here when deciding whether to perform on-the-fly funding. Since this boolean can be true whenever the incoming channel is more than half-full, even though nodes aren't setting the accountability TLV, I don't think we should do this yet: we don't want to make payments fail that would have otherwise succeeded, we only want to collect data for now. I've removed this in #3229.
Add accountability signal for HTLCs, it replaces endorsement.
See lightning/bolts#1280