ChannelKeyManager: add optional list of spent outputs to sign() methods

[sstone]

This is needed to sign inputs that spend taproot outputs.

[t-bast]

I don't understand: you're not actually using this new field anywhere, but you should? In particular in ReplaceableTxFunder, there are a few places where we use keyManager.sign and actually have added wallet inputs: not providing them in the new field is a bug! We shouldn't care about whether we're using taproot or not at that point, this is unnecessarily risky: we should just always provide the additional wallet inputs we're adding.

I don't think this new field should be optional with a default None value, that makes us miss places where we must fill it if we add wallet inputs. It's also unclear in Transactions.scala whether this should contain all inputs or all inputs that are not already our InputInfo.

Overall, I think that this new field should:

• not have a default value of None in methods
• should be for wallet inputs that we've added (ie not include the InputInfo for TransactionWithInputInfo, otherwise it creates unnecessary duplication)

You should also update the "generate valid commitment and htlc transactions (taproot)" test in TransactionsSpec.scala to show that when we add wallet inputs, not providing this field results in silently producing a signature that is invalid.

[sstone]

I don't understand: you're not actually using this new field anywhere, but you should? In particular in ReplaceableTxFunder, there are a few places where we use keyManager.sign and actually have added wallet inputs: not providing them in the new field is a bug! We shouldn't care about whether we're using taproot or not at that point, this is unnecessarily risky: we should just always provide the additional wallet inputs we're adding.

I could not find a way to demonstrate that signing in ReplaceableTxFunder will fail if these inputs are missing but you're right: I could simple verify that wallet inputs are indeed passed to the signing method even of they are not needed.

I don't think this new field should be optional with a default None value, that makes us miss places where we must fill it if we add wallet inputs. It's also unclear in Transactions.scala whether this should contain all inputs or all inputs that are not already our InputInfo.

It would be cleaner to only pass additional wallet inputs, but it means that we expect our InputInfo's output to always be the first input that is spent.

[sstone]

I went with your proposal in cd74ecc. It's cleaner, and the implicit requirement to always add additional inputs after the one we want to bump is acceptable (it's what we do now). I also added a test to ReplaceableTxPublisherSpec to verify that wallet inputs are all passed as extra inputs to the sign() method.

[sstone]

Rebased on master at 3d415bc