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

AMP payments #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

AMP payments #362

wants to merge 1 commit into from

Conversation

antonilol
Copy link
Contributor

@antonilol antonilol commented Apr 18, 2022

#353 + more commits

depends on stakwork/sphinx-stack#33

closes #361
closes #401

@antonilol
Copy link
Contributor Author

lncli -n regtest sendpayment --dest=02a38857848aca6b32ebcc3c85d07ee41354988f4f1e0b4e6ccd255eee6ed75b8d --amt=1500000 --amp
+------------+--------------+--------------+--------------+-----+----------+-----------------+---------+
| HTLC_STATE | ATTEMPT_TIME | RESOLVE_TIME | RECEIVER_AMT | FEE | TIMELOCK | CHAN_OUT        | ROUTE   |
+------------+--------------+--------------+--------------+-----+----------+-----------------+---------+
| SUCCEEDED  |        0.020 |        0.137 | 750000       | 0   |      206 | 158329674465280 | bob-lnd |
| SUCCEEDED  |        0.032 |        0.135 | 750000       | 0   |      206 | 169324790743040 | bob-lnd |
+------------+--------------+--------------+--------------+-----+----------+-----------------+---------+
Amount + fee:   1500000 + 0 sat
Payment hash:   3e344bfdbe845774c24506c2efb6a97752fb40c5c081cd0d4ada8e2afca0a612
Payment status: SUCCEEDED, preimage: 9639b62a3b1322d1f6e7fee6b5c0c8b76317ec02f0623e6a2940993300e7d9bd

amp works from alice to bob in the docker containers with 2 channels of 1 mil sat each on the sphinx stack branch with accept amp in the confs

@antonilol
Copy link
Contributor Author

sometimes the test fails when adding the contacts (even before the payment)

npx ava src/tests/controllers/ampMessage.test.ts --verbose --serial --timeout=2m

alice and bob
adding contact
  ✖ test-15-ampMessage: send more sats than one channel can handle to test AMP Rejected promise returned by test
  ─

  test-15-ampMessage: send more sats than one channel can handle to test AMP

  Rejected promise returned by test. Reason:

  'failed to getContactAndCheckKeyExchange'

  ─

  1 test failed

@kevkevinpal
Copy link
Contributor

sometimes the test fails when adding the contacts (even before the payment)

npx ava src/tests/controllers/ampMessage.test.ts --verbose --serial --timeout=2m

alice and bob
adding contact
  ✖ test-15-ampMessage: send more sats than one channel can handle to test AMP Rejected promise returned by test
  ─

  test-15-ampMessage: send more sats than one channel can handle to test AMP

  Rejected promise returned by test. Reason:

  'failed to getContactAndCheckKeyExchange'

  ─

  1 test failed

Yea I would check what alice-lnd.sphinx is logging out I was getting destination doesn't support AMP payments

you might need to run the test locally to see that log

@antonilol
Copy link
Contributor Author

Yea I would check what alice-lnd.sphinx is logging out I was getting destination doesn't support AMP payments

you might need to run the test locally to see that log

i tried to reproduce the error, but it worked.
i will look through some logs to see where the ci fails and try that again

@antonilol
Copy link
Contributor Author

found an amp test case in lnd! i will try to replicate this and hope it works

https://github.com/lightningnetwork/lnd/blob/master/lntest/itest/lnd_amp_test.go#L444-L453

@antonilol
Copy link
Contributor Author

amp tests are working 884d86d

@antonilol
Copy link
Contributor Author

i think everything works except queryRoutes, and when it fails other runs get cancelled.
when i run queryRoutes on my machine it fails no matter how much retries i give it, but if i restart the stack
docker-compose down docker-compose up -d docker wait sphinx-stack-relaysetup-1 sphinx-stack-lndsetup-1
and run the test again it works. will try this in the next commit for the workflow

@antonilol antonilol marked this pull request as ready for review April 19, 2022 17:26
@antonilol
Copy link
Contributor Author

note that stakwork/sphinx-stack#33 has to be merged before this one

@antonilol
Copy link
Contributor Author

antonilol commented Apr 19, 2022

