Witness presence check not tested

[Kixunil]

This condition appears untested. I've been doing some experiments which didn't perform the check and the tests did not fail.

[Kixunil]

Another missing test: check that decoding an otherwise-valid transaction encoded with non-minimal VarInt fails.

[yancyribbens]

it looks like this needs to be re-factored also. L1090 means the empty test in the condition you mentioned won't ever be empty here.

[Kixunil]

You're confusing it with witness flag, that one is correct. (I meant the code is, didn't verify the tests.)

[yancyribbens]

You're confusing it with witness flag, that one is correct. (I meant the code is, didn't verify the tests.)

L1088 and L1095 do the same thing. I'm not saying it's incorrect, just that it looks like it decodes twice which is not needed?

I read over BIP-141 that is mentioned in the comments, but it doesn't look like this BIP describes the TX serialization, or am I missing it? Is there a different BIP that describes the serialization/de-serialization of a general transaction?

[Kixunil]

They don't, the cursor is already moved in the second call. Note that consensus_decode takes T: Read.

From quick look it looks like the BIP doesn't explicitly say that wtxid serialization is the same as P2P serialization but nevertheless it is.

[startup-dreamer]

I would like to solve this issue, But I couldn't find any test for consensus_decode_from_finite_reader can anyone please help me regarding this

[tcharding]

You should dig into the call chain starting an encode::deserialize(). Note the Decodable trait is unusual in that both methods have default implementations that call each other, this just means that an implementor must implement at least one of them.

[Kixunil]

I think you shouldn't worry about that. Just add a test that passes in an invalid transaction with the mentioned errors. Somewhere in the tests modules you'll find a list of valid and invalid hexes.

[startup-dreamer]

I'm facing an issue. I've attempted to identify hexes that might cause failures, but none of them triggered the required error. Then
I tried using a hex from segwit_transaction() test and removed the witness with the following code:

let mut tx_without_witness = realtx;
tx_without_witness.input.iter_mut().for_each(|input| input.witness.clear());

After this modification, I serialized tx_without_witness and then attempted to deserialize it, hoping that it would produce the required error. This approach also failed to yield the desired outcome.

If anyone has insights or suggestions on how to properly serialize the transaction for testing purposes, your help would be greatly appreciated.

[Kixunil]

It's because the library serializes correctly. You have to produce invalid serialization manually.

[apoelstra]

You might also be able to modify one of the fuzztests to trigger on a specific error, and then it'll find a test case for you.

[Kixunil]

I managed to find it by chance (failing the roundtrip test) when working on #2184

0000fd000001021921212121212121212121f8b372b0239cc1dff600000000004f4f4f4f4f4f4f4f000000000000000000000000000000333732343133380d000000000000000000000000000000ff000000000009000dff000000000000000800000000000000000d

So in fact it is tested by the fuzzer. However, an explicit test would still be great because it's faster to run and much easier to understand what went wrong.