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

Peculiarities w/re paying down zeroconf channels w/ pay and getroute+sendpay #5803

Closed
dongcarl opened this issue Dec 9, 2022 · 1 comment · Fixed by #5846
Closed

Peculiarities w/re paying down zeroconf channels w/ pay and getroute+sendpay #5803

dongcarl opened this issue Dec 9, 2022 · 1 comment · Fixed by #5846
Assignees
Milestone

Comments

@dongcarl
Copy link
Contributor

dongcarl commented Dec 9, 2022

(This is for v22.11.1) Payments via zeroconf channels are broken in quite a peculiar way, in order to track down the issue I've made a parameterized pytest reproducer that is parameterized by:

  1. open_existing_normal_channel (zeroconf_only or add_existing_normal): Whether or not we open a normal (non-zeroconf and confirmed) channel between the payer and the receiver before trying to open zeroconf channels.
  2. confirm_zeroconf_channel (no_confirm_zeroconf or yes_confirm_zeroconf): Whether or not to confirm the zeroconf channel by generating a block (this is obviously not useful for zeroconf payments in general, but its failure indicates something odd about pay)
  3. payment_method (pay or getroute_sendpay): Whether to use pay to make the zeroconf payment or getroute+sendpay

Here are the test results from my machine:

no_confirm_zeroconf zeroconf_only no_confirm_zeroconf add_existing_normal yes_confirm_zeroconf zeroconf_only yes_confirm_zeroconf add_existing_normal
pay PASSED FAILED PASSED FAILED
getroute_sendpay FAILED FAILED PASSED PASSED

These results would seem to indicate that:

  1. pay is unable to make any payments down a zeroconf channel if there exists an existing normal channel, even if we mine a block to confirm the zeroconf channel and give it a real (non-aliased) SCID.
  2. getroute+sendpay Is only able to make a payment down a zeroconf channel if there's a confirmation for the zeroconf channel. I speculate that this is because sendpay does NOT know how to handle SCID aliases but DOES know how to handle real (non-aliased) SCID (getroute rightly returns a SCID alias if the zeroconf channel has never been confirmed).
@pytest.mark.parametrize("payment_method", ["pay", "getroute_sendpay"])
@pytest.mark.parametrize("confirm_zeroconf_channel", [False, True], ids=["no_confirm_zeroconf", "yes_confirm_zeroconf"])
@pytest.mark.parametrize("open_existing_normal_channel", [False, True], ids=["zeroconf_only", "add_existing_normal"])
def test_cln_sendpay_weirdness(bitcoind, node_factory, open_existing_normal_channel, payment_method, confirm_zeroconf_channel):
    # 0. Setup

    # 0.1 Setup the payer node
    l1 = node_factory.get_node()
    print("CARL: DONE l1 setup")

    # 0.2 Setup the receiver node
    zeroconf_plugin = os.path.join(os.getcwd(), 'plugin/zeroconf-selective.py')
    l2 = node_factory.get_node(
        options={
            'plugin': zeroconf_plugin,
            'zeroconf-allow': l1.info['id'],
        },
    )
    print("CARL: DONE l2 setup")

    # 0.3 Connect the nodes
    l1.connect(l2)

    # 0.4 Optionally open a normal channel l1 -> l2 if we're testing that
    if open_existing_normal_channel:
        normal_sats = 20_000
        print("CARL: Opening normal channel btw l1 and l2")
        l1.fundchannel(l2, amount=normal_sats)  # This will mine a block!
        print("CARL: DONE: Opening normal channel btw l1 and l2")

    # 0.5 Define a helper that syncs nodes to bitcoind and returns the blockheight
    def synced_blockheight(nodes):
        blockcount = bitcoind.rpc.getblockcount()
        print(f"CARL: bitcoind blockcount {blockcount}")
        wait_for(lambda: all([node.rpc.getinfo()['blockheight'] == blockcount for node in nodes]))
        return blockcount


    # 1. Open a zeoconf channel l1 -> l2
    zeroconf_sats = 100_000

    # 1.1 Add funds to l1's wallet for the channel open
    l1.fundwallet(zeroconf_sats * 2)  # This will mine a block!
    fundchannel_blockheight = synced_blockheight([l1, l2])
    print(f"CARL: blockheight before fundchannel: {fundchannel_blockheight}")

    # 1.2 Open the zeroconf channel
    print("CARL: Opening zeroconf channel btw l1 and l2")
    l1.rpc.fundchannel(l2.info['id'], zeroconf_sats, announce=False, mindepth=0)
    print("CARL: DONE Opening zeroconf channel btw l1 and l2")

    # 1.2 Optionally generate a block to confirm the zeroconf channel if we're testing that
    if confirm_zeroconf_channel:
        bitcoind.generate_block(1)
        l1.daemon.wait_for_log(r'Funding tx [a-f0-9]{64} depth 1 of 0')
        l2.daemon.wait_for_log(r'Funding tx [a-f0-9]{64} depth 1 of 0')

    # 1.3 Wait until the channel becomes active
    print("CARL: Waiting until channel becomes active")
    num_channels = 4 if open_existing_normal_channel else 2
    wait_for(lambda: len(l1.rpc.listchannels()['channels']) == num_channels)
    wait_for(lambda: len(l2.rpc.listchannels()['channels']) == num_channels)
    print("CARL: DONE Waiting until channel becomes active")

    # 1.4 Print out channels as seen from both nodes
    channels = l1.rpc.listchannels()
    print(f"CARL: l1's channels after zeroconf channel open:\n{json.dumps(channels, indent=4)}")
    channels = l2.rpc.listchannels()
    print(f"CARL: l2's channels after zeroconf channel open:\n{json.dumps(channels, indent=4)}")


    # 2. Have l2 generate an invoice to be paid
    invoice_sats = 50_000
    inv = l2.rpc.invoice(invoice_sats * 1_000, "test", "test")
    psecret = l1.rpc.decodepay(inv['bolt11'])['payment_secret']
    rhash = inv['payment_hash']


    # 3. Send a payment over the zeroconf channel
    riskfactor=0

    ## 3.1 Sanity check that we're at the block height we expect
    payment_blockheight = synced_blockheight([l1, l2])
    print(f"CARL: blockheight before payment: {payment_blockheight}")
    if confirm_zeroconf_channel:
        assert(fundchannel_blockheight + 1 == payment_blockheight)
    else:
        assert(fundchannel_blockheight == payment_blockheight)

    if payment_method == "getroute_sendpay":
        ## 3.2 Get a route to l2
        route = l1.rpc.getroute(l2.info['id'], 1, riskfactor)['route']
        print(f"CARL: l1's route to l2 for 1sat: {json.dumps(route, indent=4)}")
        route = l1.rpc.getroute(l2.info['id'], invoice_sats * 1_000, riskfactor)['route']
        print(f"CARL: l1's route to l2 for invoice amount {invoice_sats * 1_000}msat: {json.dumps(route, indent=4)}")

        ## 3.3 Send the payment via SENDPAY
        print("CARL: SENDPAY via l1's route to l2 for invoice")
        l1.rpc.sendpay(route, rhash, payment_secret=psecret, bolt11=inv['bolt11'])
        result = l1.rpc.waitsendpay(rhash)
        assert(result.get('status') == 'complete')
        print("CARL: DONE: SENDPAY via l1's route to l2 for invoice")
    elif payment_method == "pay":
        ## 3.2 Alternatively, send the payment via PAY
        print("CARL: PAY via l1's route to l2 for invoice")
        l1.rpc.pay(inv['bolt11'], riskfactor=riskfactor)
        print("CARL: DONE: PAY via l1's route to l2 for invoice")
    else:
        raise
