Move max funding and payment values to chainparams#1842
Move max funding and payment values to chainparams#1842bisoge wants to merge 5 commits intoElementsProject:masterfrom
Conversation
|
|
||
| add_err = channel_add_htlc(peer->channel, REMOTE, id, amount_msat, | ||
| add_err = channel_add_htlc(peer->channel, &peer->chain_hash, | ||
| REMOTE, id, amount_msat, |
There was a problem hiding this comment.
I prefer to hand in the struct chainparams here, and fail at channeld init if we don't know it. Doing the lookup is not really add_htlc's job, IMHO.
| *... | ||
| * - MUST set `funding_satoshis` to less than 2^24 satoshi. | ||
| */ | ||
| #define MAX_FUNDING_SATOSHI ((1 << 24) - 1) |
There was a problem hiding this comment.
I dislike removing spec quotes (our build checks they're valid, so if spec changes we know where to fix): can we move this into chainparams.c, and then use this value there?
channeld/channel.c
Outdated
| master_badmsg(WIRE_CHANNEL_OFFER_HTLC, inmsg); | ||
|
|
||
| e = channel_add_htlc(peer->channel, LOCAL, peer->htlc_id, | ||
| e = channel_add_htlc(peer->channel, &peer->chain_hash, |
channeld/channel.c
Outdated
| failmsg = tal_fmt(inmsg, "Too many HTLCs"); | ||
| goto failed; | ||
| case CHANNEL_ERR_UNKNOWN_CHAIN_HASH: | ||
| failcode = WIRE_REQUIRED_CHANNEL_FEATURE_MISSING; |
There was a problem hiding this comment.
Pretty sure this is not the correct failcode for an unsupported blockchain.
channeld/test/run-full_channel.c
Outdated
| assert(channel_add_htlc(channel, sender, 1337, msatoshi, 900, &rhash, | ||
| dummy_routing, NULL) == CHANNEL_ERR_ADD_OK); | ||
| assert(channel_add_htlc(channel, | ||
| &chainparams_by_index(0)->genesis_blockhash, |
There was a problem hiding this comment.
This index needs to be pulled out into a #define or a named const.
channeld/full_channel.c
Outdated
| *... | ||
| * - the chain_hash value is set to a hash of a chain that is unknown | ||
| * to the receiver. | ||
| */ |
There was a problem hiding this comment.
Why was this removed? This is perfectly reasonable and blockchain agnostic.
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
The BOLT quotes need to be re-enabled and the magic constants for blockchain 0 need to be extracted into named defines or consts.
|
Thanks for the reviews, I believe all the requested changes are addressed. I ended up adding the chain params to the peer struct, not totally necessary but makes the code simpler. If you would prefer to keep them out of the struct they can always be looked up on demand based on genesis blockhash. For the test I just switched to a lookup by name instead of using the magic number. If that is not clear enough I can still add a define somewhere. Finally I was able to restore the BOLT quote and pass the source check. I am not sure why based on the text in the RFC (possibly a bug with check-bolt?), but it wants to match: |
For Litecoin use 60x BTC values to stay consistent with lnd. Also enforces chain hash values are known to receiver.
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
There is really no reason to do the chain_hash check in the add_htlc method at all, so why are we passing that info down, just to fail if something isn't right (which should never happen)?
channeld/full_channel.c
Outdated
| * - the chain_hash value is set to a hash of a chain that is unknown | ||
| * to the receiver. | ||
| */ | ||
| const struct chainparams *chain_params = chainparams_by_hash(chain_hash); |
There was a problem hiding this comment.
I don't understand why this is being checked here at all. The chain_hash is channel-specific, and the chain is implicitly communicated through the channel that is being updated, so we should not be passing this down here at all.
|
I proposed #1913 as a replacement for this change. |
3 participants
For Litecoin use 60x BTC values to stay consistent with lnd.
Also enforces chain hash values are known to receiver.