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

Hotfix version 0.9.0.1 tracking #3928

Closed
cdecker opened this issue Aug 11, 2020 · 9 comments
Closed

Hotfix version 0.9.0.1 tracking #3928

cdecker opened this issue Aug 11, 2020 · 9 comments
Assignees

Comments

@cdecker
Copy link
Member

cdecker commented Aug 11, 2020

Given that a number of bugs snuck their way into the v0.9.0 release (mainly through my own omissions in the pay plugin), we're going to do a small hotfix/point release with the backported fixes for those issues.

Below is the list of issues / PRs we're going to include in the v0.9.0.1 release (reported here because we can't open PRs that merge on a tag):

Please let me know if there's anything else that is absolutely critical to include in the hotfix release. Here's a list of commits included in this hotfix release: v0.9.0...v0.9.0.1

@cdecker cdecker self-assigned this Aug 11, 2020
@cdecker
Copy link
Member Author

cdecker commented Aug 11, 2020

Here's the current range-diff with the cherry-picked commits:

  1:  b646b9675 <  -:  --------- lightningd/chaintopology.h: Remove unused `txnums` field from `struct block`.
 2:  e7d89cd7d <  -:  --------- lightningd/invoice.c: Improve programmatic error reporting for `delinvoice`.
 3:  a456d08ad <  -:  --------- pay: Be less aggressive when estimating channel capacity
 4:  5be07c5fe <  -:  --------- bcli: poll `getnetworkinfo` at startup
 5:  905730341 <  -:  --------- bcli: explicitly check at startup that bitcoind does relay transactions
 6:  5b45334d9 <  -:  --------- pyln: allow to signal may_fail in get_node()
 7:  3aad86ff9 <  -:  --------- pytest: test the blocksonly sanity checkin bcli
 8:  094eac4e9 <  -:  --------- plugins/libplugin-pay.c: Properly handle exclusions for routehints with two hops or more.
 9:  3dafddd71 <  -:  --------- channel: Base the channel forget timeout on the headercount
10:  3df2333d5 <  -:  --------- lightningd/plugin.c: Add specific function to give the directory for built-in plugins.
11:  50600dce9 <  -:  --------- lightningd/lightningd.c: Create API to exit lightningd with an exit code.
12:  a847487bb <  -:  --------- lightningd/plugin.c: Add important plugins, which if they terminate, lightningd also terminates.
13:  48f36904c <  -:  --------- tests/test_plugin.py: Add test for --important-plugin.
14:  4ca2e4981 <  -:  --------- lightningd/plugin.c: Make builtin plugins important.
15:  1aa076845 <  -:  --------- tests/test_plugin.py: Test builtin plugins are important.
16:  1ded3fc52 <  -:  --------- lightningd/plugin.c: Add a `--dev-builtin-plugins-unimportant` for developers who want to mess around with the builtin plugins.
17:  28b839f30 <  -:  --------- amount: add helper to convert u64 sats to amount_sat type
18:  36d43b871 <  -:  --------- amount: add amount_msat and amount_sat initializers.
19:  ffbb409b4 <  -:  --------- amount: use initializers everywhere.
20:  fa829f23d <  -:  --------- amount: add amount_msat_scale, amount_msat_ratio, amount_{msat,sat}_div
21:  2e1072969 <  -:  --------- Add missing test dependencies to requirements.txt
22:  a9b992a94 =  1:  bb90e49be tests/test_pay.py: Add test for conflict between presplit and routehints paymods.
23:  4fde45669 =  2:  d1a5be758 plugins/pay.c: Fix the routehints/presplit conflict.
24:  06372e13d <  -:  --------- pyln.proto.message: don't let Message() init set implicit lengths.
25:  95d3d65c6 <  -:  --------- wally: update to the latest wally version
26:  21ee2c3a9 <  -:  --------- psbt: Remove workaround for now-fixed wally tx flag behaviour
27:  499395636 <  -:  --------- psbt: remove now-unneeded const casts
28:  a51c6550e <  -:  --------- psbt: avoid allocations when adding psbt outputs
29:  68ffecba6 <  -:  --------- psbt: Use the newly exposed wally function to clone PSBTs
30:  1fe53880a <  -:  --------- psbt: use new wally functions to add PSBT inputs/outputs
31:  fef155a9e <  -:  --------- psbt: Use wallys function to check PST finalization status
32:  908a8399e <  -:  --------- psbt: Allocate correctly sized buffer for psbt_to_bytes
33:  a5fc66c38 <  -:  --------- plugins/Makefile: Only link in libplugin-pay for plugins that need it.
34:  6fadd5aea <  -:  --------- Makefile: Remove gen_version.h from ALL_GEN_HEADERS.
35:  64d40414b <  -:  --------- pytest: speed up test_restart_many_payments.
36:  8f455c8b9 <  -:  --------- pytest: new join_nodes to allow you to get all the nodes then join some of them.
37:  02b413a4d <  -:  --------- pytest: make join_nodes / line_graph wait for updates in both dirs.
38:  7aa40b6ba <  -:  --------- pytest: fix assumption in test_onchain_their_unilateral_out
39:  7bb461acd <  -:  --------- pytest: fix flake in test_channel_opened_notification
40:  929fd3e2f <  -:  --------- pytest: make sure all nodes see funds using sync_blockheight
41:  fde353ab0 <  -:  --------- pytest: use get_nodes more widely.
42:  79278b880 <  -:  --------- pytest: optimize join_nodes a little.
43:  51aae9cce <  -:  --------- pytest: make valgrind a per-node option.
44:  01a82d38f <  -:  --------- pytest: add slow_test marker.
45:  723b7223b =  3:  4955266d1 pay: Add timestamp of first part to `listpays`
46:  700897f06 =  4:  26b5baf40 listpays: fixed bolt11 null with keysend and update doc command
47:  1521c29fc !  5:  9a3073a53 listpays mod 1: add destination inside the response when bolt11 is null
    @@ doc/lightning-sendonion.7
      
     -\fBsendonion\fR \fIonion\fR \fIfirst_hop\fR \fIpayment_hash\fR [\fIlabel\fR] [\fIshared_secrets\fR] [\fIpartid\fR] [\fIbolt11\fR] [\fImsatoshi\fR]
     +\fBsendonion\fR \fIonion\fR \fIfirst_hop\fR \fIpayment_hash\fR [\fIlabel\fR] [\fIshared_secrets\fR] [\fIpartid\fR] [\fIbolt11\fR]
    -+[\fIdestination\fR] [\fImsatoshi\fR]
    ++[\fImsatoshi\fR] [\fIdestination\fR]
      
      .SH DESCRIPTION
      