integration-test (queryRoutes) constantly fails, i need some help with this one

@antonilol
Copy link
Contributor Author

antonilol commented Apr 19, 2022

=> queryRoutes alice -> bob
alice success prob to bob: 0.95
=> queryRoutes alice -> carol
alice success prob to carol: 0.95
=> queryRoutes bob -> carol
  ✖ checkContacts Rejected promise returned by test
  ─

  checkContacts

  Rejected promise returned by test. Reason:

  StatusCodeError {
    error: {
      error: '2 UNKNOWN: unable to find a path to destination',
      success: false,
    },
    ...

alice has 2 1mil sat channels to bob and 2 1mil sat channels to carol
the direct way works, but from bob to carol (with alice as hop) doesnt seem to work but did before

src/grpc/lightning.ts Outdated Show resolved Hide resolved
@antonilol
Copy link
Contributor Author

with the extra ln channel queryRoutes also succeeds

@kevkevinpal
Copy link
Contributor

with the extra ln channel queryRoutes also succeeds

awesome it might have been an issue with opening multiple channels to the same node

@antonilol
Copy link
Contributor Author

with the extra ln channel queryRoutes also succeeds

awesome it might have been an issue with opening multiple channels to the same node

i dont know

now the queryroutes only queries direct routes because of the 3rd channel, before this test case worked with an indirect path. it only succeeds because the indirect query is removed

@kevkevinpal
Copy link
Contributor

one more thing we should do on stack and in the ampMessage test is to have a node that doesn't have amp enabled yet since lots of people who are using sphinx won't upgrade till the future so I was thinking having the node dave running but only having keysend enabled for him and testing to see if you're able to send a message or keysend payment to him still as a fall back if amp is not an option

@kevkevinpal
Copy link
Contributor

with the extra ln channel queryRoutes also succeeds

awesome it might have been an issue with opening multiple channels to the same node

i dont know

now the queryroutes only queries direct routes because of the 3rd channel, before this test case worked with an indirect path. it only succeeds because the indirect query is removed

not sure I follow, what do you mean by direct route and indirect route?

@antonilol
Copy link
Contributor Author

not sure I follow, what do you mean by direct route and indirect route?

first there was a channel from alice to bob and from alice to carol. alice has 2 direct routes to peers bob and carol

bob and carol have and indirect route (via alice), but now the third channel is between bob and carol, so they have a direct route now

@antonilol
Copy link
Contributor Author

one more thing we should do on stack and in the ampMessage test is to have a node that doesn't have amp enabled yet since lots of people who are using sphinx won't upgrade till the future so I was thinking having the node dave running but only having keysend enabled for him and testing to see if you're able to send a message or keysend payment to him still as a fall back if amp is not an option

we can disable amp on carol since he only routes the payment between alice and bob

@antonilol
Copy link
Contributor Author

alice only sent htlcs to bob and none to carol, the amounts dont even add up to 1.5 mil, so trying with more timeout (8af831c)

                ...
                {
                    "incoming": false,
                    "amount": "750000",
                    "hash_lock": "822f2f6ac9e025b51ddbbc454383c8ff760cb4b69f0c94500a9a43101688ba86",
                    "expiration_height": 322,
                    "htlc_index": "1",
                    "forwarding_channel": "0",
                    "forwarding_htlc_index": "0"
                },
                {
                    "incoming": false,
                    "amount": "187500",
                    "hash_lock": "56d97bfffb9ae51f827e305ec66ba54dab4477b71ceff3fc76316548805339f9",
                    "expiration_height": 322,
                    "htlc_index": "2",
                    "forwarding_channel": "0",
                    "forwarding_htlc_index": "0"
                },
                {
                    "incoming": false,
                    "amount": "35156",
                    "hash_lock": "bc7f3b189bc78aa7d092390f34d0e7593fe1d646862e3edd3beb2b27b248aaba",
                    "expiration_height": 322,
                    "htlc_index": "3",
                    "forwarding_channel": "0",
                    "forwarding_htlc_index": "0"
                }
                ...

@kevkevinpal
Copy link
Contributor

kevkevinpal commented Apr 20, 2022

alice only sent htlcs to bob and none to carol, the amounts dont even add up to 1.5 mil, so trying with more timeout (8af831c)

Do you know if nodes that do not have amp enabled can route amp shards? If that's the case we may need another node to test the keysend portion

@antonilol
Copy link
Contributor Author

alice only sent htlcs to bob and none to carol, the amounts dont even add up to 1.5 mil, so trying with more timeout (8af831c)

Do you know if nodes that do not have amp enabled can route amp shards? If that's the case we may need another node to test the keysend portion

for a node to send AMPs, the node must have support for it, to receive it it must have support and accept-amp=true, for a router node AMPs look no different than other HTLCs and accept and route them without special flags/support

@antonilol
Copy link
Contributor Author

image

well well, i knew it it was a routing problem. by setting a large delay (i did 5 minutes) the LNDs get time enough to know about eachother's channels. this would probably also fix the one with queryRoutes on an indirect path (carol -> alice -> bob)

@kevkevinpal
Copy link
Contributor

image

well well, i knew it it was a routing problem. by setting a large delay (i did 5 minutes) the LNDs get time enough to know about eachother's channels. this would probably also fix the one with queryRoutes on an indirect path (carol -> alice -> bob)

Ok weird lol but it makes sense, I guess it takes a bit for the channel gossip to get out to the network. But the tests passing are a good thing

@antonilol
Copy link
Contributor Author

I will clean up the code if there are some useless comments/placeholders/console logs and clean up this whole commit mess with a few force pushes

@kevkevinpal
Copy link
Contributor

I will clean up the code if there are some useless comments/placeholders/console logs and clean up this whole commit mess with a few force pushes

Sounds good and we still need to add a sendPayment in the amp test to check to see if keysend works to Carol

@antonilol antonilol force-pushed the amp branch 2 times, most recently from f9122d9 to e68a3f6 Compare April 21, 2022 18:16
@antonilol
Copy link
Contributor Author

hmmm still some failing checks... will take some more time debugging

@antonilol antonilol force-pushed the amp branch 8 times, most recently from ec0255e to 0ccf1e8 Compare October 4, 2022 21:13
@antonilol antonilol force-pushed the amp branch 2 times, most recently from cf61854 to e51c1e4 Compare October 9, 2022 10:36
@antonilol
Copy link
Contributor Author

antonilol commented Oct 9, 2022

all tests pass except 2 which are caused by stakwork/sphinx-stack#47, and 1 that is fixed by #445 and probably caused by other changes not updating the warning count

i think i can say that all technical stuff works now regarding AMP payments with sphinx relay

@kevkevinpal and @Evanfeenstra can you review this

there is one major backwards compatibility issue:
when a payment is received, only if inv.is_keysend the content will be parsed as sphinx message (src/grpc/subscribe.ts:34), i had to change this because amp payments are not keysend. but this means non updated relays wont see amp payments as messages but as regular 3 sat payment. the keysend fallback will only work if their lnd does not accept amp, because only then an error over lightning is sent back.
this issue can be solved by adding an extra if that checks if some date has passed, a hardcoded date from when we think all nodes have upgraded to at least receive sphinx amp messages. i think setting this date to 6 months in the future will be enough and spamming planet sphinx with 'guys upgrade your relay' of course :)

btw, i was able to fix the amp proxy issue (bob did not accept keysend, and good i did test this because the virtualnodes could not send anything to bob). this brought me to another issue: rpc_proxy (and maybe proxy itself) doesnt support amp yet (so only send keysends)
the function that sends the payment basically looks like this:

if greenlight {
  send keysend
} else {
  if proxy {
    send keysend
  } else {
    send amp, if that fails send keysend
  }
}

@antonilol
Copy link
Contributor Author

rebased

@antonilol
Copy link
Contributor Author

the tests used to run successful here, i only rebased (no new changes) since then, do you know of changes (on the master branch) that might have caused these tests to fail?
are there any plans on when/how to introduce this change? i outlined some problems in this comment (mainly backwards compatibility)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

AMP proxy server errors - @$2000000 Enabling AMP payments - $@1,000,000
3 participants