bitcoinj
Wallet.completeTx: throw if inputs contain unconnected outputs
Rather than (if not segwit) simply add those unconnected outputs to the miner's fee, throw an IllegalArgumentException.
In the case of segwit, the transaction can't be properly signed without knowing the input values.
TODO before or after merging:
- [ ] Consider adding a checked exception for this new error case -- this should be considered separately, I think.
- [ ] Rename some of the variables in the unit test -- could be a second commit in this same PR
- [x] Consider adding connected outputs if they are available in the wallet. See PR #3055 which has a proposed implementation of this feature. After connecting outputs available in the wallet, if any unconnected outputs remain we would throw.
I'm fine with throwing an exception. Adding inputs with unknown value to the fee seems way too scary. And as you said, with segwit/taproot we can't continue the behaviour anyway.
I'm fine with throwing an exception. Adding inputs with unknown value to the fee seems way too scary. And as you said, with segwit/taproot we can't continue the behaviour anyway.
I think we might want to throw a checked exception in this case. This would be an API breaking change, but since we are changing behavior it might be a good idea to explicitly let people know about the new behavior.
I would have thrown a new subclass of CompletionException but that exception is a RuntimeException.
might want to throw a checked exception in this case.
The existing exceptions (e.g. DustySendRequested, MultipleOpReturnRequested) thrown by completeTx extend o.b.w.Wallet.CompletionException which extends RuntimeException, I take back my suggestion that we replace IllegalArgument with a checked exception, but we should probably create a new subclass of o.b.w.Wallet.CompletionException for this case.
I would have thrown a new subclass of
CompletionExceptionbut that exception is aRuntimeException.
We both were typing at the same time. I agree it should be a new subclass of o.b.w.W.CompletionException. (Not to be confused with j.u.c.CompletionException added in Java 8.)
And whatever exception we throw, there are quite some higher-level methods in Wallet that will need to throw that exception too. (Unless handled of course.)