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.

Sign up for GitHub

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

Jump to bottom

reactivate CLN integration tests #1336

Merged
merged 3 commits into from
Jun 17, 2024
Merged

reactivate CLN integration tests #1336

merged 3 commits into from
Jun 17, 2024

Conversation

daywalker90
Copy link
Contributor

Ontop of #1335

What does this PR do?

reactivate CLN integration tests

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

Issues

I managed to run these tests locally but encountered some issues:

  • when stopping with docker compose -f docker-tests.yml --env-file tests/compose.env down and then starting again with docker compose -f docker-tests.yml --env-file tests/compose.env up -d my btc container always starts at blockheight 0, basically completely reset. This of course makes CLN crash since it detected blockheight going back.
  • There is an issue with CLN v24.05 where it crashes on regtest if blockheight=0. I don't know where and how to best generate a block but i think it needs to happen before the Wait for coordinator (django server) job
  • After manually circumventing these 2 issues i can run the tests but at the end i get 3 errors, all the same error:
Traceback (most recent call last):
  File "/usr/src/robosats/tests/test_trade_pipeline.py", line 474, in test_trade_to_locked_escrow
    trade.lock_escrow(trade.taker_index)
  File "/usr/src/robosats/tests/utils/trade.py", line 196, in lock_escrow
    self.get_order()
  File "/usr/src/robosats/tests/utils/trade.py", line 112, in get_order
    self.response = self.client.get(path   params, **headers)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 289, in get
    response = super().get(path, data=data, **extra)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 206, in get
    return self.generic('GET', path, **r)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 234, in generic
    return super().generic(
           ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/test/client.py", line 617, in generic
    return self.request(**r)
           ^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 286, in request
    return super().request(**kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/test.py", line 238, in request
    request = super().request(**kwargs)
              ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/test/client.py", line 1013, in request
    self.check_exception(response)
  File "/usr/local/lib/python3.12/site-packages/django/test/client.py", line 743, in check_exception
    raise exc_value
  File "/usr/local/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper
    return view_func(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/viewsets.py", line 124, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/usr/local/lib/python3.12/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/usr/local/lib/python3.12/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/robosats/api/views.py", line 339, in get
    data["trade_satoshis"] = Logics.payout_amount(order, request.user)[
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/robosats/api/logics.py", line 740, in payout_amount
    f"Suggested mining fee is {order.payout_tx.suggested_mining_fee_rate} Sats/vbyte, the swap fee rate is {order.payout_tx.swap_fee_rate}%"
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'suggested_mining_fee_rate'

Any pointers why this happens could help me figure out what's wrong.

Reckless-Satoshi
Reckless-Satoshi reviewed Jun 16, 2024
View reviewed changes
Copy link
Collaborator

@Reckless-Satoshi Reckless-Satoshi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • when stopping with docker compose -f docker-tests.yml --env-file tests/compose.env down and then starting again with docker compose -f docker-tests.yml --env-file tests/compose.env up -d my btc container always starts at blockheight 0, basically completely reset. This of course makes CLN crash since it detected blockheight going back.

We have to stop with docker compose -f docker-tests.yml --env-file tests/compose.env down --volumes (note the --volumes) , effectively trashing everything: the blockchain and the lightning node. On every start, a new CLN node should be created and peered with the user node. That's all pretty much already automated in /tests/utils/node.py;s set_up_regtest_network() and was working long time ago. This is needed to ensure full reproducibility of tests, every test run starts from the same point, that is, from scratch.

There is an issue with CLN v24.05 where it crashes on regtest if blockheight=0. I don't know where and how to best generate a block but i think it needs to happen before the Wait for coordinator (django server) job

Two options here:

  1. The easy one would be to add a generate_blocks(address, num_blocks=1) at the beginning of /tests/utils/node.py;s set_up_regtest_network() around L165. We should call this function with any hardcoded valid regtest bitcoin address, because we cannot use the lightning nodes to generate an address just yet.
  2. If (1) is not early enough and CLN crashes even before the tests run... I guess we can write an entrypoint.sh that uses bitcoin-cli --rpcuser .... generateblocks <hardcoded_regtest_address/> to mine a single block?

After manually circumventing these 2 issues i can run the tests but at the end i get 3 errors, all the same error:

Happy to see so many tests did pass! I actually never managed so many successes with CLN. Never found anything in specific broken, but the tests would randomly fail after a few orders (hodl invoices did not reach).

The error logs seem to point that CLN might not be returning a mining fee estimate? This might also be due to regtest environment. This needs some testing, but if this hypothesis is true, we could write some logic on /api/lightning/cln.py estimate_fee() that will return 20 sat/vbyte if network==regtest .

Thank you for your work 🚀

@daywalker90
Copy link
Contributor Author

I actually could not reproduce the error with the correct down command. Anyways i added an entrypoint to generate a block.

@daywalker90 daywalker90 marked this pull request as ready for review June 17, 2024 06:50
@daywalker90
Copy link
Contributor Author

Everrything should be passing now: https://github.com/daywalker90/robosats/actions/runs/9542920360/job/26298571087

@Reckless-Satoshi
Copy link
Collaborator

Woop! 🚀
These tests passing makes me very confident we can now see mainnet robosats coordinators with CLN.

Thank you @daywalker90 !

Interesting that the coverage of the CLN class is somewhat low (well this is true too for LND I believe).

api/lightning/cln.py                                                                435    205    53%

Please post an invoice with a long expiration time ( 1 week) for 300K Sats.

@Reckless-Satoshi Reckless-Satoshi merged commit 62e6258 into RoboSats:main Jun 17, 2024
1 check failed
@daywalker90
Copy link
Contributor Author

Yes, i'd love to see one. I would do it myself but doing support kinda scares me off. LND has 54% btw. If i can get a quick grasp on how the testing framework works i might pump those numbers up, no promises tho.

Thanks for the sats :)

lnbc3m1pn8pv5esp5c5hz00telunev62zdqfm5m0js5eyta4wla08dp6f676zargujj6qpp5zsm89dp2wfhcm678a7nvlur4eg3jp5usf5la0qjd3p9q8zsgjcjqdqqxq9pycjqcqpjrzjqw5nhpalnuzjhr5x94g7hwkyee0f0d05zd6k8n239p2g6l6e0rw6jr9cgsqq0sgqqvqqqqryqqqqqqqq2q9qxpqysgq9959vfpauf4yc880k2tswxtutzcxke968qc4uxhrpa9wser2yee4ezgs423zf0rld2eeyn4tnpgkks5splns9dvl8vkwdndc2mfktpsp8d8w0d

This was referenced Jun 17, 2024
Closed
Closed
@Reckless-Satoshi
Copy link
Collaborator

If i can get a quick grasp on how the testing framework works i might pump those numbers up, no promises tho.

The way I would go with it is to get the HTML report

docker exec coordinator coverage run manage.py test
docker exec coordinator coverage html

Take a look at the html output and then either:

  1. Write a unit test for those specific functions
  2. Better, create a trade order condition that will trigger those.

I guess a lot of what is not covered has to do with mostlt with logging the onchain and channel balances, etc. Those will benefit of simple unit testing :)

@Reckless-Satoshi
Copy link
Collaborator

lnbc3m1pn8pv5esp5c5hz00telunev62zdqfm5m0js5eyta4wla08dp6f676zargujj6qpp5zsm89dp2wfhcm678a7nvlur4eg3jp5usf5la0qjd3p9q8zsgjcjqdqqxq9pycjqcqpjrzjqw5nhpalnuzjhr5x94g7hwkyee0f0d05zd6k8n239p2g6l6e0rw6jr9cgsqq0sgqqvqqqqryqqqqqqqq2q9qxpqysgq9959vfpauf4yc880k2tswxtutzcxke968qc4uxhrpa9wser2yee4ezgs423zf0rld2eeyn4tnpgkks5splns9dvl8vkwdndc2mfktpsp8d8w0d

54d6ea8da3c43ee02331ac0a590c0dc37a3bc98aa8758ed006332db39c277660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reckless-Satoshi Reckless-Satoshi Reckless-Satoshi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daywalker90 @Reckless-Satoshi @jerryfletcher21