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

Connecting with CLN peer fails with warning #100

Open
tanx opened this issue Jan 20, 2023 · 26 comments
Open

Connecting with CLN peer fails with warning #100

tanx opened this issue Jan 20, 2023 · 26 comments

Comments

@tanx
Copy link
Contributor

tanx commented Jan 20, 2023

I ran into this issue while attempting to connect with a CLN based LSP peer on testnet (no channel open, just peering). I currently get this warning message when I try to connect to the peer. I spoke with the vendor and they have been able to connect from another LDK based node, so they suggested it might be an issue in the react-native module. This is the config I'm running: tanx@a7b01bf. Thank you 🙏

LDK: Finished noise handshake for connection with 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e
 LOG  react-native-ldk: Success: add_peer_success
 LOG  addPeer Responses: ["add_peer_success"]
 LOG  Node ID: 02ebdcef4e8ce903da7555a00dd607b1d3aa77dbcd4eb3ec0b730c2bc6311cf4ce
 LOG  Backup updated for account wallet0
 LOG  LDK: Received peer Init message from 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e: DataLossProtect: supported, InitialRoutingSync: not supported, UpfrontShutdownScript: supported, GossipQueries: supported, VariableLengthOnion: required, StaticRemoteKey: supported, PaymentSecret: required, BasicMPP: supported, Wumbo: not supported, ShutdownAnySegwit: supported, OnionMessages: not supported, ChannelType: supported, SCIDPrivacy: supported, ZeroConf: supported, unknown flags: supported
 LOG  LDK: Generating channel_reestablish events for 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e
 LOG  react-native-ldk: Persisted channel manager to disk
 LOG  LDK: Got warning message from 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e: gossip_timestamp_filter for bad chain: 0109000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f494363b44b82ffffffff
 LOG  LDK: Marking channels with 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e disconnected and generating channel_updates. We believe we can make future connections to this peer.
@Jasonvdb
Copy link
Collaborator

Are you able to replicate this on regtest using something like Polar?

@tanx
Copy link
Contributor Author

tanx commented Feb 6, 2023

Well with regtest on polar I can't connect to the CLN peer either, only the LND peer. Calling listpeers also only displays the pubkey of the LND peer.

@tanx
Copy link
Contributor Author

tanx commented Feb 7, 2023

@Jasonvdb can you confirm that connecting to a CLN peer does not work for you either?

@tnull
Copy link

tnull commented Feb 8, 2023

LOG LDK: Got warning message from 025804d4431ad05b06a1a1ee41f22fefeb8ce800b0be3a92ff3b9f594a263da34e: gossip_timestamp_filter for bad chain: 0109000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f494363b44b82ffffffff

This looks like it is the correct genesis hash for Testnet, however, it's likely in wrong endianess, as I don't see where the byte order is swapped. @Jasonvdb could you check whether you're handing the genesis hash in the right order to LDK?

@Jasonvdb
Copy link
Collaborator

Jasonvdb commented Feb 9, 2023

I'm testing this on a local regtest and I'm not able to get it to add CLN as a peer. I get a true result from peerHandler.connect(...) in swift but the peer is never added. Nothing in the logs also.

@tnull I don't get the bad chain warning though. I'm not changing the byte order anywhere except for funding_txid where I noticed I had to reverse bytes. Would that be a problem specifically for CLN though? Because we can add LND and Eclair as a peer fine.

@tnull
Copy link

tnull commented Feb 9, 2023

@tnull I don't get the bad chain warning though. I'm not changing the byte order anywhere except for funding_txid where I noticed I had to reverse bytes. Would that be a problem specifically for CLN though? Because we can add LND and Eclair as a peer fine.

Well, likely only CLN chooses to close the connection upon receiving a bogus chain hash. LDK does not, not sure about LND. Note this is in line with BOLT 1 as either is allowed:

  • upon receiving networks containing no common chains
    - MAY close the connection.

@tanx
Copy link
Contributor Author

tanx commented Feb 11, 2023

I'm testing this on a local regtest and I'm not able to get it to add CLN as a peer. I get a true result from peerHandler.connect(...) in swift but the peer is never added. Nothing in the logs also.

@tnull I don't get the bad chain warning though. I'm not changing the byte order anywhere except for funding_txid where I noticed I had to reverse bytes. Would that be a problem specifically for CLN though? Because we can add LND and Eclair as a peer fine.

@Jasonvdb thanks for verifying. So it indeed looks like there is a reproducable issue in the react-native module that prevents connecting to a CLN node.

Well, likely only CLN chooses to close the connection upon receiving a bogus chain hash. LDK does not, not sure about LND. Note this is in line with BOLT 1 as either is allowed:

@tnull mmh, if CLN ist following the spec a little more strictly then perhaps the bug just isn't triggered with LND peers?

