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

Test init networks TLV #13

Open
wants to merge 110 commits into
base: master
Choose a base branch
from

Conversation

darosior
Copy link

@darosior darosior commented Dec 25, 2019

This is based upon #12.
This is rebased upon the non-merged-yet lightning#682.
This (succesfully) tests ElementsProject/lightning#3371.

Sword-Smith and others added 16 commits November 14, 2019 08:38
When the `p` multiplier is used, the amount MUST be divisible
by 10 since the resolution used internally is millisatoshi.

This addresses but does not close lightning#692.
Tabs or spaces ? Spaces seems to largely beat tabs in this files (and more globally in the repo).
We simply specify, in each case, where they will appear ("Context").

Because `globalfeatures` is already in use, we fold that into the
renamed `localfeatures` field to unify them (now called `features`),
but dissuade further use.

Note also: we REQUIRE minimal `features` field in
channel_announcement, since otherwise both sides of channel will not
agree and not be able to create their signatures!

Consider these theoretical future features:

`opt_dlog_chan`: a new channel type which uses a new discrete log HTLC
type, but can't support traditional HTLC:

* `init`: presents as odd (optional) or even (if traditional channels
  not supported)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: presents as even (compulsory), since users need
  to use the new HTLCs.

`opt_wumbochan`: a node which allows channels > 2^24 satoshis:

* `init`: presents as odd (optional), or maybe even (if you only want
  giant channels)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: not present, since size of channel indicates
  capacity.

`opt_wumbohtlc`: a channel which allows HTLCs > 2^32 millisatoshis:

* `init`: presents as odd (optional), or even (compulsory)
* `node_announcement`: the same as above, so you can seek suitable peers.
* `channel_announcement`: odd (optional) since you can use the channel
  without understanding what this option means.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Co-Authored-By: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
The feature fields refer to the properties of the channel/node, not the
message itself, so we can still propagate them (and should, to avoid
splitting the network).

If we want to make an incompatible announcement message, we'll use a
different type, or insert an even TLV type.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
A bit less dense, but avoids a separate feature space.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I recently made a cut & paste bug with the protocol tests, and
paid an HTLC of amount 100M msat, but with only a 1M msat `amt_to_forward`
in the hop_data.  To my surprise, it was accepted.

This is because we allow overpaying the routing fee (considered 0
for the final hop).  This doesn't make sense for the final hop: anything
but exact equality implies a bug, or that the previous node took the
wrong amount from the payment.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We also define what the basic_mpp feature means in an invoice, by
reference to the next commit.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This also defines the TLV format for payment_secret; the two are intertwined.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means the BOLT11 invoice must offer it (we already say it must
set the field if it offers it), and that the receiving node must
require it (again, we already say it must check it if it requires it).

Without the payment_secret, MPP payments are especially vulnerable to
probing attacks: unlike normal payments (with amounts) they can be
detected with 1msat payment probes.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
*Require* payment_secret for multi-part payments
BOLT 4 explicitly indicates var_onion_optin may appear in a BOLT 11
invoice, however, BOLT 9 only indicates it is available in init and
node_announcement contextx. Resolve this conflict in favor of BOLT 4
as there doesn't seem to be much reason to *not* allow it in BOLT
11 invoices.
@darosior darosior changed the title Test init networks Test init networks TLV Dec 25, 2019
TheBlueMatt and others added 12 commits January 6, 2020 14:34
This appears to have been an oversight in the flat features spec,
and is somewhat implicitly relied on for several new feature bits -
if var_onion_optin is set on a node_announcement (its not allowed
on a channel_announcement), then trying to route through that node
using the pre-tlv formt is somewhat nonsensical, and should be
forbidden.
Resolve two spec oddities regarding new features.
…lightning#722)

As reading of commit 6729755 shows, `final_expiry_too_soon` was
17, not PERM|17.

Note that because we folded a previously non-permanent failure into
the now-permanent PERM|15 failure code, modifications to payment
algorithms may now be needed to specificalyl detect this case,
otherwise payment algorithms may give up in some edge cases where
blocks are mined while payments are in-transit between sender and
receiver.
This commit:
 - Adds a new Dependencies column to the BOLT 9 feature table
   populated with existing feature dependencies.
 - Requires that all valid feature vectors set transitive dependencies.
 - Requires checking transitive dependencies when validating init
   messages and payment request.
 - Removes transitive feature requiremetns from the BOLT 11 writer, now
   that they are implicit by needing to comply with the BOLT 9 origin
   requirements.
As a final step, we now can remove several of the BOLT 11 writer's
requirements now that it builds on BOLT 9's, particularly:
 - setting the even bit if a feature is required.
 - only setting a feature if the node supports a given feature.

The lone requirement that remains pertains to setting the `s` value if
and only if the `payment_secret` feature is set.
rustyrussell and others added 27 commits February 18, 2020 10:53
You can still override on cmdline.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Feature bit is now 13 (0x80000), and we include the
my_current_per_commitment_point with option_static_remotekey; we just
ignore it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They worked for about two weeks after I wrote them!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Will be used later for fundchannel process
Allow for the test to specify the feerate for a fundchannel open
Build out the command for fundchannel, originates open_channel
from node under test
If we exit before fundchannel has finished, it shows errors; let's
swallow the errors if they're happening on/after shutdown
We need to make sure that any fundchannel process is killed by stop,
so that we can run multiple fundchannel attempts in the same go

Also removes 'shutdown(wait=False)' since it doesn't do anything,
really.
Simple 'open channel' check from the opener's perspective.  Uses the
'new' fundchannel impl for c-lightning.
Moving c-lightning over to 'ground' sigs means that it'd be more
flexible if the protocol test framework allowed for dynamically
generating or verifying sigs based on a hash + private key.

Uses ':' notation as a separator so that `htlc_signature` list
parsing will still work without modification
missing openchannel, will follow with cleanup
As changed in commit aab83e729b93d6ce2e2b4702681aaba71462bec8.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…e used.

Add another 16 bits.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
NameError: name 'peers' is not defined

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This has been discussed for forever, but now we have TLVs
the correct encoding seems obvious.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@darosior
Copy link
Author

Rebased now that lightningnetwork#682 was merged

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.