doc: big BOLT update to incorporate warnings language.

We do this (send warnings) in almost all cases anyway, so mainly this is a textual update, but there are some changes: 1. Send ERROR not WARNING if they send a malformed commitment secret. 2. Send WARNING not ERROR if they get the shutdown_scriptpubkey wrong (vs upfront) 3. Send WARNING not ERROR if they send a bad shutdown_scriptpubkey (e.g. p2pkh in future) 4. Rename some vars 'err' to 'warn' to make it clear we send a warning. This means test_option_upfront_shutdown_script can be made reliable, too, and it now warns and doesn't automatically close channel. Signed-off-by: Rusty Russell <[email protected]>
stephenwebel · Apr 1, 2022 · 2526e80 · 2526e80
1 parent 9f06a59
commit 2526e80
Show file tree

Hide file tree

Showing 22 changed files with 191 additions and 144 deletions.
diff --git a/Makefile b/Makefile
@@ -24,7 +24,7 @@ CCANDIR := ccan
 
 # Where we keep the BOLT RFCs
 BOLTDIR := ../lightning-rfc/
-DEFAULT_BOLTVERSION := c876dac2b5038f6499154d0a739240b6ff5db70d
+DEFAULT_BOLTVERSION := 93909f67f6a48ee3f155a6224c182e612dd5f187
 # Can be overridden on cmdline.
 BOLTVERSION := $(DEFAULT_BOLTVERSION)
 

diff --git a/channeld/channeld.c b/channeld/channeld.c
@@ -728,7 +728,8 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg)
 	 * A receiving node:
 	 *...
 	 *  - if the sender is not responsible for paying the Bitcoin fee:
-	 *    - MUST fail the channel.
+	 *    - MUST send a `warning` and close the connection, or send an
+	 *      `error` and fail the channel.
 	 */
 	if (peer->channel->opener != REMOTE)
 		peer_failed_warn(peer->pps, &peer->channel_id,
@@ -742,7 +743,8 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg)
 	 * A receiving node:
 	 *   - if the `update_fee` is too low for timely processing, OR is
 	 *     unreasonably large:
-	 *     - SHOULD fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (!feerate_same_or_better(peer->channel, feerate,
 				    peer->feerate_min, peer->feerate_max))
@@ -757,7 +759,8 @@ static void handle_peer_feechange(struct peer *peer, const u8 *msg)
 	 *
 	 *  - if the sender cannot afford the new fee rate on the receiving
 	 *    node's current commitment transaction:
-	 *    - SHOULD fail the channel,
+	 *    - SHOULD send a `warning` and close the connection, or send an
+	 *      `error` and fail the channel.
 	 *      - but MAY delay this check until the `update_fee` is committed.
 	 */
 	if (!channel_update_feerate(peer->channel, feerate))
@@ -1627,7 +1630,8 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 	 *  - once all pending updates are applied:
 	 *    - if `signature` is not valid for its local commitment transaction
 	 *      OR non-compliant with LOW-S-standard rule...:
