[docs]: Confusion in code comments regarding the encryption of justice-kit

[oren-z0]

Background

I recently started learning about watchtowers. Many online papers explain that watchtowers receive from clients the parameters for the revocation transaction (a.k.a justice-kit?) in the following way:

• They receive a "hint" for the breach transaction-id that they are looking for: the first half (16 bytes) of the transaction-id.
• They receive the revocation transaction encrypted, and the encryption-key the full transaction-id of the breach commitment transaction.

This is useful for privacy reasons - no need for the watchtower to know about the revocation transaction in the optimistic use-case, where the breach transaction is not submitted.

However, I've noticed from your code that:

• The hint is calculated from the first half of the SHA256 of the trasnaction-id:

  lnd/watchtower/blob/derivation.go

  Lines 18 to 26 in 0dd58ee

  	// NewBreachHintFromHash creates a breach hint from a transaction ID.
  	func NewBreachHintFromHash(hash *chainhash.Hash) BreachHint {
  	h := sha256.New()
  	h.Write(hash[:])
  	var hint BreachHint
  	copy(hint[:], h.Sum(nil))
  	return hint
  	}

• The encryption-key is calculated from SHA256(txid || txid):

  lnd/watchtower/blob/derivation.go

  Lines 53 to 71 in 0dd58ee

  	// NewBreachHintAndKeyFromHash derives a BreachHint and BreachKey from a given
  	// txid in a single pass. The hint and key are computed as:
  	//
  	// hint = SHA256(txid)
  	// key = SHA256(txid || txid)
  	func NewBreachHintAndKeyFromHash(hash *chainhash.Hash) (BreachHint, BreachKey) {
  	var (
  	hint BreachHint
  	key BreachKey
  	)
  	h := sha256.New()
  	h.Write(hash[:])
  	copy(hint[:], h.Sum(nil))
  	h.Write(hash[:])
  	copy(key[:], h.Sum(nil))
  	return hint, key
  	}

This makes sense from a cryptographic viewpoint, because revealing to the watchtower even half of the encryption-key is a bad practice, and its better to have both the hint and the encryption-key derived from hashing the transaction-id. However, I think there are some comments left from the old implementation that don't make sense:

• lnd/watchtower/wtclient/backup_task.go

  Lines 342 to 343 in 0dd58ee

  	// Compute the breach key as SHA256(txid).
  	hint, key := blob.NewBreachHintAndKeyFromHash(&breachTxID)

  says "Compute the breach key as SHA256(txid)." instead of SHA256(txid || txid), and says nothing about the hint that is also calculated there.

• lnd/watchtower/wtclient/backup_task_internal_test.go

  Line 655 in 0dd58ee

  // Verify that the breach hint matches the breach txid's prefix.

  says "Verify that the breach hint matches the breach txid's prefix." even though the hint must match the prefix of the SHA256 of the full txid, not only the txid's prefix.

• lnd/watchtower/lookout/lookout.go

  Lines 203 to 208 in 0dd58ee

  	// The decryption key for the state update should be the full
  	// txid of the breaching commitment transaction.
  	// The decryption key for the state update should be computed as
  	// key = SHA256(txid).
  	breachTxID := commitTx.TxHash()
  	breachKey := blob.NewBreachKeyFromHash(&breachTxID)

  says "The decryption key for the state update should be computed as SHA256(txid)" instead of SHA256(txid || txid)

[ellemouton]

@oren-z0 - are you interested in creating a PR to these comments more clear? I'd be happy to review it

[oren-z0]

@ellemouton Done: #9220