@Jasonvdb how can I help in debugging the root cause?

@tnull
Copy link

tnull commented Feb 11, 2023

@tnull mmh, if CLN ist following the spec a little more strictly then perhaps the bug just isn't triggered with LND peers?

Right, this is likely what happens.

@Jasonvdb how can I help in debugging the root cause?

I think you could try to find where the network graph is setup and byte-swap the genesis hash before it is passed in.

Note we'll also mitigate the issue upstream with the next release by replacing the genesis hash parameter with Network, which doesn't allow for any confusion over byte order (see lightningdevkit/rust-lightning#2025)

@Jasonvdb
Copy link
Collaborator

Tested this quickly and reversing bytes of genesis hash doesn't seem to help. Can still connect to other nodes but not CLN.

I also noticed I needed to reverse currentBlockchainTipHash when initialising a new node. I'll add these in #103 but I still wasn't able to connect to a CLN node.

@tnull
Copy link

tnull commented Feb 13, 2023

Tested this quickly and reversing bytes of genesis hash doesn't seem to help. Can still connect to other nodes but not CLN.

I also noticed I needed to reverse currentBlockchainTipHash when initialising a new node. I'll add these in #103 but I still wasn't able to connect to a CLN node.

Hm, does it always fail with above mentioned message?

@tanx
Copy link
Contributor Author

tanx commented Feb 23, 2023

Hey, just wanted to follow up and ask if there are any updates on this? Thanks 🙏

@gkrizek
Copy link

gkrizek commented Feb 23, 2023

I know that Mutiny Wallet is able to peer with CLN just fine and it's based on LDK. Not sure if any of their code could be helpful
https://github.com/MutinyWallet/mutiny-web
https://github.com/MutinyWallet/mutiny-node

@Jasonvdb
Copy link
Collaborator

Sorry for the delay, I'll dive into this properly next week

@tnull
Copy link

tnull commented Mar 14, 2023

@Jasonvdb Any progress on this? Happy to help debug anything on the LDK side.

@tanx
Copy link
Contributor Author

tanx commented Mar 28, 2023

Just rebased my config on top of the current master in tanx@10b2c37 and still having this issue.

@tnull
Copy link

tnull commented Mar 28, 2023

Mh, just confirmed again that I can indeed connect to the peer without issue from a Rust-based LDK peer.

Not sure if the endianness is indeed an issue here, but meanwhile 0.0.114 was released in which we remove any exposed genesis hashes from the public API to mitigate any confusion in that regard. Would be good to know if the issue still persist after the library has upgraded.

@tanx
Copy link
Contributor Author

tanx commented Mar 28, 2023

Great, thanks for verifying @tnull. Good to know we can rule out LDK core 🙏

@Jasonvdb
Copy link
Collaborator

Jasonvdb commented Apr 1, 2023

Sorry for the delay here, I was away for a while. I did reverse bytes for genesis hash and it didn't fix, nothing actually seemed to change after doing that.

I'm busy updating to 114 now so I'll test again and get back to you.

@gkrizek
Copy link

gkrizek commented Apr 12, 2023

@Jasonvdb How is the update to 114 going?

@BitcoinErrorLog
Copy link

The bindings or other aspects have caused an increase in fatal bugs. We are working with the LDK team to resolve them, but there will be delays.

@Jasonvdb
Copy link
Collaborator

Looks like there's a fix for the 114 update so hopefully this will land today #115

@Jasonvdb
Copy link
Collaborator

Tested with 114 and still no luck connecting from Android or iOS. @tnull if it wasn't the genesis hashes do you have any other ideas of what we could try?

@tnull
Copy link

tnull commented Apr 14, 2023

So I just also confirmed that I'm able to connect the CLN-based peer on testnet without issue using the Swift bindings (see https://gist.github.com/tnull/ab5df1b4d889e62ae420e51837448f31), which means that the issue is very likely on the react-native-ldk side (which is even more plausible if the issue is also present on Android, which I wasn't aware of so far).

Will have another look at the codebase to see if I find something suspicious.

@tnull
Copy link

tnull commented Apr 20, 2023

Just got the same warning message with a Rust LDK client, in fact in the same setup I checked before. Will be investigating what's going on.

@TheBlueMatt
Copy link

TheBlueMatt commented Apr 20, 2023

Huh, interesting, just checked my LDK sample (with the network graph wiped and with one loaded from disk) and I don't see it :/.

@tnull
Copy link

tnull commented Apr 20, 2023

Huh, interesting, just checked my LDK sample (with the network graph wiped) and I don't see it :/.

Argh, nevermind, I had accidently set it to mainnet this time around, then the warning makes sense. So still works fine with the Rust client. Spoke too soon, excuse the noise! 🤦‍♂️

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

No branches or pull requests

6 participants