Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update android branch #1162

Merged
merged 62 commits into from
Oct 8, 2019
Merged

Update android branch #1162

merged 62 commits into from
Oct 8, 2019

Conversation

sstone
Copy link
Member

@sstone sstone commented Oct 3, 2019

No description provided.

araspitzu and others added 30 commits July 29, 2019 10:32
* Add missing API endpoints to eclair-cli help
* Fix README style warnings
* Add documentation links to the wiki
* Update build instructions.
* Type all amounts used in eclair

* Add eclair.MilliSatoshi class

* Use bitcoin-lib 0.14

* Add specialized codecs for Satoshi/MilliSatoshi

* Rename 'toSatoshi' to 'truncateToSatoshi' to highlight it's a precision-losing conversion
* Route computation: fix fee check

Fee check during route computation is:
- fee is below maximum value
- OR fee is below amout * maximum percentage

The second check was buggy and route computation would failed when fees we above maximum value but below maximum percentage of amount being paid.
Follow up to #1082.

The goal is to be able to publish transactions only after we have
persisted the state. Otherwise we may run into corner cases like [1]
where a refund tx has been published, but we haven't kept track of it
and generate a different one (with different fees) the next time.

As a side effect, we can now remove the special case that we were
doing when publishing the funding tx, and remove the `store` function.

NB: the new `calling` transition method isn't restricted to publishing
transactions but that is the only use case for now.

[1] ACINQ/eclair-mobile#206
Untyped cltv expiry was confusing: delta and absolute expiries really need to be handled differently.
Even variable names were sometimes misleading.
Now the compiler will help us catch errors early.
This is the implementation of lightning/bolts#557.

* Correctly handle multiple channel_range_replies

The scheme we use to keep tracks of channel queries with each peer would forget about
missing data when several channel_range_replies are sent back for a single channel_range_queries.

* RoutingSync: remove peer entry properly

* Remove peer entry on our sync map only when we've received
a `reply_short_channel_ids_end` message.
* Make routing sync test more explicit

* Routing Sync: rename Sync.count to Sync.totalMissingCount

* Do not send channel queries if we don't want to sync

* Router: clean our sync state when we (re)connect to a peer

We must clean up leftovers for the previous session and start the sync process again.

* Router: reset sync state on reconnection

When we're reconnected to a peer we will start a new sync process and should reset our sync
state with that peer.

* Extended Queries: use TLV format for optional data

Optional query extensions now use TLV instead of a custom format.
Flags are encoded as varint instead of bytes as originally proposed. With the current proposal they will all fit on a single byte, but will be
much easier to extends this way.

* Optional TLVs are represented as a list, not an optional list

TLVs that extend regular LN messages can be represented as a TlvStream and not an Option[TlvStream] since we don't need
to explicitely terminate the stream (either by preprending its length or using a specific terminator) as we do in Onion TLVs.

No TLVs simply means that the TLV stream is empty.

* TLV Stream: Implement a generic "get" method for TLV fields

If a have a TLV stream of type MyTLV which is a subtype of TLV, and MyTLV1 and MYTLV2 are both
subtypes of MyTLV then we can use stream.get[MyTLV1] to get the TLV record of type MYTLV1 (if any)
in our TLV stream.

* Use extended range queries on regtest and testnet

We will use them on mainnet as soon as lightning/bolts#557 has been merged.

