Make chainparams available in channeld #1913
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
utACK cca47fd
|
ZmnSCPxj
approved these changes
Sep 7, 2018
|
Why don't we use the genesis block hash elsewhere, rather than having the arbitrary id? |
bisoge
reviewed
Sep 8, 2018
common/initial_channel.c
Outdated
| @@ -58,6 +60,8 @@ struct channel *new_initial_channel(const tal_t *ctx, | |||
| channel->commitment_number_obscurer | |||
| = commit_number_obscurer(&channel->basepoints[funder].payment, | |||
| &channel->basepoints[!funder].payment); | |||
| channel->chainparams = chainparams_by_chainhash(chain_hash); | |||
| assert(channel->chainparams != NULL); | |||
There was a problem hiding this comment.
Per Bolt #2 the receiver must reject the channel if the chain hash is unknown. Do we really just want to assert here?
There was a problem hiding this comment.
Excellent point, I'm making this an if (!channel->chainparams) return tal_free(channel); 👍
cdecker
force-pushed
the
chainparams
branch
2 times, most recently
from
370310b to
9a590d0
Compare
bisoge
reviewed
Sep 8, 2018
channeld/full_channel.h
Outdated
| @@ -81,6 +82,7 @@ u32 actual_feerate(const struct channel *channel, | |||
| /** | |||
| * channel_add_htlc: append an HTLC to channel if it can afford it | |||
| * @channel: The channel | |||
| * @chain_hash: genesis hash of the target blockchain | |||
There was a problem hiding this comment.
This comment can be removed, leftover from #1842
rustyrussell
approved these changes
Sep 9, 2018
cdecker
force-pushed
the
chainparams
branch
2 times, most recently
from
a88a505 to
65e9fb7
Compare
We no longer use it to reference chainparams, so we can remove it completely.
cdecker
force-pushed
the
chainparams
branch
from
65e9fb7 to
7365165
Compare
|
Rebased and squashed one fixup that fixed a doc comment. |
|
Re-applying @rustyrussell's ACK after rebase ACK 7365165 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a replacement of #1842, which passes a reference to the
chainparamsto thechannelso that we can check various constraints during runtime, instead of having global constants and magic values in the code.The fact that these become configurable is really just a side-effect.