@rustyrussell rustyrussell added this to the v23.02 milestone Dec 12, 2022
@dongcarl dongcarl changed the title Unable to pay down zeroconf channel when there's existing channel Peculiarities w/re paying down zeroconf channels w/ pay and getroute+sendpay Dec 14, 2022
@dongcarl
Copy link
Contributor Author

See #5846 for a resolution to the getroute+sendpay part of this

rustyrussell added a commit to rustyrussell/lightning that referenced this issue Feb 2, 2023
Modifications from issue ElementsProject#5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
rustyrussell added a commit to rustyrussell/lightning that referenced this issue Feb 2, 2023
If we only specify the node_id, we get the "first" channel.

Closes: ElementsProject#5803
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `pay` uses the correct local channel for payments when there are multiple available (not just always the first!)
rustyrussell pushed a commit to rustyrussell/lightning that referenced this issue Feb 2, 2023
Modifications from issue ElementsProject#5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
rustyrussell added a commit to rustyrussell/lightning that referenced this issue Feb 2, 2023
If we only specify the node_id, we get the "first" channel.

Closes: ElementsProject#5803
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `pay` uses the correct local channel for payments when there are multiple available (not just always the first!)
rustyrussell pushed a commit that referenced this issue Feb 2, 2023
Modifications from issue #5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
ddustin pushed a commit to ddustin/lightning that referenced this issue Apr 11, 2023
Modifications from issue ElementsProject#5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
ddustin pushed a commit to ddustin/lightning that referenced this issue Apr 11, 2023
If we only specify the node_id, we get the "first" channel.

Closes: ElementsProject#5803
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `pay` uses the correct local channel for payments when there are multiple available (not just always the first!)
gkrizek pushed a commit to voltagecloud/lightning that referenced this issue Apr 26, 2023
Modifications from issue ElementsProject#5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
gkrizek pushed a commit to voltagecloud/lightning that referenced this issue Apr 26, 2023
If we only specify the node_id, we get the "first" channel.

Closes: ElementsProject#5803
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `pay` uses the correct local channel for payments when there are multiple available (not just always the first!)
ddustin pushed a commit to ddustin/lightning that referenced this issue May 12, 2023
Modifications from issue ElementsProject#5803 to work here:

1. import json
2. Add xfail
3. Increase channel sizes by 10x so we can open them
4. Fix plugin path

Signed-off-by: Rusty Russell <[email protected]>
ddustin pushed a commit to ddustin/lightning that referenced this issue May 12, 2023
If we only specify the node_id, we get the "first" channel.

Closes: ElementsProject#5803
Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `pay` uses the correct local channel for payments when there are multiple available (not just always the first!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants