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

connectd: check all chain hashs of the network array in init #3371

Merged
merged 3 commits into from
Dec 28, 2019

Conversation

darosior
Copy link
Collaborator

@darosior darosior commented Dec 23, 2019

As per lightning/bolts#682 (comment).

We would always check against the first element of the array.. Once again my bad.

This also cleans up a redundant check as pointed out by @rustyrussell.

Changelog-None

@darosior darosior changed the title connectd: check all chain hashs of the array connectd: check all chain hashs of the network array in init Dec 23, 2019
@rustyrussell
Copy link
Contributor

Ack 9fa93df

@rustyrussell
Copy link
Contributor

Ack 9fa93df

@rustyrussell rustyrussell reopened this Dec 24, 2019
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

needs a test (specifically to show it fixes the reported error would be ideal) 😉

@@ -75,12 +75,6 @@ static struct io_plan *peer_init_received(struct io_conn *conn,
* - MAY fail the connection.
*/
if (tlvs->networks) {
if (!tlvs->networks->chains) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this change belongs in a separate commit from the first, as they're not related changes

@cdecker
Copy link
Member

cdecker commented Dec 24, 2019

Ack 9fa93df

@rustyrussell you're offline, remember? 😜

@darosior
Copy link
Collaborator Author

Separated into two commits.. Looking at the test now

@darosior
Copy link
Collaborator Author

darosior commented Dec 25, 2019

Could finally test it using protocol tests, will cleanup and submit a PR there. What do you think ?

@darosior
Copy link
Collaborator Author

Finally I also added a commit to return an error instead of io_closeing the connection, which broke protocol tests.

Tests at ElementsProject/lightning-rfc-protocol-test#13

@cdecker cdecker self-assigned this Dec 27, 2019
@cdecker cdecker added this to the 0.8.1 milestone Dec 27, 2019
@cdecker
Copy link
Member

cdecker commented Dec 27, 2019

Only missing a test for this. I think the pyln-proto library might be candidate to inject the init message here.

@darosior
Copy link
Collaborator Author

@cdecker isn't the protocol test better to test this ? I at first wanted to use pyln-proto, but then prefered ElementsProject/lightning-rfc-protocol-test#13

@cdecker cdecker merged commit 77eb9e4 into ElementsProject:master Dec 28, 2019
@darosior darosior deleted the network_tlv_fix branch December 28, 2019 13:23
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

Successfully merging this pull request may close these issues.

4 participants