1
0
Fork 0
mirror of https://github.com/ACINQ/eclair.git synced 2025-02-22 06:21:42 +01:00

Proper handling of gossiped channels being spent (#838)

While it makes sense to exclude from the routing table channels for
which there is a spending tx in the mempool, we shouldn't blame our
peers for considering the channel still opened if the spending tx hasn't
yet been confirmed.

Also, reworked `ValidateResult` with better types. It appears that the
`NonexistingChannel` error wasn't really useful (a malicious peer would
probably point to an existing funding tx, so there is no real difference
between "txid not found" and "invalid script"), so it was replaced by
`InvalidAnnouncement` error which is a more serious offense (punished
by a disconnection, and probably a ban when we implement that sort of
things).
This commit is contained in:
Pierre-Marie Padiou 2019-01-31 16:03:01 +01:00 committed by GitHub
parent 4db9c8daf7
commit 1da0017311
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 72 additions and 81 deletions

View file

@ -77,6 +77,11 @@ final case class WatchEventLost(event: BitcoinEvent) extends WatchEvent
*/
final case class PublishAsap(tx: Transaction)
final case class ValidateRequest(ann: ChannelAnnouncement)
final case class ValidateResult(c: ChannelAnnouncement, tx: Option[Transaction], unspent: Boolean, t: Option[Throwable])
sealed trait UtxoStatus
object UtxoStatus {
case object Unspent extends UtxoStatus
case class Spent(spendingTxConfirmed: Boolean) extends UtxoStatus
}
final case class ValidateResult(c: ChannelAnnouncement, fundingTx: Either[Throwable, (Transaction, UtxoStatus)])
// @formatter:on

View file

@ -19,7 +19,7 @@ package fr.acinq.eclair.blockchain.bitcoind.rpc
import fr.acinq.bitcoin._
import fr.acinq.eclair.ShortChannelId.coordinates
import fr.acinq.eclair.TxCoordinates
import fr.acinq.eclair.blockchain.ValidateResult
import fr.acinq.eclair.blockchain.{UtxoStatus, ValidateResult}
import fr.acinq.eclair.wire.ChannelAnnouncement
import org.json4s.JsonAST._
@ -153,8 +153,16 @@ class ExtendedBitcoinClient(val rpcClient: BitcoinJsonRPCClient) {
}
tx <- getRawTransaction(txid)
unspent <- isTransactionOutputSpendable(txid, outputIndex, includeMempool = true)
} yield ValidateResult(c, Some(Transaction.read(tx)), unspent, None)
fundingTxStatus <- if (unspent) {
Future.successful(UtxoStatus.Unspent)
} else {
// if this returns true, it means that the spending tx is *not* in the blockchain
isTransactionOutputSpendable(txid, outputIndex, includeMempool = false).map {
case res => UtxoStatus.Spent(spendingTxConfirmed = !res)
}
}
} yield ValidateResult(c, Right((Transaction.read(tx), fundingTxStatus)))
} recover { case t: Throwable => ValidateResult(c, None, false, Some(t)) }
} recover { case t: Throwable => ValidateResult(c, Left(t)) }
}

View file

@ -44,7 +44,7 @@ class ElectrumWatcher(client: ActorRef) extends Actor with Stash with ActorLoggi
txIn = Seq.empty[TxIn],
txOut = List.fill(outputIndex + 1)(TxOut(Satoshi(0), pubkeyScript)), // quick and dirty way to be sure that the outputIndex'th output is of the expected format
lockTime = 0)
sender ! ValidateResult(c, Some(fakeFundingTx), true, None)
sender ! ValidateResult(c, Right((fakeFundingTx, UtxoStatus.Unspent)))
case _ => log.warning(s"unhandled message $message")
}

View file

