Skip to content

Larger internal messages #296

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

Merged
merged 4 commits into from
Sep 29, 2017

Conversation

rustyrussell
Copy link
Contributor

TODO: include stress test that triggered it.

…'s specified.

For our own internal comms CSVs, we should always name explicit types.

Signed-off-by: Rusty Russell <[email protected]>
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Despite these being internal messages only I think we should stick to network byte order, we might need it later.

wire/wire_io.c Outdated
if (arg->u2.s == INSIDE_HEADER_BIT + HEADER_LEN) {
arg->u2.s = be16_to_cpu(*(be16 *)p);
arg->u2.s = *(wire_len_t *)p;
Copy link
Member

Choose a reason for hiding this comment

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

Ever since implementing the Bitcoin protocol I have a very strong aversion against using native host byte order for anything that could potentially be used in a networked scenario. I know we currently don't send this anywhere but the same host, but I'd use network byte order anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, was a line-ball for me. I'll fix.

rustyrussell and others added 2 commits September 29, 2017 10:26
We don't anticipate daemons across machines, but you never know.

Suggested-by: Christian Decker
Signed-off-by: Rusty Russell <[email protected]>
@cdecker
Copy link
Member

cdecker commented Sep 29, 2017

Had to rerun CI a few times, we have a flaky test test_penalty_outhtlc, but this seems to work 👍

@cdecker cdecker merged commit f35e296 into ElementsProject:master Sep 29, 2017
@rustyrussell rustyrussell deleted the larger-internal-messages branch October 27, 2017 10:07
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.

2 participants