* Channel range queries: send back node announcements if requested (#1108)

This PR adds support for sending back node announcements when replying to channel range queries:
- when explicitly requested (bit is set in the optional query flag)
- when query flags are not used and a channel announcement is sent (as per the BOLTs)

A new configuration option `request-node-announcements` has been added in the `router` section. If set to true, we
will request node announcements when we receive a channel id (through channel range queries) that we don't know of.
This is a setting that we will probably turn off on mobile devices.

* Extended Channel Queries: add CL interop test
Instead of using two separate maps (for channels and channel_updates), we now use a single map, which groups channel+channel_updates. This is also true for data storage, resulting in the removal of the channel_updates table.
Add comparisons and postfix operators.
Update most of the codebase to leverage those.
* Use unsigned comparison for HtlcValueTooHighInFlight
We will only sync with whilelisted peer. If the whitelist is empty then
we sync with everyone.
* Move Service and FormParamExtractor to eclair-node

* Move dependency akka-http-json4s into eclair-node

* Move json serializers to eclair-node
We must consider `nextRemoteCommit` when applicable.

This is a regression caused in #784. The core bug only exists when we
have a pending unacked `commit_sig`, but since we only send the
`AvailableBalanceChanged` event when sending a signature (not when
receiving a revocation), actors relying on this event to know the
current available balance (e.g. the `Relayer`) will have a wrong
value in-between two outgoing sigs.
It allowed probing attacks and the spec deprecated it in favor of IncorrectOrUnknownPaymentDetails.
Also add better support for unknown failure messages.
A new file was added that used the removed failure code.
Since this was a new file, Github didn't
detect a merge conflicts.
* Fix maven mirror (and move to a US mirror)
* Use maven 3.6.2
* Use Long to back the UInt64 type

* Define comparison operators between UInt64 and MilliSatoshi
lightning/bolts#656 introduced invoice feature bits as a pre-requisite for AMP and other advanced payment use-cases.
* Update docker base image to jdk11, update maven to 3.6.2 [ci skip]
This allows us to choose smaller parameters for tests and reduce cpu
requirement during testing.

NB: The default value of 3500 for `reply_channel_range` was wrong. Theoretical max is ~2700.
This is now enabled by default.
We forward variable-length onions if we receive some.
We accept variable-length payments.
However for maximum compatibility with the network, we send payments using legacy payloads.
* Add comments and fix warnings in graph processing
* Add small feature to set the htlcMaximumMsat for routing hints (otherwise the graph processing algorithm used a minimum value which slightly reduced the benefits of those routing hints)
* Add the computation of network statistics to the router: this will be useful for multi-part payments to decide what thresholds should be used to split a payment
For now:
- we only track some tasks (especially in the router, but not even
`node_announcement` and `channel_update`
- all db calls are monitored
- kamon is disabled by default
Instead of satoshi, which could introduce rounding errors.

Also, we check first the balance before the max-inflight amount, because
it makes more sense in terms of error management.

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
Instead of hardcoding the channel version when we instantiate the
`Commitments` object, we rather define it when the channel is
instantiated. This is saner and prepares future usage.
This is a prerequisite to parallelization of tests.
There are two level of parallelization:
- between test suites (a suite = a test file)
- within a suite (depends on tests suites, some rely on sequential execution of tests, some don't)
* Add codecov integration to semaphore CI

* Remove codecov integration from travis CI
sstone and others added 16 commits September 23, 2019 11:44
* Derive channel keys from funding pubkey

We now generate a random funding key for each new channel, and use its public key to deterministically derive all channel keys and secrets. This will let us easily recover funds using DLP even if we've lost everything but our seed: we just need to connect to the node we had a channel with, ask them to publish their commit tx, and once we see it on the blockchain we can extract our funding pubkey, recompute channel keys and spend our output.

* Add rationale for new channel derivation scheme

* Add a "funding pubkey path" option to the channel version field

This option is checked when we need to compute channel keys. For old channels it won't be set, and we always set it for new ones.

* ChannelVersion: make sure that all bits are set to 0 for legacy channels

* ChannelVersion: USE_PUBKEY_KEYPATH is set by default

* Move recovery test out of OfflineStateSpec
…HTLC (#1084)

If the sender of an htlc isn't the funder, then both sides will have to afford the payment:
- the sender needs to be able to afford the htlc amount
- the funder needs to be able to afford the greater commit tx fee incurred by the additional htlc output.

Fixes #1081.

Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>
* Fix funding pubkey to channel key path computation

Channel key path is generated from 8 bytes computed from our funding pubkey, but we extracted 4 uint32 values instead of 2 (last 2 were always 0). We now use 128 bits to derive channel key paths.

* Add a channel key path compatibility test

This test will fail if we change the way we compute channel key paths, which would break existing channels.
To save memory, once we check that a channel_update's chain hash matches what
we expect we just replace it with a reference to our own chain hash.
Our balance computation was slightly incorrect. If you want to know how much you can send (or receive), you need to take into account the fact that you'll add a new HTLC which adds weight to the commit tx (and thus adds fees).
This is a copy of the spray-based API developped by @araspitzu (akka-http does not
work for akka 2.3 which we use on the android branch)
Cleans up the JSON payment status (easier to interpret for callers).
Kamon does not work on Android and does not make much sense, so we replace
it with a basic Mock implementation that does nothing.
* Electrum wallet: improve coin selection (fixes #1146)

Our previous coin selection would sometimes fail when there was one wallet utxo and and low 
 feerate, because our first pass used a fee estimate that was too high and could sometimes not be met.
Our random funding key path is now 8 * 32 bits plus a 1' (funder) or 0' (fundee).
Channel key paths are computed from the sha256 of the funding public key (we take all 256 bits).
* Use bitcoin 0.18.1 during test
We had 2 open PRs, one that added new tests using the 0.API, one that switched to 0.18.1, when they were merged the new tests failed since they had not been upgraded....
Also:
* explicitely set endpoint identification algorithm in strict mode
* force TLS protocols 1.2/1.3 in strict mode

Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
@codecov-io
Copy link

codecov-io commented Oct 3, 2019

Codecov Report

Merging #1162 into android will decrease coverage by 2.51%.
The diff coverage is 78.4%.

Impacted file tree graph

@@             Coverage Diff             @@
##           android    #1162      +/-   ##
===========================================
- Coverage    73.24%   70.73%   -2.52%     
===========================================
  Files           99      114      +15     
  Lines         7546     8128     +582     
  Branches       302      316      +14     
===========================================
+ Hits          5527     5749     +222     
- Misses        2019     2379     +360
Impacted Files Coverage Δ
...lockchain/bitcoind/rpc/ExtendedBitcoinClient.scala 23.07% <ø> (ø) ⬆️
...n/scala/fr/acinq/eclair/transactions/Scripts.scala 55.88% <ø> (ø) ⬆️
...n/scala/fr/acinq/eclair/router/Announcements.scala 96.87% <ø> (-0.1%) ⬇️
...in/scala/fr/acinq/eclair/api/JsonSerializers.scala 96.87% <ø> (ø)
...in/scala/fr/acinq/eclair/api/ExtraDirectives.scala 0% <ø> (ø)
...cala/fr/acinq/eclair/api/FormParamExtractors.scala 0% <ø> (ø)
...e/src/main/scala/fr/acinq/eclair/api/Service.scala 0% <ø> (ø)
...air-node/src/main/scala/fr/acinq/eclair/Boot.scala 0% <ø> (ø)
...src/main/scala/fr/acinq/eclair/SyncLiteSetup.scala 0% <0%> (ø) ⬆️
...ore/src/main/scala/fr/acinq/eclair/PimpKamon.scala 0% <0%> (ø)
... and 94 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fe2a87...fb4b648. Read the comment docs.

araspitzu and others added 2 commits October 4, 2019 12:23
We already wipe the updates table, and this make upgrading much simpler since we had different structures on
android vs mater.
@sstone
Copy link
Member Author

sstone commented Oct 7, 2019

There are lots of changes that impact many files, but areas that require particular attention are:

  • network db: new structure is different from the old one, and instead of upgrading we just drop tables which means that users will need to re-sync their network db after they've upgraded. That's what we did before with channel updates anyway
  • the impact of the extended optional queries: we need the chain hash field to compute channel update checksums, which we used to strip. It is still stripped when we store data but we now provide it when we load data from back the db, and it does not feel consistent and stripping/restoring stripped fields should probably be done in the router
  • payment lifecycle which has changed a lot

sstone and others added 5 commits October 7, 2019 11:43
* Activate extended channel range queries

By default we now set the `gossip_queries_ex` feature bit.
We also change how we compare feature bits, and will use channel queries (or extended queries) only if the corresponding feature bit is set in both local and remote init messages.
CRC32C is not available in JDK 7 which we target on Android.
@sstone sstone marked this pull request as ready for review October 7, 2019 17:19
@sstone sstone merged commit ce720ba into android Oct 8, 2019
@sstone sstone deleted the android-update branch October 8, 2019 11:21
pm47 added a commit that referenced this pull request Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants