feat: actually check push data bytes

[ChrisCho-H]

Miniscript witness item can be maximum 72 bytes(except length prefix), but it only throws error when it's >= 4294967296 bytes as only dependant on PushBytesError from bitcoin crate. I've changed the logic to check the maximum length internally so that actual check can be done.

[apoelstra]

Is it possible to hit these new error paths? I think the Satisfier trait is only able to produce witnesses that are in-range (no matter how maliciously you implement it) because all its methods return types that have limited size.

[apoelstra]

Oh, of course, the script itself could be too large! Ok concept ACK.

[apoelstra]

....which I think is also what's triggering your new tighter expect in witness_to_scriptsig. Interesting. I think this is an existing panicking bug in rust-miniscript. (Though kinda an academic one since to trigger it in our existing code you need to construct a Miniscript which encodes to more than 4Gb.) (But upstream we are considering tightening this to 32M to match Bitcoin consensus rules, and in that case we need to fix this!!)

[ChrisCho-H]

My trigger is actually just the error message of All pushes in miniscript are < 73 bytes. It seems weird not to check the maximum bytes with such error message.

However, I've realized that it needs to allow the script itself from your comment... maybe we can allow 520 bytes(using MAX_SCRIPT_ELEMENT_SIZE) for redeem script(which is the last element of vector for p2sh, but not for bare) and 72 bytes for others?

The problem you mentioned might belong to PushBytes itself in bitcoin crate, not specific to witness_to_scriptsig.

[ChrisCho-H]

I've added the logic to check the size of redeem script and other push separately 9dbaad3.

Or we might change only the error message, which confuses.

[apoelstra]

I would suggest squashing these two commits because they have substantial overlap in the code that they change.

Secondly, I'm struggling to understand the point of calling map_err and and_then on something we're just going to unwrap.

My suggestion, if you want to make the assertion more precise, would be to add a debug_assert above the push line, then change the expect message to just say "checked above".

Your method of checking whether we are on the last push is also pretty inefficient. One better way to do this is to add .enumerate() to the iterator and then compare the index to witness.len() - 1.

[apoelstra]

It can also be a real assert if you want. No need to debug_assert.

[ChrisCho-H]

Thanks for great review! I've fixed following your suggestion.

[apoelstra]

324b33d is looking great!

But now it doesn't look like witness_to_scriptsig actually returns an error anywhere :).

Could this PR be reduced to just adding the new assertions and not changing the signature of witness_to_scriptsig at all?

[ChrisCho-H]

You are right. No need to change the original signature. Reduced down by e2e9281!

[apoelstra]

ACK e2e9281 successfully ran local tests; thanks for iterating!