Closed kostyantyn closed 5 years ago
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 f0524593d36edaf84fb6abbe71c622147d311103.
@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:
~What is your suggestion on how the description should look like?~
@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
Tested on a testnet, haven't noticed it breaks the protocol.
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
@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.
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.
what we want to maintain This
TxType
type is used inCoin
andsnapshot::UTXOSubset
and we serialize it asuint8_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
isuint16_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