We introduced a way to get a "light" payments view in #1225.
This was a performance improvement for mobile wallets embedding eclair.
Mobile wallets should now use lightning-kmp instead of eclair, so we can
get rid of that unused code.
The flaky test was `ZeroConfAliasIntegrationSpec`.`a->b->c (b-c private)`.
Due to a race between node watchers, we were sometimes entering into
this scenario between bob and carol:
1) carol's watcher wins the race and sends her `channel_ready` to bob
before bob has received `WatchFundingConfirmedTriggered` from his own
watcher
2) the channel isn't officially zeroconf, but bob gladly accepts carol's
early `channel_ready` and moves directly to `WAIT_FOR_CHANNEL_READY` and
then `NORMAL`, *without a real scid*.
3) bob ignores the `WatchFundingConfirmedTriggered` that it receives in
state `WAIT_FOR_CHANNEL_READY` or `NORMAL`
4) bob later receives the `WatchFundingDeeplyBuriedTriggered`, discovers
his own real scid and emits a `ShortChannelIdAssigned` event, but the
channel relayer only listens to `LocalChannelUpdate`.
5) subsequent a->b->c payment using a real scid for b-c as routing hint
fails because b's relayer doesn't know it.
There are two ways of fixing this:
a) make the relayer listen to `ShortChannelIdAssigned` (like it did before)
b) emit a `LocalChannelUpdate` event containing the new real scid and
the same `channel_update`.
In this commit, we chose to implement (b).
Co-authored-by: t-bast <bastuc@hotmail.fr>
We were using hints from `SendPaymentConfig.invoice` to populate `PathFindingExperimentMetrics`, but the invoice is optional and not populated for trampoline relay.
In `SendPaymentToNode`, the routing hints were present twice: once in `invoice` and once in `extraEdges`. I'm only keeping `invoice`.
Also make the `ExtraEdge` trait use `def` as there may be problems with `val`.
When a channel force-close without any pending htlcs, funds are not at
risk. We want to eventually get our main output back, but if we are not
in a rush we can save on fees by never spending the anchors.
This is disabled by default as there is a potential risk: if the commit
tx doesn't confirm and the feerate rises, the commit tx may eventually be
below the network's min-relay-fee and won't confirm (at least until package
relay is available).
We now use either our local alias or the real scid in the channel update
that we store internally. When we send that channel update directly to
our peer, we override it to use the remote alias when it makes sense.
The `override-init-features` field in `eclair.conf` was not previously
applied on top of the `features` field, so node operators were usually
copy-pasting their `features` overrides in every `override-init-features`.
The overrides are now applied on top of the base `features` configuration,
which makes it easier and more intuitive to configure per-node features.
* improve bitcoin client metrics
* only use batching bitcoin client for the watcher
* use named args for defaultFromFeatures method
* use default min_depth in edge case scenario
* add OutgoingChannelParams trait
* update amounts in ChannelRelayerSpec
* make register take typed replyTo
* improve codecs non-reg tests
Bolt11 uses routing hints to add extra edges for the path-finding, blinded routes are also extra edges added to the path-finding but they have a different format. The new type `ExtraEdge` is more flexible and can be expanded to cover new kinds of extra edges such as blinded routes.
If the current chunk of data read from the TCP connection only contains unknown messages (in particular, could be only one isolated unknown message on an otherwise idle connection), we never resumed reading on the connection.
This means all subsequent messages, including pings/pongs, won't be read, which is why the most visible effect is disconnecting due to no response to ping.
Related to https://github.com/ElementsProject/lightning/pull/5347.
Reported by @wtogami.
Since actors are initialized asynchronously, and the initialization sometimes involves subscribing to an `EventStream`, we don't know when they are ready to process messages, especially in tests, which lead to race conditions. By making actors publish `SubscriptionsComplete ` on the same `EventStream` they are subscribing to, we guarantee that if we receive `SubscriptionsComplete ` the actor has been initialized and its subscriptions have been taken into account.
Create new database for each unit test instead of spawning a new db
instance.
Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
We can currently deduce the `channel_id` of a public channel because it
is based on the funding transaction coordinates, but that won't be the
case with dual-funding, where the `channel_id` will be private information
between the two channel peers.
We make it explicit that conversion from a string to an scid uses the tx
coordinates and can fail.
We also clean up compiler warnings in updated test files.
Eclair batches bitcoind request by default for efficiency.
However this can create latency and reliability issues when
running bitcoind on a remote machine over an unreliable
network. In those cases it's better to opt-out of batching.
We also make the threshold of missing blocks for blockchain
watchdog configurable instead of a hard-coded value of 6.
When a channel is offline and we want to relay an HTLC through it, we emit
a new `channel_update` with the disable bit set (unless it was already
disabled).
We previously didn't persist our internal state with this new
`channel_update`, which created the following issue: if eclair is restarted
before the channel comes back online, eclair would only try to rebroadcast
the previous `channel_update` which has a lower timestamp than the one
disabling the channel. That `channel_update` is then rejected by the
network, causing the channel to stay disabled until the next refresh,
which only happens after 10 days.
* use zero-args method to skip fixture
This is the proper way to skip building the fixture. Using `_` actually
creates a fixture and then ignores it.
* consistency nit
* use list/map instead for test cases
* add a custom reporter to highlight slowest tests
When finding a payment route (typically for routing a trampoline payment) we already log the recipient node id, however it can be a private node hidden behind a routing hint in which case it is more useful to log the public node from the routing hint.
This is added to the `audit.path_finding_metrics` and we also add the `payment_hash` so that it can be joined with the `audit.relayed` and `audit.relayed_trampoline` tables.
* Remove close() in db interfaces
It shouldn't be the responsibility of individual db classes to close
the underlying db connection because they typically share the same db
instance (postgres) or db files (sqlite). Closing should be handled in
the `Databases` level (which is already the case for postgres.
For sqlite, closing was only useful for mobile apps, which now use
lightning-kmp.
Also removed `DbFeeProvider`, which was only used by mobile apps.
* increase github ci build time 20min->30min
This PR does two main things:
- introduce a new `FixtureSpec` base class for tests that involve a fixture. See the scaladoc for more info.
- add new simple integration tests in package `integration.basic`. They are based on `MinimalNodeFixture`, which is a full setup for a node with real actors, except the bitcoin part (watcher/wallet) which is mocked. They are much lighter than our previous integration tests, which allow us to keep each test individual, as opposed to having all tests of the same suite depend on each other. We can define more complex fixtures with any number of nodes.
Other minor improvements:
- update scalatest version
- simplify `ChannelStateTestsHelperMethods`
- replace all === by ==
Triple equals allows customizing the equality method, but we never use
that.
Detailed errors (which is what we were looking for) are provided by
mixing in scalatest's `Assertions` and using the regular `assert(x==y)`
syntax. The `Assertions` trait is already mixed in by default in all
scalatest suites.
Even though we use our anchor to CPFP our commit tx, the commit tx could
confirm on its own. If that happens, we don't care about the anchor tx so
we shouldn't RBF it. Ideally we'd like to cancel it but that's impossible,
so we just wait for it to either confirm or drop out of mempools after a
while.
When running behind a load balancer that doesn't preserve the client source
IP address, it doesn't make sense to include the `remote-address` tlv in
our `init` message, it will contain an IP address that is completely
unrelated to our peer.
Fixes#2256
This gives more time to rollback the funding tx in case where we moved
from `WAIT_FOR_FUNDING_INTERNAL`->`CLOSED`.
We could do it differently, with an additional step, or by only sending
the "shutdown" symbol after we receive the wallet response for this
specific transition, but it would be more complicated.
This is the final preparatory step before we can introduce channel aliases, which are arbitrary values of type `ShortChannelId`. We had to move to a stable identifier for private channels. Public channels can keep using the `shortChannelId`, it is stable because it is a "real scid" (and only used after 6 confs) and is useful as an index for channel queries.
This is a preparation for https://github.com/lightning/bolts/pull/910.
The relaying logic is now based on the `channelId` instead of
`shortChannelId`.
We used to consider `shortChannelId` as a unique identifier for the
channel, but it's not true anymore: there will be multiple aliases per
channel.
This is easier to use than having to decide which params we should look
into (local or remote). It will also be easier to integrate with dual funding.
Rename initialFeeratePerKw: this name was very confusing.
This feerate only applies to the commit tx, so we make that explicit.
In this PR we add a basic integration test between `Channel` and `Router` that checks the proper handling of local announcements.
* disable router rebroadcast in tests
* use separate ActorSystem for alice and bob in tests
* add a simple channel-router integration test
* fix bug found in new channel-router test
The new test was failing, due to a bug. When a local
channel graduates from private to public, we do not copy existing
known `channel_update`s. Current implementation guarantees that we
will process our local `channel_update` immediately, but what about our
peer?
* fix rebroadcast for local announcements
We fix a second bug where gossip origin wasn't properly set for local
announcements
* increase ram for tests to 2G
* improve debuggability of integration tests
In tests we can sleep for at least X seconds but because of other tests running in parallel, we can't get an upper bound on the sleeping time. This makes some tests that depend on time unfeasible.
Fixes#2175#2276
From scalatest's `ParallelTestExecution` doc:
> ScalaTest's normal approach for running suites of tests in parallel is to run different suites in parallel, but the tests of any one suite sequentially.
> [...]
> To make it easier for users to write tests that run in parallel, this trait runs each test in its own instance of the class. Running each test in its own instance enables tests to use the same instance vars and mutable objects referenced from instance variables without needing to synchronize. Although ScalaTest provides functional approaches to factoring out common test code that can help avoid such issues, running each test in its own instance is an insurance policy that makes running tests in parallel easier and less error prone.
This means that for each single test of the `channel.states` package, we instantiate one actor system, which contains two thread pools. With default settings, that's a minimum of 2*8 threads per individual test.
That's already pretty bad, and with 65cf238 (#2270) we add a factor of 3 on top of that, which makes us go past the OS limits on github CI.
setup | peak thread count | run time
-------|---------------------|----
baseline | 5447 | 5m 44s
sequential | 1927 | 5m 9s (*)
(*) It's actually so bad, that tests run actually faster without parallelization!