48:  33fae1f38 <  -:  --------- travis: Reorganize tests a bit to move non-integration tests out
49:  03b4662c2 <  -:  --------- travis: Use a stripped bitcoind bundle
50:  c1df8d586 <  -:  --------- utxo: remove unused scriptSig field.
51:  600d0a4a1 <  -:  --------- psbt: make psbt_from_b64 more conventional.
52:  6966cf99e <  -:  --------- bitcoin: add wally_tx_output helper to create standalone output.
53:  5e0b03fba <  -:  --------- common: hoist param_bitcoin_address where plugins can use it.
54:  09f065fc4 <  -:  --------- script: expose script_push_bytes().
55:  a6a8a4059 <  -:  --------- psbt: implement psbt_txid.
56:  601fc805d <  -:  --------- fundpsbt: simplify the logic a little.
57:  0cbc6447b <  -:  --------- doc: refer to correct option name in STYLE.md.
58:  930e29480 <  -:  --------- help: better handing of deprecated commands
59:  151bc4758 <  -:  --------- JSON-RPC: getmanifest passes allow-deprecated-apis flag.
60:  c85b14f06 <  -:  --------- libplugin: initialize deprecated in getmanifest.
61:  4ce8f5632 <  -:  --------- pyln.client.plugin: set deprcated_apis based on getmanifest.
62:  7a7a849fc <  -:  --------- lightningd: allow plugin commands and options to mark themselves deprecated.
63:  fc097b8b4 <  -:  --------- libplugin: allow commands and options to mark themselves deprecated.
64:  624df6433 <  -:  --------- pyln-client: allow commands and options to mark themselves deprecated.
65:  639eaaf2b <  -:  --------- json-rpc: Do not return a null amount_msat to listpays
66:  0dcd974d9 !  6:  edf7174aa pytest: Reproduce #3915
    @@ tests/test_pay.py
     @@ tests/test_pay.py: from pyln.client import RpcError, Millisatoshi
      from pyln.proto.onion import TlvPayload
      from utils import (
    -     DEVELOPER, wait_for, only_one, sync_blockheight, TIMEOUT,
    --    EXPERIMENTAL_FEATURES
    -+    EXPERIMENTAL_FEATURES, env
    +     DEVELOPER, wait_for, only_one, sync_blockheight, SLOW_MACHINE, TIMEOUT,
    +-    VALGRIND, EXPERIMENTAL_FEATURES
    ++    VALGRIND, EXPERIMENTAL_FEATURES, env
      )
      import copy
      import os