@ -362,8 +362,16 @@ class Peer(nodeParams: NodeParams, remoteNodeId: PublicKey, authenticator: Actor
}
log.error(s"peer sent us a routing message with invalid sig: r=$r bin=$bin")
// for now we just return an error, maybe ban the peer in the future?
// TODO: this doesn't actually disconnect the peer, once we introduce peer banning we should actively disconnect
d.transport ! Error(CHANNELID_ZERO, s"bad announcement sig! bin=$bin".getBytes())
d.behavior
case InvalidAnnouncement(c) =>
// they seem to be sending us fake announcements?
log.error(s"couldn't find funding tx with valid scripts for shortChannelId=${c.shortChannelId}")
// for now we just return an error, maybe ban the peer in the future?
// TODO: this doesn't actually disconnect the peer, once we introduce peer banning we should actively disconnect
d.transport ! Error(CHANNELID_ZERO, s"couldn't verify channel! shortChannelId=${c.shortChannelId}".getBytes())
d.behavior
case ChannelClosed(_) =>
if (d.behavior.ignoreNetworkAnnouncement) {
// we already are ignoring announcements, we may have additional notifications for announcements that were received right before our ban
@ -375,18 +383,6 @@ class Peer(nodeParams: NodeParams, remoteNodeId: PublicKey, authenticator: Actor
setTimer(ResumeAnnouncements.toString, ResumeAnnouncements, IGNORE_NETWORK_ANNOUNCEMENTS_PERIOD, repeat = false)
d.behavior.copy(fundingTxAlreadySpentCount = d.behavior.fundingTxAlreadySpentCount + 1, ignoreNetworkAnnouncement = true)
}
case NonexistingChannel(_) =>
// this should never happen, unless we are not in sync or there is a 6+ blocks reorg
if (d.behavior.ignoreNetworkAnnouncement) {
// we already are ignoring announcements, we may have additional notifications for announcements that were received right before our ban
d.behavior.copy(fundingTxNotFoundCount = d.behavior.fundingTxNotFoundCount + 1)
} else if (d.behavior.fundingTxNotFoundCount < MAX_FUNDING_TX_NOT_FOUND) {
d.behavior.copy(fundingTxNotFoundCount = d.behavior.fundingTxNotFoundCount + 1)
} else {
log.warning(s"peer sent us too many channel announcements with non-existing funding tx (count=${d.behavior.fundingTxNotFoundCount + 1}), ignoring network announcements for $IGNORE_NETWORK_ANNOUNCEMENTS_PERIOD")
setTimer(ResumeAnnouncements.toString, ResumeAnnouncements, IGNORE_NETWORK_ANNOUNCEMENTS_PERIOD, repeat = false)
d.behavior.copy(fundingTxNotFoundCount = d.behavior.fundingTxNotFoundCount + 1, ignoreNetworkAnnouncement = true)
}
}
stay using d.copy(behavior = behavior1)
@ -539,8 +535,8 @@ object Peer {
sealed trait BadMessage
case class InvalidSignature(r: RoutingMessage) extends BadMessage
case class InvalidAnnouncement(c: ChannelAnnouncement) extends BadMessage
case class ChannelClosed(c: ChannelAnnouncement) extends BadMessage
case class NonexistingChannel(c: ChannelAnnouncement) extends BadMessage
case class Behavior(fundingTxAlreadySpentCount: Int = 0, fundingTxNotFoundCount: Int = 0, ignoreNetworkAnnouncement: Boolean = false)

View file

@ -27,7 +27,7 @@ import fr.acinq.eclair._
import fr.acinq.eclair.blockchain._
import fr.acinq.eclair.channel._
import fr.acinq.eclair.crypto.TransportHandler
import fr.acinq.eclair.io.Peer.{ChannelClosed, InvalidSignature, NonexistingChannel, PeerRoutingMessage}
import fr.acinq.eclair.io.Peer.{ChannelClosed, InvalidSignature, InvalidAnnouncement, PeerRoutingMessage}
import fr.acinq.eclair.payment.PaymentRequest.ExtraHop
import fr.acinq.eclair.router.Graph.GraphStructure.DirectedGraph.graphEdgeToHop
import fr.acinq.eclair.router.Graph.GraphStructure.{DirectedGraph, GraphEdge}
@ -195,25 +195,26 @@ class Router(nodeParams: NodeParams, watcher: ActorRef, initialized: Option[Prom
sender ! RoutingState(d.channels.values, d.updates.values, d.nodes.values)
stay
case Event(v@ValidateResult(c, _, _, _), d0) =>
case Event(v@ValidateResult(c, _), d0) =>
d0.awaiting.get(c) match {
case Some(origin +: others) => origin ! TransportHandler.ReadAck(c) // now we can acknowledge the message, we only need to do it for the first peer that sent us the announcement
case _ => ()
}
log.info("got validation result for shortChannelId={} (awaiting={} stash.nodes={} stash.updates={})", c.shortChannelId, d0.awaiting.size, d0.stash.nodes.size, d0.stash.updates.size)
val success = v match {
case ValidateResult(c, _, _, Some(t)) =>
case ValidateResult(c, Left(t)) =>
log.warning("validation failure for shortChannelId={} reason={}", c.shortChannelId, t.getMessage)
false
case ValidateResult(c, Some(tx), true, None) =>
case ValidateResult(c, Right((tx, UtxoStatus.Unspent))) =>
val TxCoordinates(_, _, outputIndex) = ShortChannelId.coordinates(c.shortChannelId)
// let's check that the output is indeed a P2WSH multisig 2-of-2 of nodeid1 and nodeid2)
val fundingOutputScript = write(pay2wsh(Scripts.multiSig2of2(PublicKey(c.bitcoinKey1), PublicKey(c.bitcoinKey2))))
if (tx.txOut.size < outputIndex + 1) {
log.error("invalid script for shortChannelId={}: txid={} does not have outputIndex={} ann={}", c.shortChannelId, tx.txid, outputIndex, c)
false
} else if (fundingOutputScript != tx.txOut(outputIndex).publicKeyScript) {
log.error("invalid script for shortChannelId={} txid={} ann={}", c.shortChannelId, tx.txid, c)
if (tx.txOut.size < outputIndex + 1 || fundingOutputScript != tx.txOut(outputIndex).publicKeyScript) {
log.error(s"invalid script for shortChannelId={}: txid={} does not have script=$fundingOutputScript at outputIndex=$outputIndex ann={}", c.shortChannelId, tx.txid, c)
d0.awaiting.get(c) match {
case Some(origins) => origins.foreach(_ ! InvalidAnnouncement(c))
case _ => ()
}
false
} else {
watcher ! WatchSpentBasic(self, tx, outputIndex, BITCOIN_FUNDING_EXTERNAL_CHANNEL_SPENT(c.shortChannelId))
@ -231,23 +232,20 @@ class Router(nodeParams: NodeParams, watcher: ActorRef, initialized: Option[Prom
}
true
}
case ValidateResult(c, Some(tx), false, None) =>
log.warning("ignoring shortChannelId={} tx={} (funding tx already spent)", c.shortChannelId, tx.txid)
d0.awaiting.get(c) match {
case Some(origins) => origins.foreach(_ ! ChannelClosed(c))
case _ => ()
case ValidateResult(c, Right((tx, fundingTxStatus: UtxoStatus.Spent))) =>
if (fundingTxStatus.spendingTxConfirmed) {
log.warning("ignoring shortChannelId={} tx={} (funding tx already spent and spending tx is confirmed)", c.shortChannelId, tx.txid)
// the funding tx has been spent by a transaction that is now confirmed: peer shouldn't send us those
d0.awaiting.get(c) match {
case Some(origins) => origins.foreach(_ ! ChannelClosed(c))
case _ => ()
}
} else {
log.debug("ignoring shortChannelId={} tx={} (funding tx already spent but spending tx isn't confirmed)", c.shortChannelId, tx.txid)
}
// there may be a record if we have just restarted
db.removeChannel(c.shortChannelId)
false
case ValidateResult(c, None, _, None) =>
// we couldn't find the funding tx in the blockchain, this is highly suspicious because it should have at least 6 confirmations to be announced
log.warning("could not retrieve tx for shortChannelId={}", c.shortChannelId)
d0.awaiting.get(c) match {
case Some(origins) => origins.foreach(_ ! NonexistingChannel(c))
case _ => ()
}
false
}
// we also reprocess node and channel_update announcements related to channels that were just analyzed
@ -332,7 +330,7 @@ class Router(nodeParams: NodeParams, watcher: ActorRef, initialized: Option[Prom
}
val staleChannelsToRemove = new mutable.MutableList[ChannelDesc]
staleChannels.map(d.channels).foreach( ca => {
staleChannels.map(d.channels).foreach(ca => {
staleChannelsToRemove += ChannelDesc(ca.shortChannelId, ca.nodeId1, ca.nodeId2)
staleChannelsToRemove += ChannelDesc(ca.shortChannelId, ca.nodeId2, ca.nodeId1)
})
@ -816,7 +814,7 @@ object Router {
val foundRoutes = Graph.yenKshortestPaths(g, localNodeId, targetNodeId, amountMsat, ignoredEdges, extraEdges, numRoutes).toList match {
case Nil => throw RouteNotFound
case route :: Nil if route.path.isEmpty => throw RouteNotFound
case route :: Nil if route.path.isEmpty => throw RouteNotFound
case foundRoutes => foundRoutes
}
@ -824,7 +822,7 @@ object Router {
val minimumCost = foundRoutes.head.weight
// routes paying at most minimumCost + 10%
val eligibleRoutes = foundRoutes.filter(_.weight <= (minimumCost + minimumCost * DEFAULT_ALLOWED_SPREAD).round)
val eligibleRoutes = foundRoutes.filter(_.weight <= (minimumCost + minimumCost * DEFAULT_ALLOWED_SPREAD).round)
Random.shuffle(eligibleRoutes).head.path.map(graphEdgeToHop)
}
}

View file

@ -168,36 +168,20 @@ class PeerSpec extends TestkitBaseClass {
}
transport.expectNoMsg(1 second) // peer hasn't acknowledged the messages
// now let's assume that the router isn't happy with those channels because the funding tx is not found
for (c <- channels) {
router.send(peer, Peer.NonexistingChannel(c))
}
// peer will temporary ignore announcements coming from bob
for (ann <- channels ++ updates) {
transport.send(peer, ann)
transport.expectMsg(TransportHandler.ReadAck(ann))
}
router.expectNoMsg(1 second)
// other routing messages go through
transport.send(peer, query)
router.expectMsg(Peer.PeerRoutingMessage(transport.ref, remoteNodeId, query))
// now let's assume that the router isn't happy with those channels because the announcement is invalid
router.send(peer, Peer.InvalidAnnouncement(channels(0)))
// peer will return a connection-wide error, including the hex-encoded representation of the bad message
val error1 = transport.expectMsgType[Error]
assert(error1.channelId === CHANNELID_ZERO)
assert(new String(error1.data).startsWith("couldn't verify channel! shortChannelId="))
// after a while the ban is lifted
probe.send(peer, ResumeAnnouncements)
// and announcements are processed again
for (c <- channels) {
transport.send(peer, c)
router.expectMsg(Peer.PeerRoutingMessage(transport.ref, remoteNodeId, c))
}
transport.expectNoMsg(1 second) // peer hasn't acknowledged the messages
// let's assume that one of the sigs were invalid
router.send(peer, Peer.InvalidSignature(channels(0)))
// peer will return a connection-wide error, including the hex-encoded representation of the bad message
val error = transport.expectMsgType[Error]
assert(error.channelId === CHANNELID_ZERO)
assert(new String(error.data).startsWith("bad announcement sig! bin=0100"))
val error2 = transport.expectMsgType[Error]
assert(error2.channelId === CHANNELID_ZERO)
assert(new String(error2.data).startsWith("bad announcement sig! bin=0100"))
}
}

View file

@ -59,13 +59,13 @@ class AnnouncementsBatchValidationSpec extends FunSuite {
val sender = TestProbe()
extendedBitcoinClient.validate(announcements(0)).pipeTo(sender.ref)
sender.expectMsgType[ValidateResult].tx.isDefined
sender.expectMsgType[ValidateResult].fundingTx.isRight
extendedBitcoinClient.validate(announcements(1).copy(shortChannelId = ShortChannelId(Long.MaxValue))).pipeTo(sender.ref) // invalid block height
sender.expectMsgType[ValidateResult].tx.isEmpty
sender.expectMsgType[ValidateResult].fundingTx.isRight
extendedBitcoinClient.validate(announcements(2).copy(shortChannelId = ShortChannelId(500, 1000, 0))).pipeTo(sender.ref) // invalid tx index
sender.expectMsgType[ValidateResult].tx.isEmpty
sender.expectMsgType[ValidateResult].fundingTx.isRight
}

View file

@ -22,7 +22,7 @@ import fr.acinq.bitcoin.Crypto.PrivateKey
import fr.acinq.bitcoin.Script.{pay2wsh, write}
import fr.acinq.bitcoin.{BinaryData, Block, Satoshi, Transaction, TxOut}
import fr.acinq.eclair.TestConstants.Alice
import fr.acinq.eclair.blockchain.{ValidateRequest, ValidateResult, WatchSpentBasic}
import fr.acinq.eclair.blockchain.{UtxoStatus, ValidateRequest, ValidateResult, WatchSpentBasic}
import fr.acinq.eclair.io.Peer.PeerRoutingMessage
import fr.acinq.eclair.router.Announcements._
import fr.acinq.eclair.transactions.Scripts
@ -126,10 +126,10 @@ abstract class BaseRouterSpec extends TestkitBaseClass {
watcher.expectMsg(ValidateRequest(chan_cd))
watcher.expectMsg(ValidateRequest(chan_ef))
// and answers with valid scripts
watcher.send(router, ValidateResult(chan_ab, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_b)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_bc, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_b, funding_c)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_cd, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_c, funding_d)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_ef, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_e, funding_f)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_ab, Right((Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_b)))) :: Nil, lockTime = 0), UtxoStatus.Unspent))))
watcher.send(router, ValidateResult(chan_bc, Right((Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_b, funding_c)))) :: Nil, lockTime = 0), UtxoStatus.Unspent))))
watcher.send(router, ValidateResult(chan_cd, Right((Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_c, funding_d)))) :: Nil, lockTime = 0), UtxoStatus.Unspent))))
watcher.send(router, ValidateResult(chan_ef, Right((Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_e, funding_f)))) :: Nil, lockTime = 0), UtxoStatus.Unspent))))
// watcher receives watch-spent request
watcher.expectMsgType[WatchSpentBasic]
watcher.expectMsgType[WatchSpentBasic]

