sauron: Add mutinynet and tests

[ca-ruz]

Adds mutinynet support to sauron. Mutinynet.com uses the mempool.space REST API, which has a slightly different spec from blockstream's Esplora so there is new code to handle that.

We pinned pyln-client to <=24.5 because version 24.8 has some extra requirements that sauron does not meet, we should probably fix this but for now pinning 24.5 should work fine.

[sip-21]

You need to pin the version of pyln-client in requirements.txt as
pyln-client==24.5 as otherwise a newer version gets installed which causes this error when starting the plugin:
lightningd: --sauron-api-endpoint=https://mutinynet.com/api/: unknown option

[sip-21]

You might want to add some instructions in the README for how to start sauron with Mutinynet. Below command worked for me once I had pyln-client==24.5 installed (see comment above):
lightningd --signet --disable-plugin bcli --plugin $PWD/sauron.py --sauron-api-endpoint https://mutinynet.com/api/

[chrisguida]

See comments

[chrisguida]

Btw @sip-21 noticed that this PR breaks mainnet on blockstream.info. Can you look into this @ca-ruz @iorch ?

[ca-ruz]

@sip-21 Can you please give me more details about the tests you wrote and what were you trying to do when Sauron failed? I am trying to replicate it to fix it. Thanks!

[sip-21]

@ca-ruz The failing GitHub actions/tests are here: https://github.com/sip-21/plugins/actions/runs/11116007452/job/30885476517?pr=15

[gw0] [ 20%] PASSED test_sauron.py::test_rpc_getchaininfo 
[gw1] [ 40%] ERROR test_sauron.py::test_rpc_sendrawtransaction_invalid 
[gw2] [ 60%] ERROR test_sauron.py::test_rpc_getrawblockbyheight 
[gw3] [ 80%] ERROR test_sauron.py::test_rpc_getutxout 
[gw1] [ 80%] ERROR test_sauron.py::test_rpc_sendrawtransaction_invalid 
[gw2] [ 80%] ERROR test_sauron.py::test_rpc_getrawblockbyheight 
[gw4] [100%] ERROR test_sauron.py::test_rpc_estimatefees 
[gw3] [100%] ERROR test_sauron.py::test_rpc_getutxout 
[gw4] [100%] ERROR test_sauron.py::test_rpc_estimatefees 

The tests run fine together locally, but in GitHub action lightningd doesn't seem to not shutdown properly after a test. This is why in the logs you then see

  lightningd-0 2024-10-01T00:02:01.508Z **BROKEN** connectd: Failed to listen on socket 127.0.0.1:9735: Address already in use
  Time-out: can't find [re.compile('Server started with public key')] in logs

lightningd seems to randomly be able to shutdown properly, though, and two subsequent tests pass as in this case: https://github.com/sip-21/plugins/actions/runs/11089120680/job/30809842202

[chrisguida]

@sip-21 didn't you fix the concurrency? We can get rid of this I think?

[chrisguida]

If removing this breaks the tests, we should add the changes to this file back, but under the other commit (the one that adds the tests).

[ca-ruz]

Removed these lines from the code. Tests are all failing now.

[chrisguida]

We're almost there! Just a couple of minor issues.

[chrisguida]

@sip-21

After adding back concurrency, the tests are failing because they're trying to listen on port 9735... that actually doesn't seem necessary. Can you fix the tests not to require listening on port 9735? (or maybe just listen on a random port)

[cdecker]

I don't think you need to override this class at all. Just set the options appropriately when using the NodeFactory.get_node(). That'll obviate the need for the subclass and the fixtures.

[ca-ruz]

Hey @cdecker it looks like we actually do need to override both the LightningNode class and the LightningD class.

We need to override the LightningNode class because it doesn't allow us to override self.daemon as you can see in this code here: https://github.com/ElementsProject/lightning/blob/e66653fa1d847e28bd812356a762225a44bfae83/contrib/pyln-testing/pyln/testing/utils.py#L780-L783

In addition, we have to override the LightningD class because there is no way to instantiate it without the bcli options which are set here: https://github.com/ElementsProject/lightning/blob/e66653fa1d847e28bd812356a762225a44bfae83/contrib/pyln-testing/pyln/testing/utils.py#L618-L622

These options if set, crash lightningd when bcli is disabled.

On top of this, the LightningD.startmethod requires bitcoin-rpcuser to be set (which as above crashes lightningd when we disable bcli). You can see this here: https://github.com/ElementsProject/lightning/blob/e66653fa1d847e28bd812356a762225a44bfae83/contrib/pyln-testing/pyln/testing/utils.py#L667

So unfortunately we need to leave these classes as they are unless I'm missing something.

[cdecker]

Please don't override the fixtures and the node class unless you absolutely need to. options passed to NodeFactory.gey_node() is where you can disable one and enable the other. Furthermore, the tests differ only in the backend used, so you could use pytest's parametrized Tests to reuse the implementation but get the backend as a parameter (they'd run once for each backend then). Finally, you are doing format checking which is great, but we could go one further and actually use the JSON schemas from CLN to ensure all fields are present and types match up.

[chrisguida]

@cdecker do you have an example of any pytests that use the JSON schema from CLN to check the fields and types? That sounds amazing!

[cdecker]

It's done automatically by instrumenting the pyln-client library to load and check the schemas. Anything going through pyln-client in a test environment should be checked afaik. I don't think notifications and hooks are checked yet (there are no schemas yet), but that'd be an amazing extension covering the docs vs implementation too

[ca-ruz]

@chrisguida @cdecker @sip-21 Made some changes to the tests.

[chrisguida]

@ca-ruz some tests still failing, looks like blockstream is ratelimiting us

[ca-ruz]

@chrisguida Guess we need to work on a workaround for that!

[ca-ruz]

@chrisguida @sip-21 Skipped the sendrawtransaction_invalid test across all backends since we were intentionally sending an invalid transaction to test the API call and response. Esplora had been rate-limiting us, likely due to the high frequency of invalid requests. We know the call works because we get an expected response from the API.

[chrisguida]

@ca-ruz just make an issue to refactor these tests to repeat ourselves less, and clean up the commits, then let's merge

[ca-ruz]

@chrisguida I've created an issue to address this: #622.

[chrisguida]

ACK 569dabb

#618 and #622 still open from this feature