Transaction: Use `Script` in `hasForWitnessSignature`

[msgilligan]

Take a Script rather than byte[] in the canonical implementation.

In related methods, instead of providing "convenience" overloads to take Script have deprecated overloaded methods that take byte[].

~~Marked as DRAFT because it will probably need a rebase after PR #3671 is merged.~~

[schildbach]

Ah, and of course scripts could be unparsable – throwing an exception. Yet maybe users want to sign such unparsable scripts? Not sure if this is a usecase tbh.

[msgilligan]

We can migrate byte[] to Script, however note that Script.parse() parses the bytes immediately (not on-demand), so this could be a performance issue for some.

I was able to deprecate all the methods that take byte[] and they are not used elsewhere in bitcoinj. These methods are only called when signing or verifying transactions and (I believe) they are trivial compared to the actual signing/verify. If it becomes an issue for someone we can address it then. Getting rid of the four now-deprecated methods seems like a big win for simplicity.

I think this is the main reason why scripts are often kept as unparsed byte arrays, e.g. in TransactionInputs and TransactionOutputs.

I'm sure it is, but on current JVMs the performance difference is probably negligible and not worth the complexity. If we were to implement a full-node in bitcoinj it might be an issue. And until we get Valhalla, there is (minor) memory size issue with the chunks, but I think it would only be major in a full node.

As much as I like proper types, in this case we may have to look into how we can have unparsed but typed scripts – maybe using subclasses?

The simplest thing to do might be to implement lazy parsing for scripts that are constructed from byte[] -- that would be the vast majority of them in almost all use cases. We could also make Script an interface and have two different implementations. (We might want to do that anyway to get rid of the @Nullable creation time field.

But I think we should merge this PR now and worry about those issues later (if at all.)

[schildbach]

I agree about the negliable performance difference when signing. What do you think about the problem that unparseable scripts cannot be signed any more?

If we merge this, I think we (in a second commit) also migrate hashForSignature() (the code path used for non-segwit signing). Can you add that already, so we're confident we'll not get stuck half-way? (Maybe rebase the branch on master first.)

[msgilligan]

I agree about the negliable performance difference when signing. What do you think about the problem that unparseable scripts cannot be signed any more?

I guess there are two use-cases for signing unparseable scripts:

1. There is a valid script that we can't parse (due to a bug in bitcoinj or a protocol change)
2. For testing

I think we can address these cases later with a sub-class (or sibling class) when needed.

If we merge this, I think we (in a second commit) also migrate hashForSignature() (the code path used for non-segwit signing). Can you add that already, so we're confident we'll not get stuck half-way?

Yeah, I'll take a look.