Bug on witness verification (witness flag but empty witnesses)

[brunoerg]

By applying differential fuzzing (bitcoinfuzz) with rust-bitcoin and Bitcoin Core for block deserialization, I found out a bug during the transaction verification step. Basically, it's illegal to have the witness flag present and all witness stacks empty. That is, it has inputs, they have empty witnesses, but the transaction is flagged as segwit. It seems this case is not being catched properly.

Base64 to reproduce (for block deserialization): //////////+puampqalJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJqampKak4ODg4ODg4qampqVdRqampAgICAgIB/QITAAICAgICAgICAAABAABRAgICAgH9AgICAgICAgICAgICAgICAgICAgICAACRAAIAAAAAAAAAAFECAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgAAApECAAIAAAAAAAAAUQICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAICRAAAAAAAAFv////8AAD0A/////y3/////////AP////8AAQAAAAD///8=

[TheBlueMatt]

Hmm, right. So, the difference in serialization is very deliberate on the transaction level (and in previous discussion with Bitcoin Core folks they seemed interested in changing their transaction deserializer to match ours at least for RPC, see #2238), but this really shouldn't carry over to blocks! Rather, we should explicitly reject any blocks that transactions that hit this case.

[apoelstra]

@TheBlueMatt to be clear, this isn't about zero-input transactions (where we do deliberately deviate from Core) but transactions that have inputs but don't use segwit. I think in this case the correct behavior is to reject the marker, even for individual transactions.

[brunoerg]

@TheBlueMatt to be clear, this isn't about zero-input transactions (where we do deliberately deviate from Core) but transactions that have inputs but don't use segwit. I think in this case the correct behavior is to reject the marker, even for individual transactions.

Exactly.

[TheBlueMatt]

Ah, sorry, my mistake. We should reject those as well. We should also reject zero-input transactions in blocks :)

[benthecarman]

Would be good to add to https://github.com/bitcoin-core/qa-assets

[brunoerg]

Would be good to add to https://github.com/bitcoin-core/qa-assets

I'll check it.

[maflcko]

Would be good to add to https://github.com/bitcoin-core/qa-assets

My understanding is that the corresponding error message in Bitcoin Core for the rejection of this type of block is Superfluous witness record. Fuzz inputs that yield this error message can already be found in the block and block_deserialize folders, whose format should be suitable for the bitcoinfuzz differential fuzz target block_des: https://github.com/brunoerg/bitcoinfuzz/blob/63d03224514e94c847b2f2c6aaf60f04df9fe5d9/targets/block_des.cpp#L12

So I think it could make sense to re-use the qa-assets fuzz inputs for differential fuzzing, because years of CPU have been spent on them. Though, if fuzz inputs with new coverage are found by bitcoinfuzz, it also makes sense to feed them back to qa-assets.

[brunoerg]

Fuzz inputs that yield this error message can already be found in the block and block_deserialize folders, whose format should be suitable for the bitcoinfuzz differential fuzz target block_des

I intentionally wrote this target to be compatible with the qa-assets fuzz inputs. Unfortunately, it will not be possible for all the targets.