Change TxType type from uint16_t to uint8_t

[kostyantyn]

what we want to maintain
This TxType type is used in Coin and snapshot::UTXOSubset and we serialize
it as uint8_t to reduce the disk usage as 256 types of transactions must be
more than enough for all future transactions that we might create.

possible issue
Since TxType is uint16_t there is a risk that we create a record
that will have a number larger than 255 and it will cause incorrect
value to be stored in the Coin or transferred via snapshot.

proposed fix
To make it consistent on how this field is used,
we should stick to uint8_t everywhere.

Signed-off-by: Kostiantyn Stepaniuk kostia@thirdhash.com

[frolosofsky]

Since TxType is uint16_t there is a risk that we create a record
that will have a number larger than 255 and it will cause incorrect
value to be stored in the Coin or transferred via snapshot.

The reasoning we have at the moment is misleading, with this rationale we must change type in the snapshot instead.

Instead, I'd change the description. The rationale of this PR is to adjust TxType underlying type to the value which could satisfy our need for now and for nearest perspective.

utACK for code change f052459.

[kostyantyn]

@frolosofsky I wrote few reasons. The first paragraph of the description says that we want to save 1 byte so we keep uint8_t in snapshot and Coin. This is the reason we don't change to uint16_t.

basically, the description is grouped like this:

1. what we want to have
2. what is the issue with the current approach
3. how to fix

What is your suggestion on how the description should look like?

[kostyantyn]

@frolosofsky I extended the first paragraph which explains what we want to achieve. Let me, please, know if it's clearer.

edited: Also added titles to every paragraph. Hope it brings also extra clarity

[AM5800]

utACK f052459
It is nice to have uniform types

[kostyantyn]

Tested on a testnet, haven't noticed it breaks the protocol.

[scravy]

I would much prefer if we did not just trial-and-error understand whether a change breaks the protocol but if we actually understood what the repercussions of a given change are.

Having said that, please do think about the protocol and maybe take into consideration how GetType and SetType look like:
https://github.com/dtr-org/unit-e/blob/master/src/primitives/transaction.h#L271

[kostyantyn]

@scravy this is, of course, was checked and we have tests that:
https://github.com/dtr-org/unit-e/blob/master/src/test/transaction_tests.cpp#L832-L841
https://github.com/dtr-org/unit-e/blob/master/src/test/bloom_tests.cpp#L184-L235

During serializing, we cast uint8_t to uint16_t so the leading byte is always 0
During unserialization, we cast uint16_t to uint8_t which will drop the leading byte.

Do you see how the issue can occur using our client? Because the rationality of this PR is to avoid the error in our client first.

Thinking about the external client, if they will set the first byte of nVersion, it will cause the overflow and in this case, we might pick incorrect tx, but then we won't able to validate it and will reject.

[scravy]

See #1011 and the referenced code path. My concern is not about the change itself but how it is approached. From a protocol perspective this is just not sound and should be cleaned up properly.