View file

@ -77,10 +77,10 @@ class RouterSpec extends BaseRouterSpec {
watcher.expectMsg(ValidateRequest(chan_ax))
watcher.expectMsg(ValidateRequest(chan_ay))
watcher.expectMsg(ValidateRequest(chan_az))
watcher.send(router, ValidateResult(chan_ac, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_c)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_ax, None, false, None))
watcher.send(router, ValidateResult(chan_ay, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, randomKey.publicKey)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(chan_az, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, priv_funding_z.publicKey)))) :: Nil, lockTime = 0)), false, None))
watcher.send(router, ValidateResult(chan_ac, Right(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_c)))) :: Nil, lockTime = 0), UtxoStatus.Unspent)))
watcher.send(router, ValidateResult(chan_ax, Left(new RuntimeException(s"funding tx not found"))))
watcher.send(router, ValidateResult(chan_ay, Right(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, randomKey.publicKey)))) :: Nil, lockTime = 0), UtxoStatus.Unspent)))
watcher.send(router, ValidateResult(chan_az, Right(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, priv_funding_z.publicKey)))) :: Nil, lockTime = 0), UtxoStatus.Spent(spendingTxConfirmed = true))))
watcher.expectMsgType[WatchSpentBasic]
watcher.expectNoMsg(1 second)
@ -245,7 +245,7 @@ class RouterSpec extends BaseRouterSpec {
probe.send(router, PeerRoutingMessage(null, remoteNodeId, announcement))
watcher.expectMsgType[ValidateRequest]
probe.send(router, PeerRoutingMessage(null, remoteNodeId, update))
watcher.send(router, ValidateResult(announcement, Some(Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_c)))) :: Nil, lockTime = 0)), true, None))
watcher.send(router, ValidateResult(announcement, Right((Transaction(version = 0, txIn = Nil, txOut = TxOut(Satoshi(1000000), write(pay2wsh(Scripts.multiSig2of2(funding_a, funding_c)))) :: Nil, lockTime = 0), UtxoStatus.Unspent))))
probe.send(router, TickPruneStaleChannels)
val sender = TestProbe()