-	 *      - MUST fail the channel.
+	 *      - MUST send a `warning` and close the connection, or send an
+	 *        `error` and fail the channel.
 	 */
 	if (!check_tx_sig(txs[0], 0, NULL, funding_wscript,
 			  &peer->channel->funding_pubkey[REMOTE], &commit_sig)) {
@@ -1651,7 +1655,8 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 	 *...
 	 *    - if `num_htlcs` is not equal to the number of HTLC outputs in the
 	 * local commitment transaction:
-	 *      - MUST fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (tal_count(htlc_sigs) != tal_count(txs) - 1)
 		peer_failed_warn(peer->pps, &peer->channel_id,
@@ -1662,7 +1667,8 @@ static void handle_peer_commit_sig(struct peer *peer, const u8 *msg)
 	 *
 	 *   - if any `htlc_signature` is not valid for the corresponding HTLC
 	 *     transaction OR non-compliant with LOW-S-standard rule...:
-	 *     - MUST fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	for (i = 0; i < tal_count(htlc_sigs); i++) {
 		u8 *wscript;
@@ -1813,13 +1819,13 @@ static void handle_peer_revoke_and_ack(struct peer *peer, const u8 *msg)
 	 * A receiving node:
 	 *  - if `per_commitment_secret` is not a valid secret key or does not
 	 *    generate the previous `per_commitment_point`:
-	 *    - MUST fail the channel.
+	 *     - MUST send an `error` and fail the channel.
 	 */
 	memcpy(&privkey, &old_commit_secret, sizeof(privkey));
 	if (!pubkey_from_privkey(&privkey, &per_commit_point)) {
-		peer_failed_warn(peer->pps, &peer->channel_id,
-				 "Bad privkey %s",
-				 type_to_string(msg, struct privkey, &privkey));
+		peer_failed_err(peer->pps, &peer->channel_id,
+				"Bad privkey %s",
+				type_to_string(msg, struct privkey, &privkey));
 	}
 	if (!pubkey_eq(&per_commit_point, &peer->old_remote_per_commit)) {
 		peer_failed_err(peer->pps, &peer->channel_id,
@@ -1957,7 +1963,8 @@ static void handle_peer_fail_malformed_htlc(struct peer *peer, const u8 *msg)
 	 *
 	 *   - if the `BADONION` bit in `failure_code` is not set for
 	 *    `update_fail_malformed_htlc`:
-	 *      - MUST fail the channel.
+	 *      - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (!(failure_code & BADONION)) {
 		peer_failed_warn(peer->pps, &peer->channel_id,
@@ -2011,17 +2018,18 @@ static void handle_peer_shutdown(struct peer *peer, const u8 *shutdown)
 	 * feature, and the receiving node received a non-zero-length
 	 * `shutdown_scriptpubkey` in `open_channel` or `accept_channel`, and
 	 * that `shutdown_scriptpubkey` is not equal to `scriptpubkey`:
+	 *    - MAY send a `warning`.
 	 *    - MUST fail the connection.
 	 */
 	/* openingd only sets this if feature was negotiated at opening. */
 	if (tal_count(peer->remote_upfront_shutdown_script)
 	    && !memeq(scriptpubkey, tal_count(scriptpubkey),
 		      peer->remote_upfront_shutdown_script,
 		      tal_count(peer->remote_upfront_shutdown_script)))
-		peer_failed_err(peer->pps, &peer->channel_id,
-				"scriptpubkey %s is not as agreed upfront (%s)",
-			    tal_hex(peer, scriptpubkey),
-			    tal_hex(peer, peer->remote_upfront_shutdown_script));
+		peer_failed_warn(peer->pps, &peer->channel_id,
+				 "scriptpubkey %s is not as agreed upfront (%s)",
+				 tal_hex(peer, scriptpubkey),
+				 tal_hex(peer, peer->remote_upfront_shutdown_script));
 
 	/* We only accept an wrong_funding if:
 	 * 1. It was negotiated.
@@ -2528,12 +2536,12 @@ static void check_future_dataloss_fields(struct peer *peer,
  *    commitment transaction:
  * ...
  *  - if `your_last_per_commitment_secret` does not match the expected values:
- *     - SHOULD fail the channel.
+ *     - SHOULD send an `error` and fail the channel.
  *  - otherwise, if it supports `option_data_loss_protect`:
  *...
  *    - otherwise (`your_last_per_commitment_secret` or
  *     `my_current_per_commitment_point` do not match the expected values):
- *      - SHOULD fail the channel.
+ *      - SHOULD send an `error` and fail the channel.
  */
 static void check_current_dataloss_fields(struct peer *peer,
 			u64 next_revocation_number,
@@ -2950,10 +2958,10 @@ static void peer_reconnect(struct peer *peer,
 	 *    - if `next_revocation_number` is not equal to 1 greater
 	 *      than the commitment number of the last `revoke_and_ack` the
 	 *      receiving node has sent:
-	 *      - SHOULD fail the channel.
+	 *      - SHOULD send an `error` and fail the channel.
 	 *    - if it has not sent `revoke_and_ack`, AND
 	 *      `next_revocation_number` is not equal to 0:
-	 *      - SHOULD fail the channel.
+	 *      - SHOULD send an `error` and fail the channel.
 	 */
 	if (next_revocation_number == peer->next_index[LOCAL] - 2) {
 		/* Don't try to retransmit revocation index -1! */
@@ -3021,7 +3029,7 @@ static void peer_reconnect(struct peer *peer,
 	 *     - if `next_commitment_number` is not 1 greater than the
 	 *       commitment number of the last `commitment_signed` message the
 	 *       receiving node has sent:
-	 *       - SHOULD fail the channel.
+	 *       - SHOULD send an `error` and fail the channel.
 	 */
 	} else if (next_commitment_number != peer->next_index[REMOTE])
 		peer_failed_err(peer->pps,

diff --git a/channeld/full_channel.c b/channeld/full_channel.c
@@ -559,7 +559,8 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	 *...
 	 *  - if sending node sets `cltv_expiry` to greater or equal to
 	 *    500000000:
-	 *    - SHOULD fail the channel.
+	 *    - SHOULD send a `warning` and close the connection, or send an
+	 *      `error` and fail the channel.
 	 */
 	if (!blocks_to_abs_locktime(cltv_expiry, &htlc->expiry)) {
 		return CHANNEL_ERR_INVALID_EXPIRY;
@@ -584,7 +585,8 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	 * A receiving node:
 	 *  - receiving an `amount_msat` equal to 0, OR less than its own
 	 *    `htlc_minimum_msat`:
-	 *    - SHOULD fail the channel.
+	 *        - SHOULD send a `warning` and close the connection, or send an
+	 *        `error` and fail the channel.
 	 */
 	if (amount_msat_eq(htlc->amount, AMOUNT_MSAT(0))) {
 		return CHANNEL_ERR_HTLC_BELOW_MINIMUM;
@@ -652,7 +654,8 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	 *   - if a sending node... adds more than receiver
 	 *     `max_htlc_value_in_flight_msat` worth of offered HTLCs to its
 	 *     local commitment transaction:
-	 *     - SHOULD fail the channel.
+	 *     - SHOULD send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 
 	/* We don't enforce this for channel_force_htlcs: some might already
@@ -670,7 +673,8 @@ static enum channel_add_err add_htlc(struct channel *channel,
 	 *  - receiving an `amount_msat` that the sending node cannot afford at
 	 *    the current `feerate_per_kw` (while maintaining its channel
 	 *    reserve and any `to_local_anchor` and `to_remote_anchor` costs):
-	 *    - SHOULD fail the channel.
+	 *      - SHOULD send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (enforce_aggregate_limits) {
 		struct amount_msat remainder;
@@ -894,7 +898,8 @@ enum channel_remove_err channel_fulfill_htlc(struct channel *channel,
 	 *
 	 *  - if the `payment_preimage` value in `update_fulfill_htlc`
 	 *  doesn't SHA256 hash to the corresponding HTLC `payment_hash`:
-	 *    - MUST fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (!sha256_eq(&hash, &htlc->rhash))
 		return CHANNEL_ERR_BAD_PREIMAGE;
@@ -905,7 +910,8 @@ enum channel_remove_err channel_fulfill_htlc(struct channel *channel,
 	 *
 	 *  - if the `id` does not correspond to an HTLC in its current
 	 *    commitment transaction:
-	 *    - MUST fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (!htlc_has(htlc, HTLC_FLAG(!htlc_owner(htlc), HTLC_F_COMMITTED))) {
 		status_unusual("channel_fulfill_htlc: %"PRIu64" in state %s",
@@ -957,7 +963,8 @@ enum channel_remove_err channel_fail_htlc(struct channel *channel,
 	 * A receiving node:
 	 *   - if the `id` does not correspond to an HTLC in its current
 	 *     commitment transaction:
-	 *     - MUST fail the channel.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	if (!htlc_has(htlc, HTLC_FLAG(!htlc_owner(htlc), HTLC_F_COMMITTED))) {
 		status_unusual("channel_fail_htlc: %"PRIu64" in state %s",
@@ -1145,7 +1152,8 @@ static int change_htlcs(struct channel *channel,
  *...
  *   - if the sender cannot afford the new fee rate on the receiving node's
  *     current commitment transaction:
- *      - SHOULD fail the channel,
+ *     - SHOULD send a `warning` and close the connection, or send an
+ *       `error` and fail the channel.
  */
 u32 approx_max_feerate(const struct channel *channel)
 {
@@ -1257,7 +1265,8 @@ bool can_opener_afford_feerate(const struct channel *channel, u32 feerate_per_kw
 	 *
 	 *   - if the sender cannot afford the new fee rate on the receiving
 	 *     node's current commitment transaction:
-	 *     - SHOULD fail the channel
+	 *     - SHOULD send a `warning` and close the connection, or send an
+	 *       `error` and fail the channel.
 	 */
 	/* Note: sender == opener */
 

diff --git a/closingd/closingd.c b/closingd/closingd.c
@@ -291,7 +291,8 @@ receive_offer(struct per_peer_state *pps,
 	 *   - if the `signature` is not valid for either variant of closing transaction
 	 *   specified in [BOLT #3](03-transactions.md#closing-transaction)
 	 *   OR non-compliant with LOW-S-standard rule...:
-	 *     - MUST fail the connection.
+	 *     - MUST send a `warning` and close the connection, or send an
+	 *	 `error` and fail the channel.
 	 */
 	tx = close_tx(tmpctx, chainparams, pps, channel_id,
 		      local_wallet_index,

diff --git a/common/decode_array.c b/common/decode_array.c
@@ -33,10 +33,12 @@ struct short_channel_id *decode_short_ids(const tal_t *ctx, const u8 *encoded)
 	 * The receiver:
 	 *   - if the first byte of `encoded_short_ids` is not a known encoding
 	 *     type:
-	 *     - MAY fail the connection
+	 *     - MAY send a `warning`.
+	 *     - MAY close the connection.
 	 *   - if `encoded_short_ids` does not decode into a whole number of
 	 *     `short_channel_id`:
-	 *     - MAY fail the connection
+	 *     - MAY send a `warning`.
+	 *     - MAY close the connection.
 	 */
 	type = fromwire_u8(&encoded, &max);
 	switch (type) {
@@ -75,10 +77,12 @@ bigsize_t *decode_scid_query_flags(const tal_t *ctx,
 	 *...
 	 *  - if the incoming message includes `query_short_channel_ids_tlvs`:
 	 *    - if `encoding_type` is not a known encoding type:
-	 *      - MAY fail the connection
+	 *      - MAY send a `warning`.
+	 *      - MAY close the connection.
 	 *    - if `encoded_query_flags` does not decode to exactly one flag per
 	 *      `short_channel_id`:
-	 *      - MAY fail the connection.
+	 *      - MAY send a `warning`.
+	 *      - MAY close the connection.
 	 */
 	switch (qf->encoding_type) {
 	case ARR_ZLIB:

diff --git a/common/features.c b/common/features.c
@@ -123,7 +123,8 @@ static const struct dependency feature_deps[] = {
 	 * `option_anchor_outputs` | ...      | ...      | `option_static_remotekey`
 	 */
 	{ OPT_ANCHOR_OUTPUTS, OPT_STATIC_REMOTEKEY },
-	/* BOLT #9:
+	/* FIXME: This dep was added later! */
+	/* BOLT-master #9:
 	 * Name                | Description  | Context  | Dependencies  |
 	 *...
 	 * `option_anchors_zero_fee_htlc_tx` | ...      | ...      | `option_static_remotekey`

diff --git a/common/ping.c b/common/ping.c
@@ -16,7 +16,6 @@ bool check_ping_make_pong(const tal_t *ctx, const u8 *ping, u8 **pong)
 	/* BOLT #1:
 	 *
 	 * A node receiving a `ping` message:
-	 *...
 	 *  - if `num_pong_bytes` is less than 65532:
 	 *    - MUST respond by sending a `pong` message, with `byteslen` equal
 	 *      to `num_pong_bytes`.

diff --git a/common/read_peer_msg.c b/common/read_peer_msg.c
@@ -33,17 +33,24 @@ bool is_peer_error(const tal_t *ctx, const u8 *msg,
 	 * 0 (i.e. all bytes are 0), in which case it refers to all channels.
 	 * ...
 	 * The receiving node:
-	 *   - upon receiving `error`:
-	 *    - MUST fail the channel referred to by the error message, if that
-	 *      channel is with the sending node.
-	 *  - if no existing channel is referred to by the message:
-	 *    - MUST ignore the message.
+	 *  - upon receiving `error`:
+	 *     - if `channel_id` is all zero:
+	 *       - MUST fail all channels with the sending node.
+	 *     - otherwise:
+	 *       - MUST fail the channel referred to by `channel_id`, if that channel is with the sending node.
+	 *   - upon receiving `warning`:
+	 *     - SHOULD log the message for later diagnosis.
+	 *     - MAY disconnect.
+	 *     - MAY reconnect after some delay to retry.
+	 *     - MAY attempt `shutdown` if permitted at this point.
+	 *   - if no existing channel is referred to by `channel_id`:
+	 *     - MUST ignore the message.
 	 */
-	/* FIXME: The spec changed, so for *errors* all 0 is not special.
-	 * But old gossipd would send these, so we turn them into warnings */
-	if (channel_id_is_all(&err_chanid))
-		*warning = true;
-	else if (!channel_id_eq(&err_chanid, channel_id))
+
+	/* FIXME: We don't actually free all channels at once, they'll
+	 * have to error each in turn. */
+	if (!channel_id_is_all(&err_chanid)
+	    && !channel_id_eq(&err_chanid, channel_id))
 		*desc = tal_free(*desc);
 
 	return true;

diff --git a/common/wireaddr.h b/common/wireaddr.h
@@ -26,10 +26,7 @@ struct sockaddr_un;
  *
  *   * `1`: ipv4; data = `[4:ipv4_addr][2:port]` (length 6)
  *   * `2`: ipv6; data = `[16:ipv6_addr][2:port]` (length 18)
- *   * `3`: Tor v2 onion service; data = `[10:onion_addr][2:port]` (length 12)
- *       * version 2 onion service addresses; Encodes an 80-bit, truncated `SHA-1` hash
- *         of a 1024-bit `RSA` public key for the onion service (a.k.a. Tor
- *	   hidden service).
+ *   * `3`: Deprecated (length 12). Used to contain Tor v2 onion services.
  *   * `4`: Tor v3 onion service; data = `[35:onion_addr][2:port]` (length 37)
  *       * version 3 ([prop224](https://gitweb.torproject.org/torspec.git/tree/proposals/224-rend-spec-ng.txt))
  *         onion service addresses; Encodes:

diff --git a/connectd/connectd.c b/connectd/connectd.c
@@ -364,7 +364,7 @@ struct io_plan *peer_connected(struct io_conn *conn,
 	 *  - upon receiving unknown _odd_ feature bits that are non-zero:
 	 *    - MUST ignore the bit.
 	 *  - upon receiving unknown _even_ feature bits that are non-zero:
-	 *    - MUST fail the connection.
+	 *    - MUST close the connection.
 	 */
 	unsup = features_unsupported(daemon->our_features, their_features,
 				     INIT_FEATURE);

diff --git a/connectd/multiplex.c b/connectd/multiplex.c
@@ -1225,7 +1225,6 @@ void send_manual_ping(struct daemon *daemon, const u8 *msg)
 	/* BOLT #1:
 	 *
 	 * A node receiving a `ping` message:
-	 *...
 	 *  - if `num_pong_bytes` is less than 65532:
 	 *    - MUST respond by sending a `pong` message, with `byteslen` equal
 	 *      to `num_pong_bytes`.

diff --git a/connectd/peer_exchange_initmsg.c b/connectd/peer_exchange_initmsg.c
@@ -75,7 +75,7 @@ static struct io_plan *peer_init_received(struct io_conn *conn,
 	 * The receiving node:
 	 * ...
 	 *  - upon receiving `networks` containing no common chains
-	 *    - MAY fail the connection.
+	 *    - MAY close the connection.
 	 */
 	if (tlvs->networks) {
 		if (!contains_common_chain(tlvs->networks)) {