Sometimes the notifier for node A has not been killed before node A connects. This causes an extra GetPeerInfo message to be sent to the switchboard and the `assert(request3.remoteNodeId == remoteNodeId2)` check to fail when the GetPeerInfo for node A comes first. Just waiting to receive the `AsyncPaymentTimeout` message is not enough.
If node A connects after node B then this race does not occur.
Other implementations and Eclair (#2703) disable channels only when the other peer is offline.
So disabled channels should no be used for routing messages.
This PR removes `ActiveEdge` that was introduced in #2656
Some channels have only a few sats available to send but other nodes don't know it so they try to use them and fail.
When the balance goes below configurable thresholds we now advertize a lower maxHtlcAmount.
This should reduce the number of failed attempts and benefit the network.
We assumed that once quiescent, the local and remote commitment index
would be the same, but that's not true. Those two indices may diverge
because of concurrent updates. We need to keep using the right index for
each commitment during and after a splice, otherwise it leads to "invalid
commit sig" errors.
When we start, bitcoind may not be ready to handle requests if we restarted
it around the same time. We thus poll it for 30 seconds before giving up.
This is especially useful for tests (e.g. on regtest).
The `ChannelFeatures`->`ChannelType` conversion is problematic for the following reasons:
- It creates an implicit bidirectional mapping between the two classes, which is illogical and error-prone because both directions must be kept in sync.
- Not only must the conversions be kept in sync, it must also be stable over time.
- From the spec p.o.v., the scope of `ChannelType` is really limited to channel opening negotiation, while we currently make it a permanent concept.
- It has weird semantic side effects where we would check channel types in places where we really meant to check commitment formats.
If a channel has negotiated quiescence with pending htlcs then `InteractiveTxBuilder` will create/verify commit txs which include htlc outputs and create/verify htlc signatures when exchanging `CommitSig`.
The new attribute `htlcsAmount` of `Output.Shared` and `Input.Shared` accounts for the value in htlcs when computing fees.
* Move some logs to debug level
This should reduce the pressure on the file system and RAM without impacting
our ability to troubleshoot common issues.
* Avoid herd effect watching external channels
When we restart, we put watches on every public channel in the network.
That creates a lot of RPC calls to bitcoind, which aren't time-sensitive.
It's ok if we don't see immediately that an external channel was closed,
the spec even recommends waiting for 12 blocks to distinguish a channel
close from a splice.
By default, we now smooth that over a 1 hour period. This means we should
also allow our peers to be late at discovering that a channel was closed.
We thus stop sending a `warning` in that case and increase our tolerance
to that kind of behavior.
* Avoid herd effect watching local channels
When we restart, we set watches on our funding transactions. But we don't
actually need to watch them immediately, we just need enough time to react
to our peer broadcasting their commitment. We use long `cltv_delta` delays
to guarantee funds safety, so we can spread out the watches across several
blocks to reduce the start-up load. It essentially is the same thing as
receiving mempool transactions or blocks after a delay, which is something
that our threat model already takes into account.
* Allow splicing on non dual-funded channels
We currently allow splicing on top of a channel that was created without
using dual funding, but it results in an incorrect channel reserve being
used post-splice.
* Rename requestedChannelReserve_opt
This field only applies to the first funding index, so we rename it to
make that explicit. Once a splice has happened, it will be ignored.
A minor fix for tests that rely on the minimum feerate being low.
---------
Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
We want to be able to bump a force-close regardless of which commitment
was published. If the remote commitment is in our mempool, we will use
our anchor on that commit tx to bump the fees.
* Always retransmit tx_signatures if requested
If our peer asks us to retransmit our `tx_signatures`, we should do it even
if the transaction is already confirmed. That means we need to store our
`tx_signatures` because they're annoying to recompute.
* Ignore previously received commit_sig
When expecting a retransmission of `tx_signatures`, we should ignore the
`commit_sig` they send just before if we've already received it. The right
way to check that we've already received it is to compare its signature
to our latest commitment transaction.
Some tests related to bitcoind's funding behavior have been randomly
failing since we updated to bitcoind 24.1. They now have more sensibility
over the exact set of available utxos, so we needed to fine-tune them
accordingly.
I also imported some tests that were added to feature branches but never
backported to `master`.
If we're unable to find the spending tx in the mempool, we start looking
for it in the blockchain. Unfortunately, there are cases where we end up
in that code path even though the spending tx is not confirmed:
- a timeout on the `getTransaction` call
- the spending tx gets dropped from our mempool for some reason
- bitcoind is malicious or buggy
Fetching the whole blockchain isn't really useful: after enough time has
passed, we can be pretty sure that a potential attacker would have claimed
the transaction's outputs already and we can't punish them.
We make the distinction between _not_ defining a bitcoin wallet name, and using an bitcoin wallet with an _empty_ name.
This remains mostly backward compatible, as:
- by default `eclair.bitcoind.wallet` is undefined so we keep using the default loaded wallet as before
- if user already set `eclair.bitcoind.wallet`, then eclair will use that wallet.
- only in the case where the user left `eclair.bitcoind.wallet` undefined, and if bitcoin had only one wallet with a non-empty name, then eclair will fail right at startup with an explicit error
---------
Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
* Make Eclair manage bitcoin core's wallet private keys
We create an empty watch-only wallet and import public descriptors generated by Eclair.
Bitcoin Core can fund transaction and manage utxos, but can no longer sign transactions.
* Check that spent amounts and utxos are consistent before we sign a PSBT
PSBT utxo fields include the amount that are being spent by the PSBT inputs, but there is a "fee attack"
where using amounts that are lower than what is actually spent may make us sign a tx that spends much more
in fees than we think.
* Check that non-segwit uxto has been provided and inputs are signed with SIGHASH_ALL
* Verify that Bitcoin Core's fee match what we specified
When we call Bitcoin Core's `fundrawtransaction` RPC method, we check that the fee that we pay match the fee rate that we requested.
The fee is computed using the utxo information that Bitcoin Core adds to our PSBT before we sign it.
We can safely used this information because if Bitcoin Core lies about the value of the inputs that we're spending then the signature we produce will also not be valid (it commits to the value being spent).
When we're adding wallet inputs to "bump" the fees of a parent transaction we need to take the whole package into account when we verify the actual fee rate, which is why some internal methods were modified to return the package weight that was used as reference when `fundrawtransaction` was called.
* Check that fundrawtransaction does not add more than 1 change output
* Validate addresses and keys generated by bitcoin core
When eclair manages private keys, make sure that we can re-compute addresses and keys
generated by bitcoin core.
* Add a separate configuration file for Eclair's onchain signer
Eclair's onchain signer now has its own `eclair-signer.conf` configuration file in HOCON format.
It includes BIP39 mnemonic codes and passphrase, a wallet name and a timestamp.
When an `eclair-signer.conf` file is found, Eclair's API will return descriptors that can be imported into an
empty watch-only Bitcoin Wallet.
When wallet name in `eclair-signer.conf` matches the name of the Bitcoin Core wallet defined in `eclair.conf`
(`eclair.bitcoind.wallet`), Eclair will bypass Bitcoin Core and sign onchain transactions directly.
* Skip validation of local non-change outputs:
Local non-change outputs send to an external address, for splicing out funds for example.
We assume that these inputs are trusted i.e have been created by a trusted API call and our local
onchain key manager will not validate them during the signing process.
* Document why we use a separate, specific file for the onchain key manager
Using a new signer section is eclair.conf would be simpler but "leaks" because it becomes available everywhere
in the code through the actor system's settings instead of being contained to where it is actually needed
and could potentially be exposed through a bug that "exports" the configuration (through logs, ....)
though this is highly unlikely.
* Additional changes to delegate bitcoin core keys to eclair (#2726)
Refactor the `BitcoinCoreClient` and `LocalOnChainKeyManager` to:
- rely less on exceptions
- use more idiomatic scala (reduce dependency on kotlin types)
- provide more detailed logs
We also simplify the `useEclairSigner` field in `BitcoinCoreClient`.
The complexity of handling the case where there was an on-chain key
manager but for a different wallet than the one configured isn't
something that should be used, so it wasn't worth supporting.
Some checks were inconsistent and are now unified:
- checking the exact `scriptPubKey` in our outputs in TODO and TODO
- we allow using `fundTransaction` with a tx that already includes a
change output (which may happen when RBF-ing a transaction)
- `getP2wpkhPubkeyHashForChange` didn't verify the returned key
We completely separate the two cases in `signPsbt`, because otherwise
in the non eclair-backed case, we were calling bitcoind's `processpsbt`
twice for no good reason, which is bad for performance.
We also decouple the `OnChainKeyManager` from the `BitcoinCoreClient`.
This lets users keep running their eclair node with a bitcoin client that
owns the private key while configuring the on-chain key manager for a
future bitcoin client that will leverage this on-chain key manager.
Users can use the eclair APIs to get the master xpub and descriptors to
properly configure their next bitcoin core node, and switch to it once it
has synchronized the descriptors.
* Simplify replaceable tx funding
We were previously signing twice (with makes a call to `bitcoind`),
just to get the final weights and adjust the change outputs. This was
unnecessary, as we can adjust the weights before adding inputs.
We were also duplicating the checks where we verify that `bitcoind` is
malicious. We only need to check that once, during the final signing step.
---------
Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
Node operators may disable automatic fee-bumping on their local commit if
they don't have anything at stake (no pending HTLCs), which saves fees in
most cases. A drawback in that case is that if the commitment doesn't
confirm quickly enough, the remote's funds are also locked.
This can be an issue for LSPs, where the remote peer doesn't have a good
ability to fee-bump commit txs. We give more control to the node operator
by letting them fee-bump local commit txs explicitly through the RPC to
unblock wallet users funds.
When reconnecting in the middle of signing a splice, we must ensure that
splice_locked is sent *after* tx_signatures. Otherwise when using 0-conf
we may retransmit splice_locked before tx_signatures, which our peer will
ignore because they don't have a corresponding fully signed commitment.
We start watching funding outputs once the corresponding funding transaction
has been confirmed with 3 confirmations. We may have already spent that
transaction with a child splice transaction by that time: providing hints
with the corresponding txids to the `ZmqWatcher` improves the performance
when that happens.
This is a follow up to #2565, more precisely to the _caveat_ here: https://github.com/ACINQ/eclair/pull/2565#issuecomment-1397052582.
We now create a fresh shutdown script even if one was already generated at channel creation, if the channel doesn't have the mandatory `option_upfront_shutdown_script` negotiated.
The (reasonable) assumption is that other implementations will ignore our pre-generated script if they didn't support the `option_upfront_shutdown_script` feature.
This "on-the-fly" approach is simpler and safer than a db migration.
This lets us use the new `gettxspendingprevout` instead of fetching the
whole mempool when looking for txs spending one of our channels.
A new feature was added to bitcoind 24.1+ that tries to make the change
output indistinguishable from the payment output. This is a great for
privacy, but it adds randomness to coin selection and uses a non-minimal
set of inputs sometimes. We work around this in tests by updating the
amount of the output we want bitcoind to use to make sure it's sufficient to
pay for both the channel funding and the change output.
This shouldn't be too much of an issue for normal operation, where we'll
sometimes use two inputs instead of one, which costs more fees, but
increases privacy.
See https://github.com/bitcoin/bitcoin/pull/24494 for details.
When our peer fails HTLCs, we only propagate the failure upstream once
we've received their revocation for the previous commitment (because they
could otherwise publish the previous commitment and claim those HTLCs).
If they publish the new commitment without sending us their revocation,
we previously didn't propagate the failure upstream, which leads to an
unnecessary force-close. We now correctly handle this scenario.
This change adds support for the quiescence negotiation protocol via the new `stfu` message. When a channel is quiescent, both sides will have the same set of signed htlc commitments and a splice can be performed without requiring the channel to be idle.
An additional PR is still required to update our splice implementation to properly account for in-flight htlcs. Quiescence should currently only be enabled for compatibility testing.
We send a warning and disconnect when a forbidden messages is received during quiescence; a disconnect ends quiescence. If an htlc is fulfilled/failed while quiescent, any preimage will be relayed immediately and the update will be replayed when quiescence ends.
We also send a warning and disconnect if both quiescence and splice negotiation are not complete before the quiescence timeout.
---------
Co-authored-by: t-bast <bastien@acinq.fr>
* Update code dependencies
I also wanted to update logback, but I'm hitting issues because of our
custom logger in `FixtureSpec` (`LoggingEventAware` not found).
* Update build dependencies
We're now using mvn 3.9.2 to build eclair, which reports warnings in some
of the build plugins we use. Updating plugins fixes most of the warnings,
and the remaining warnings have to be fixed by the plugins themselves
to support mvn 4.x.
After exchanging `tx_complete`, we validate the splice transaction before
sending our `commit_sig`. If we consider the transaction invalid, we send
`tx_abort`. But if our peer thinks the transaction is valid, they will send
their `commit_sig`, which we must ignore until they've acked our `tx_abort`.
Test assumes that payments inserted in the db will be returned in a deterministic order because they have different timestamps.
It may not always be the case if TimestampMilli.now() returns the save value twice.
So here we use explicitly different timestamps: now = TimestampMilli.now(), now + 1.milli, now + 2.milli ....
The previous repository we used removed the binaries for mvn 3.9.2, which
broke our docker build. It looks like `archive.apache.org` keeps artifacts
longer and hopefully won't break our docker build again soon.
When restarting, we weren't checking whether it was using blinded paths.
If we were an intermediate node in the blinded path, we were incorrectly
returning a normal failure: it should be ok, since the introduction node
is supposed to translate those failures, but it's safer to assume that
they don't.
For fighting jamming attempts, or even just to detect one, we need to know how fast relayed HTLCs are fulfilled. We now measure this and store it in the audit database. Previously the "IN" and "OUT" directions for the same HTLC were storing the same timestamp (corresponding to when the HTLC is fulfilled), we now use the timestamp at which we received the UpdateAddHtlc for the "IN" direction.
Move away from the "block target" approach.
Get rid of the `FeeEstimator` abstraction and use an `AtomicReference` to store and update the current feerates, similar to the block count.
When sending a message, the postman can now ask the router to find a route using channels only.
The same route is also used as a reply path when applicable.
The graph data structure has been updated to include both active and disabled edges.
The graph now contains features for vertices.
It turns out that #2688 broke the JSON serialization of the `channels`
API by introducing a new `ActorRef`. We never want to surface `ActorRef`
in JSON responses, so we're defining serializers to filter them out.
Co-authored-by: Fabrice Drouin <sstone@users.noreply.github.com>
