We've been getting a bunch of 'warnings' about an unknown mark. This
silences the warning, by registering the mark as expected.
=========================================================== warnings summary ===========================================================
tests/test_closing.py:152
/home/niftynei/dev/lightning/tests/test_closing.py:152: PytestUnknownMarkWarning: Unknown pytest.mark.slow_test - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/latest/mark.html
@pytest.mark.slow_test
tests/test_closing.py:214
/home/niftynei/dev/lightning/tests/test_closing.py:214: PytestUnknownMarkWarning: Unknown pytest.mark.slow_test - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/latest/mark.html
@pytest.mark.slow_test
tests/test_closing.py:704
We used to create the entire reply, the if it was too big, split in
half and retry.
Now that the main network is larger, this always happens with a full
request, which is inefficient.
Instead, produce a reply assuming no compression, then compress as a
bonus. This is simpler and more efficient, at cost of sending more
packets.
I also renamed an internal dev var to make it clearer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We previously registered hooks up in who-replies-to-getmanifest-first
order, but then if any had dependencies it would scatter that order.
This allows users to manually set dependencies developers have
forgotten by specifying the plugins manually in their configuration or
cmdline. This was an excellent consideration by @mschmook.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will allow nodes (with log-level=debug) to gather how many payments
are made without payment_secrets. We need to know this so we know when
we can make them compulsory.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We let the plugin decide what feerate to accept/whether or not to add
funds to the open. To aid this decision, we also send the plugin what we
(c-lightning) currently have as our max and min acceptable feerates.
We also now use these as our default for max/min acceptable feerate
range when sending an openchannel offer to a peer.
In the future, it might be a good idea to make these more easily
changeable, either via a config setting (?) or a command param.
For compatibility, we only do this if `allow-deprecated-apis` is false
for now. Otherwise scripts parsing should use `grep -v '^# '` or
start using `-N none`.
Changelog-Added: JSON-RPC: `close` now sends notifications for slow closes (if `allow-deprecated-apis`=false)
Changelog-Deprecated: cli: scripts should filter out '^# ' or use `-N none`, as commands will start returning notifications soon
Fixes: #3925
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This adds a new configuration, --enable-fuzzing (which is more than
welcome to be coupled with --enable-address-sanitizer), to pass the
fuzzer sanitizer argument when compiling objects. This allows libfuzzer
to actually be able "to fuzz" by detecting coverage and be smart when
mutating inputs.
As libfuzzer brings its own ~~fees~~ main(), we compile objects with
fsanitize=fuzzer-no-link, and special-case the linkage of the fuzz
targets.
A "lib" is added to abstract out the interface to the fuzzing tool used.
This allow us to use the same targets to fuzz using AFL, hongfuzz or w/e
by adding their entrypoints into libfuzz. (h/t to practicalswift who
introduced this for bitcoin-core, which i mimiced)
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Instead of a boutique message, use a "real" channel_announcement for
private channels (with fake sigs and pubkeys). This makes it far
easier for gossmap to handle local channels.
Backwards compatible update, since we update old stores.
We also fix devtools/dump-gossipstore to know about the tombstone markers.
Since we increment our channel_announce count for local channels now,
the stats in the tests changed too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Since `fundchannel` now supports the 'close_to' argument, we can remove
all the logic needed to call fundchannel_start here.
Underneath, we're still calling `fundchannel_start`, we're just one (or
two, if you count multifundchannel) call levels away from it now.
We had one report of this, and then Eugene and Roasbeef of Lightning
Labs confirmed it; they saw misordered HTLCs on reconnection too.
Since we didn't enforce this when we receive HTLCs, we never noticed :(
Fixes: #3920
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: Protocol: fixed retransmission order of multiple new HTLCs (causing channel close with LND)
We didn't care, but other implementations (particularly lnd) do. And it
does violate the spec.
(We need to use skip not xfail on the test which catches this, since
xfail doesn't seem to stop errors reported by cleanup)
(Includes Christian's typo fix!)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're rarely in a hurry here, and bitcoind is aggressive with fees.
You can always spend this output if you really have to, using CPFP.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: mutual closing feerate reduced to "slow" to avoid overpaying.
lightning-5 can sometimes see itself sweeping the unilateral output resulting
in this weird line:
```log
HTLC already resolved by SELF when we found preimage
```
In both cases the flakyness arises from the destination not knowing about the
modified fees of the forwarding node, thus including the outdated details in
the routehint, and the sender being unlucky and always trying with the
routehint anyway.
The long-term solutions to this is going to be #4111, this commit just reduces
the flakyness to get back to business.
We really are just interested in their on-chain footprint, so actually
starting the nodes is pointless overhead, and caused a lot of flakyness due to
the output ordering sometimes not matching up. We now just use the `bitcoind`
API to fund, sign and send a raw transaction that matches the stashed gossip
messages.
We added a conversion of failcodes that do not have sufficient information in
faac4b28ad. That means that a failcode that'd require additional information
in order to be a correct error to return in an onion is mapped to a generic
one since we can't backfill the information.
This tests that the mapping is performed correctly and replicates the
situation in #4070
Changelog-Added: JSON-RPC: delpay a new method to delete the payment completed or failed.
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Fixes: #3926
(probably)
Changelog-Fixed: pay: Also limit the number of splits if the payee seems to have a low number of channels that can enter it, given the max-concurrent-htlcs limit.
Hooks do not tolerate failures at all. If we return a JSON-RPC error to a hook
call the only thing the main daemon can really do is to crash. This commit
adds a mapping of error to a safe fallback result, including a warning to the
node operator that this should be addressed in the plugin. The warning is
reported as a `**BROKEN**` message, and should therefore fail any testing done
on the plugin.
Changelog-Fixed: pyln: Fixed HTLCs hanging indefinitely if the hook function raises an exception. A safe fallback result is now returned instead.
This test is flaky because the generated PSBT seems to not have the change
output adjusted, or it is missing.
Tracking-Issue: ElementsProject/lightning#3998
v2 channel open uses a different method to derive the channel_id, so now
we save it to the database so that we dont have to remember how to
derive it for each.
includes a migration for existing channels
fundpsbt / utxopsbt create a (typically) output-less PSBT,
however for elements we require the fees to be encapsulated in an
output.
this patch updates fundpsbt / utxopsbt to add a fee output for elements
transactions. includes test updates.
Fixes#3998
Too trivial a fix to really list in Changelog, but I noticed that we
specified "wumbo" twice. We should really just use the proper name
in listconfigs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: plugin: `bcli` replacements should note that `sendrawtransaction` now has a second required Boolean argument, `allowhighfees`, which if `true`, means ignore any fee limits and just broadcast the transaction. Use `--deprecated-apis` to use older `bcli` replacement plugins that only support a single argument.
I screwed up the rotation logic in an earlier varient of this PR, and
it lead me to discover why test_mpp_interference_2 was flaky.
Really, we should keep a fuzzy estimator of how much payment is
outstanding, but in practice rotation is probably good enough.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This removes the reservation cleanup at startup, too, now they're all
using 'reserved_til'.
This changes test_withdraw, since it asserted that outputs were marked
spent as soon as we broadcast a transaction: now they're reserved until
it's mined. Similarly, test_addfunds_from_block assumed we'd see funds
as soon as we broadcast the tx.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: `withdraw` now randomizes input and output order, not BIP69.
These tests appear in both test_misc.py and test_wallet.py.
I checked, and they're the only duplcates; they were moved in
0543149b89 then re-added (merge error?)
in 67fc8ffbcf.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
First, simplify: amount is set to 1000000, but then we deposit 1000000 + 0.01btc
(i.e. 2000000), and we always use 2 * amount. Just use a single constant to
make it clear.
Secondly, we assume that the wallet considers outputs spent as soon as
we created the tx: this will not be true once withdraw uses sendpsbt.
So, we generate blocks, but now sometimes withdraw will pick up change
txs, so we need to reserve them to avoid that messing our coinmovements.
Finally, we assumed the withdrawl order was BIP69, which becomes
variable.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Marking spent means if the transaction doesn't confirm for some
reason, the user will need to force a rescan to find the funds. Now
we have timed reservations, reserving for (an additional) 12 hours
should be sufficient.
We also take this opportunity (now we have our own callback path)
to record the tx in the wallet only on success.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Some minor phrasing differences cause test changes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: txprepare reservations stay across restarts: use fundpsbt/reservepsbt/unreservepsbt
Changelog-Removed: txprepare `destination` `satoshi` argument form removed (deprecated v0.7.3)
With a feerate of 7500perkw and subtracting 660 sats for anchors, a
20,000 sat channel has capacity about 9800 sat, below our default:
You gave bad parameters: channel capacity with funding 20000sat, reserves 546sat/546sat, max_htlc_value_in_flight_msat is 18446744073709551615msat, channel capacity is 9818sat, which is below 10000000msat
So bump channel amounts.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And document exactly what it does: insist that an HTLC can pass of
this value (module assumptions of feerate).
Note that we remove the "is_opener" test from the capacity calculation
for anchor fees: it doesn't matter which side it is, someone has to pay
for anchor fees to it deducts from capacity.
This change breaks the test, which we rewrite.
Changelog-Changed: config: `min-capacity-sat` is now stricter about checking usable capacity of channels.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We create ALL_PROGRAMS, ALL_TEST_PROGRAMS, ALL_C_SOURCES and
ALL_C_HEADERS. Then the toplevel Makefile knows which are
autogenerated (by wildcard), so it can have all the rules to clean
them or check the source as necessary.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's actually not possible to currently tell if you're using anchor_outputs
with a peer (since it depends on whether you both supported it at *channel open*).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-added: JSON-RPC: `listpeers` shows `features` list for each channel.
We're failing this too often: we'd fail it more but it's disabled
with VALGRIND (it shouldn't be: @slow_test removes VALGRIND if SLOW_MACHINE
is set).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the simplest possible fix: increase the target amount until we get
the desired number of parts, while still bucketizing payments together that
are in approximately the same size.
The current logic puts all payments that are in the range x < amount <= 16*x
in the same bucket, making them harder to distinguish.
Changelog-Fixed: pay: The `presplit` modifier now supports large payments without exhausting the available HTLCs.
You can't pay them anyway, and at least one person used 0 instead of "any".
Closes: #3808
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: `invoice` no longer accepts zero amounts (did you mean "any"?)
This is an extra safety check for dual funding, where we only want to sign
the inputs we provided!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `signpsbt` takes an optional `signonly` array to limit what inputs to sign.
Required for dual funding where the opener sets it.
Changelog-Added: JSON-RPC: `fundpsbt` takes a new `locktime` parameter
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Anchor outputs break many assumptions in our tests:
1. Remove some hardcoded numbers in favor of a fee calc, so we only have to
change in one place.
FIXME: This should also be done for elements!
2. Do binary search to get feerate for a given closing fee.
3. Don't assume output #0: anchor outputs perturb them.
4. Don't assume we can make 1ksat channels (anchors cost 660 sats!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's currently always 0, but it won't be once we replace txprepare.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `fundchannel` has new `outnum` field indicating which output of the transaction funds the channel.
Reported-by: ZmnSCPxj
Signed-off-by: Christian Decker <@cdecker>
Changelog-Fixed: pay: Correct a case where we put the sub-payment value instead of the *total* value in the `total_msat` field of a multi-part payment.
The worst effect is that unpublished nodes are harder to pay, but
even published ones make us do unnecessary work, since we are
losing routehints from the published ones that could help us
actually route better to them.
On my test machine, we queried bitcoind before the close tx was sent:
```
# When output is spent, it should give us null !
txo = l1.rpc.call("getutxout", {"txid": txid, "vout": 0})
> assert txo["amount"] is txo["script"] is None
E AssertionError: assert '20000000msat' is '00205b8cd3b914cf67cdd8fa6273c930353dd36476734fbd962102c2df53b90880cd'
tests/test_plugin.py:1221: AssertionError
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It might have already happened, and anyway, we wait for the actual state
below.
```
# make database snapshot of l2
l2.stop()
l2_db_path = os.path.join(l2.daemon.lightning_dir, chainparams['name'], 'lightningd.sqlite3')
l2_db_path_bak = os.path.join(l2.daemon.lightning_dir, chainparams['name'], 'lightningd.sqlite3.bak')
copyfile(l2_db_path, l2_db_path_bak)
l2.start()
sync_blockheight(bitcoind, [l2])
# push some money from l3->l2, so that the commit counter advances
l2.rpc.connect(l3.info['id'], 'localhost', l3.port)
> l2.daemon.wait_for_log('now ACTIVE')
tests/test_closing.py:908:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
contrib/pyln-testing/pyln/testing/utils.py:288: in wait_for_log
return self.wait_for_logs([regex], timeout)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pyln.testing.utils.LightningD object at 0x7f0c145a32d0>
regexs = ['now ACTIVE'], timeout = 60
def wait_for_logs(self, regexs, timeout=TIMEOUT):
"""Look for `regexs` in the logs.
We tail the stdout of the process and look for each regex in `regexs`,
starting from last of the previous waited-for log entries (if any). We
fail if the timeout is exceeded or if the underlying process
exits before all the `regexs` were found.
If timeout is None, no time-out is applied.
"""
logging.debug("Waiting for {} in the logs".format(regexs))
exs = [re.compile(r) for r in regexs]
start_time = time.time()
pos = self.logsearch_start
while True:
if timeout is not None and time.time() > start_time + timeout:
print("Time-out: can't find {} in logs".format(exs))
for r in exs:
if self.is_in_log(r):
print("({} was previously in logs!)".format(r))
> raise TimeoutError('Unable to find "{}" in logs.'.format(exs))
E TimeoutError: Unable to find "[re.compile('now ACTIVE')]" in logs.
contrib/pyln-testing/pyln/testing/utils.py:264: TimeoutError
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The modded_psbt is not necessarily invalid:
# Try a modified (invalid) PSBT string
modded_psbt = psbt[:-3] + 'A' + psbt[-3:]
with pytest.raises(RpcError, match=r"should be a PSBT, not"):
> l1.rpc.signpsbt(modded_psbt)
E AssertionError: Pattern 'should be a PSBT, not' not found in 'RPC call failed: method: signpsbt, payload: {'psbt': 'cHNidP8BAM0CAAAABJ9446mTRp/ml8OxSLC1hEvrcxG1L02AG7YZ4syHon2sAQAAAAD9////JFJH/NjKwjwrP9myuU68G7t8Q4VIChH0KUkZ5hSAyqcAAAAAAP3///8Uhrj0XDRhGRno8V7qEe4hHvZcmEjt3LQSIXWc+QU2tAEAAAAA/f///wstLikuBrgZJI83VPaY8aM7aPe5U6TMb06+jvGYzQLEAQAAAAD9////AcDGLQAAAAAAFgAUyQltQ/QI6lJgICYsza18hRa5KoEAAAAAAAEBH0BCDwAAAAAAFgAUqc1Qh7Q5kY1noDksmj7cJmHaIbQAAQEfQEIPAAAAAAAWABS3bdYeQbXvBSryHNoyYIiMBwu5rwABASBAQg8AAAAAABepFD1r0NuqAA+R7zDiXrlP7J+/PcNZhwEEFgAUKvGgVL/ThjWE/P1oORVXh/ObucYAAQEgQEIPAAAAAAAXqRRsrE5ugA1VJnAith5+msRMUTwl8ocBBBYAFMrfGCiLi0ZnOCY83ERKJ1sLYMY8AAAA='}, error: {'code': -1, 'message': 'No wallet inputs to sign'}'
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
listpays: make doc-all missed
Changelog-Added: JSON-RPC: `listpays` can be used to query payments using the `payment_hash`
Changelog-Added: JSON-RPC: `listpays` now includes the `payment_hash`
And when it's set, and we're SLOW_MACHINE, simply disable valgrind.
Since Travis (SLOW_MACHINE=1) only does VALGRIND=1 DEVELOPER=1 tests,
and VALGRIND=0 DEVELOPER=0 tests, it was missing tests which needed
DEVELOPER and !VALGRIND.
Instead, this demotes them to non-valgrind tests for SLOW_MACHINEs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I started replacing all get_node() calls, but got bored, so then just did the
tests which call get_node() 3 times or more.
Ends up not making a measurable speed difference, but it does make some
things neater and more standard.
Times with SLOW_MACHINE=1 (given that's how Travis tests):
Time before (non-valgrind):
393 sec (had 3 failures?)
Time after (non-valgrind):
410 sec
Time before (valgrind):
890 seconds (had 2 failures)
Time after (valgrind):
892 sec
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I thought this was timing out because I made it slow with the
change to txprepare as a plugin. In fact, it was timing out
because sometimes gossip comes so fast it gets suppressed
and we never get the log messags.
Still, before this it took 98 seconds under valgrind and
24 under non-valgrind, so it's an improvement to time as
well as robustness.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: pay: Fixed a bug where routehints would be ignored if the payment exceeded 10,000 satoshi. This is particularly bad if the payee is only reachable via routehints in an invoice.
Changelog-Added: New option `--important-plugin` loads a plugin is so important that if it dies, `lightningd` will exit rather than continue. You can still `--disable-plugin` it, however, which trumps `--important-plugin` and it will not be started at all.
niftynei
Antoine Poinsot
Rusty Russell
Michael Schmoock
ZmnSCPxj jxPCSnmZ
Christian Decker
Jan Sarenik
Vincenzo Palazzo
Vincent