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

Sendpay custom onion #1715

Closed
wants to merge 7 commits into from
Closed

Sendpay custom onion #1715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
83b897f
sphinx: pubkeys can be const in create_onionpacket
bitonic-cjp Jun 14, 2018
47b3ae6
sphinx: refactor the way how onion packet serialization is done
bitonic-cjp Jun 13, 2018
2161a6f
pay: separate generating the route data from the rest of send_payment
bitonic-cjp Jun 14, 2018
e09f61e
pay: in json_sendpay, use send_payment_custom_route_data
bitonic-cjp Jun 18, 2018
525c2c0
pay: add realm and data elements to sendpay route input
bitonic-cjp Jun 19, 2018
035602f
sphinx: don't zero the realm value
bitonic-cjp Jun 19, 2018
544cbb0
test_pay.py: test for rejection of unknown realm value
bitonic-cjp Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions common/sphinx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ static void generate_key_set(const u8 secret[SHARED_SECRET_SIZE],
static struct hop_params *generate_hop_params(
const tal_t *ctx,
const u8 *sessionkey,
struct pubkey path[])
const struct pubkey path[])
{
int i, j, num_hops = tal_count(path);
secp256k1_pubkey temp;
Expand Down Expand Up @@ -314,10 +314,7 @@ static void serialize_hop_data(tal_t *ctx, u8 *dst, const struct hop_data *data)
{
u8 *buf = tal_arr(ctx, u8, 0);
towire_u8(&buf, data->realm);
towire_short_channel_id(&buf, &data->channel_id);
towire_u64(&buf, data->amt_forward);
towire_u32(&buf, data->outgoing_cltv);
towire_pad(&buf, 12);
towire(&buf, data->per_hop_data, PAYLOAD_SIZE);
towire(&buf, data->hmac, SECURITY_PARAMETER);
memcpy(dst, buf, tal_count(buf));
tal_free(buf);
Expand All @@ -328,16 +325,13 @@ static void deserialize_hop_data(struct hop_data *data, const u8 *src)
const u8 *cursor = src;
size_t max = HOP_DATA_SIZE;
data->realm = fromwire_u8(&cursor, &max);
fromwire_short_channel_id(&cursor, &max, &data->channel_id);
data->amt_forward = fromwire_u64(&cursor, &max);
data->outgoing_cltv = fromwire_u32(&cursor, &max);
fromwire_pad(&cursor, &max, 12);
fromwire(&cursor, &max, &data->per_hop_data, PAYLOAD_SIZE);
fromwire(&cursor, &max, &data->hmac, SECURITY_PARAMETER);
}

struct onionpacket *create_onionpacket(
const tal_t *ctx,
struct pubkey *path,
const struct pubkey *path,
struct hop_data hops_data[],
const u8 *sessionkey,
const u8 *assocdata,
Expand Down Expand Up @@ -368,7 +362,6 @@ struct onionpacket *create_onionpacket(

for (i = num_hops - 1; i >= 0; i--) {
memcpy(hops_data[i].hmac, nexthmac, SECURITY_PARAMETER);
hops_data[i].realm = 0;
generate_key_set(params[i].secret, &keys);
generate_cipher_stream(stream, keys.rho, ROUTING_INFO_SIZE);

Expand Down Expand Up @@ -453,6 +446,40 @@ struct route_step *process_onionpacket(
return step;
}

void serialize_per_hop_data(
const struct short_channel_id *channel_id,
u64 amt_forward,
u32 outgoing_cltv,
u8 *per_hop_data
)
{
u8 *buf = tal_arr(tmpctx, u8, 0);

towire_short_channel_id(&buf, channel_id);
towire_u64(&buf, amt_forward);
towire_u32(&buf, outgoing_cltv);
towire_pad(&buf, 12);

memcpy(per_hop_data, buf, tal_bytelen(buf));
tal_free(buf);
}

void deserialize_per_hop_data(
const u8 *per_hop_data,
struct short_channel_id *channel_id,
u64 *amt_forward,
u32 *outgoing_cltv
)
{
const u8 *cursor = per_hop_data;
size_t max = PAYLOAD_SIZE;

fromwire_short_channel_id(&cursor, &max, channel_id);
*amt_forward = fromwire_u64(&cursor, &max);
*outgoing_cltv = fromwire_u32(&cursor, &max);
fromwire_pad(&cursor, &max, 12);
}

u8 *create_onionreply(const tal_t *ctx, const struct secret *shared_secret,
const u8 *failure_msg)
{
Expand Down
43 changes: 34 additions & 9 deletions common/sphinx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,7 @@ enum route_next_case {
*/
struct hop_data {
u8 realm;
struct short_channel_id channel_id;
u64 amt_forward;
u32 outgoing_cltv;
/* Padding omitted, will be zeroed */
u8 per_hop_data[PAYLOAD_SIZE];
u8 hmac[SECURITY_PARAMETER];
};

Expand All @@ -84,17 +81,16 @@ struct route_step {
*
* @ctx: tal context to allocate from
* @path: public keys of nodes along the path.
* @hoppayloads: payloads destined for individual hosts (limited to
* HOP_PAYLOAD_SIZE bytes)
* @num_hops: path length in nodes
* @hops_data: data destined for individual hosts.
* hops_data[..].hmac will be filled in by this function.
* @sessionkey: 32 byte random session key to derive secrets from
* @assocdata: associated data to commit to in HMACs
* @assocdatalen: length of the assocdata
* @path_secrets: (out) shared secrets generated for the entire path
*/
struct onionpacket *create_onionpacket(
const tal_t * ctx,
struct pubkey path[],
const struct pubkey path[],
struct hop_data hops_data[],
const u8 * sessionkey,
const u8 *assocdata,
Expand All @@ -121,7 +117,6 @@ bool onion_shared_secret(
* @ctx: tal context to allocate from
* @packet: incoming packet being processed
* @shared_secret: the result of onion_shared_secret.
* @hoppayload: the per-hop payload destined for the processing node.
* @assocdata: associated data to commit to in HMACs
* @assocdatalen: length of the assocdata
*/
Expand All @@ -133,6 +128,36 @@ struct route_step *process_onionpacket(
const size_t assocdatalen
);

/**
* serialize_per_hop_data - Serialize the per_hop data (for realm 0)
*
* @channel_id: the short channel ID
* @amt_forward: the amount to forward
* @outgoing_cltv: the CLTV value of the outgoing HTLC
* @per_hop_data: the serialized data buffer
*/
void serialize_per_hop_data(
const struct short_channel_id *channel_id,
u64 amt_forward,
u32 outgoing_cltv,
u8 *per_hop_data
);

/**
* deserialize_per_hop_data - Deserialize the per_hop data (for realm 0)
*
* @per_hop_data: the serialized data buffer
* @channel_id: the short channel ID
* @amt_forward: the amount to forward
* @outgoing_cltv: the CLTV value of the outgoing HTLC
*/
void deserialize_per_hop_data(
const u8 *per_hop_data,
struct short_channel_id *channel_id,
u64 *amt_forward,
u32 *outgoing_cltv
);

Copy link
Collaborator

@wythe wythe Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here and in other places the style should be a little more like:

void deserialize_per_hop_data(const u8 *per_hop_data,
                              struct short_channel_id *channel_id,
                              u64 *amt_forward,
                              u32 *outgoing_cltv);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should just have a clang-format linting step in travis...

Copy link
Member

Choose a reason for hiding this comment

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

I'm running clang-format on patches that I create, but it's slow progress and having travis check it is really painful if the code mostly does its own thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to create a new commit to fix this, but then I saw the surrounding, old code was inconsistent w.r.t. this coding style. For instance, the process_onionpacket declaration does not conform to your style suggestion.

Instead of only fixing my new code, it might be better to make one commit that fixes all code. Including such a commit in this pull request would mess up the readability of this pull request, since you'd have a mix of functional and non-functional changes, obscuring the view on where the functional changes are.

So I agree it should be done, but I don't think it should be done right here right now. Let's make a separate pull request that makes the coding style consistent across the entire code base.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we maybe commit a .clang-format config to format all future code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could just copy what the linux kernel does:

https://www.kernel.org/doc/html/v4.17/process/clang-format.html

And here's their config file:

https://github.com/torvalds/linux/blob/master/.clang-format

I currently just use indent -kr -i8 -l80 and then just touch-up the result by hand.

/**
* serialize_onionpacket - Serialize an onionpacket to a buffer.
*
Expand Down
13 changes: 9 additions & 4 deletions devtools/onion.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,17 @@ static void do_generate(int argc, char **argv)
}

for (int i = 0; i < num_hops; i++) {
struct short_channel_id channel_id;
memset(&hops_data[i], 0, sizeof(hops_data[i]));
hops_data[i].realm = i;
memset(&hops_data[i].channel_id, i,
sizeof(hops_data[i].channel_id));
hops_data[i].amt_forward = i;
hops_data[i].outgoing_cltv = i;
memset(&channel_id, i,
sizeof(channel_id));
serialize_per_hop_data(
&channel_id,
i,
i,
hops_data[i].per_hop_data
);
fprintf(stderr, "Hopdata %d: %s\n", i, tal_hexstr(NULL, &hops_data[i], sizeof(hops_data[i])));
}

Expand Down
Loading