67:  894c886bd =  7:  0b2e996c2 pay: Inherit payment label to all children
68:  3d902501e =  8:  19bcb4b8c pay: Group payments by their payment_hash, not by bolt11 string
69:  a8672a3ca =  9:  9b5977f8e tests/test_pay.py: Provide test showing that blockheight disagreement causes us to advance routehints too aggressively.
70:  232451f1e = 10:  fa64750b4 plugins/libplugin-pay.c: Be less aggressive with advancing through routehints.
71:  b09447fdb = 11:  6ae412ee6 pay: Fix final TLV payload if not going through MPP modifiers
72:  ce05d3f6b = 12:  c93a0a5ee plugins/libplugin-pay.c: Make sure blockheight disagreement does not prevent all future progress.
 -:  --------- > 13:  9d4ba426b repro: Allow dashes in the version number
 -:  --------- > 14:  c42ddcd70 release: Add changelog for hotfix release v0.9.0.1

@ZmnSCPxj
Copy link
Contributor

Tangent: to avoid visual confusion between 0.9.0.1 and 0.9.1, I suggest numbering the next release after this hotfix as 0.9.2 instead.

@ZmnSCPxj
Copy link
Contributor

ZmnSCPxj commented Aug 12, 2020

How about #3914 as well? It reduces the chances of degenerate splits (#3926) when paying to unpublished nodes in general, due to being less aggressive with routehints in all cases (08e128e), not just blockheight disagreement; the blockheight-disagreement case is the easiest way to trigger it in a test but it can also trigger by chance when others are paying through the public network near your destination. We probably need to think more about adaptive splitting and maybe optimistically dropping channel hints but that is probably not enough time in a hotfix, whereas #3914 is practically complete at this point.

@cdecker
Copy link
Member Author

cdecker commented Aug 12, 2020

Tangent: to avoid visual confusion between 0.9.0.1 and 0.9.1, I suggest numbering the next release after this hotfix as 0.9.2 instead.

How about calling out the difference by using a different point separator like v0.9.0~1 or v0.9.0-1? I'd like not having to change the numbering scheme, which always ends up confusing people.

@cdecker
Copy link
Member Author

cdecker commented Aug 12, 2020

How about #3914 as well? It reduces the chances of degenerate splits (#3926) when paying to unpublished nodes in general, due to being less aggressive with routehints in all cases (08e128e), not just blockheight disagreement; the blockheight-disagreement case is the easiest way to trigger it in a test but it can also trigger by chance when others are paying through the public network near your destination. We probably need to think more about adaptive splitting and maybe optimistically dropping channel hints but that is probably not enough time in a hotfix, whereas #3914 is practically complete at this point.

I'd prefer not having this one in because a) the waitblockheight modifier is unlikely to ever be hit, b) it changes substantially the order in which we iterate over the routehints and I'd like to get more feedback on it being correct first. This really is a hotfix release, so we should only include fixes for issues that impede correct usage and impacts a sufficiently large number of users.

@ZmnSCPxj
Copy link
Contributor

ZmnSCPxj commented Aug 12, 2020

(a) The core fix is not related to waitblockheight, see 08e128e like I linked (b) it does not change the order of how routehints are traversed at all, I put that in #3913 . I suspect a good number of the "pay never ends due to splitting into tons of sub-payments" will disappear with that PR, because my testing with #3913 show the similars failures that are not waitblockheight-related is substantially improved by #3914.

@cdecker
Copy link
Member Author

cdecker commented Aug 12, 2020

Ok, you convinced me, I'll cherry-pick the commits into the release :-)

@ZmnSCPxj
Copy link
Contributor

How about calling out the difference by using a different point separator like v0.9.0~1 or v0.9.0-1?

That is better IMO.

@cdecker
Copy link
Member Author

cdecker commented Aug 12, 2020

Ok, cherry-picked #3914 onto the hotfix release branch and updated the above range-diff. Now testing and publishing the release if all goes well.

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

No branches or pull requests

2 participants