I noticed this while reviewing #5478. Apparently it's safe enough, but it makes me uneasy anyway. If this is desired, I'm happy to rebase this after #5478's merge.
Since uint256 has .begin() and .size(), I don't see why we should trust that a pointer to the object lines up with the data we're after rather than using those functions.
I did a naive grep to find these, there are likely others as well.
We test somewhere that sizeof(uint256) is 256 bits, so in principle these constructions should be safe. That said, using begin() is better code style.
#5478 does away with external access to uint256's guts completely, which means big-endian can work. This change still makes sense for blob256 tho.
Not sure how far you want to take this, but BTW also ALL usages of BEGIN and END could be replaced, except for (6) which is special as it covers the entire block header:
1. main.cpp: hashRand = Hash(BEGIN(hashRand), END(hashRand));
2. main.cpp: hashKey = Hash(BEGIN(hashKey), END(hashKey));
3. main.cpp: hashRand = Hash(BEGIN(hashRand), END(hashRand));
4. merkleblock.cpp: return Hash(BEGIN(left), END(left), BEGIN(right), END(right));
5. merkleblock.cpp: return Hash(BEGIN(left), END(left), BEGIN(right), END(right));
6. primitives/block.cpp: return Hash(BEGIN(nVersion), END(nNonce));
7. primitives/block.cpp: vMerkleTree.push_back(Hash(BEGIN(vMerkleTree[j+i]), 8. END(vMerkleTree[j+i]),
8. primitives/block.cpp: BEGIN(vMerkleTree[j+i2]), END(vMerkleTree[j+i2])));
9. primitives/block.cpp: hash = Hash(BEGIN(*it), END(*it), BEGIN(hash), END(hash));
10. primitives/block.cpp: hash = Hash(BEGIN(hash), END(hash), BEGIN(*it), END(*it));
There's also a rogue FLATDATA in COutPoint:
primitives/transaction.h: READWRITE(FLATDATA(*this));
This covers the entire structure which is a blob256 and an int. I had to replace this with
template <typename Stream, typename Operation>
inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
- READWRITE(FLATDATA(*this));
+ READWRITE(hash);
+ READWRITE(n);
}
... but I'd leave above (6) and this one for a different pull (e.g. when we actually introduce bigendian compatibility) and just focus on using .begin() / .end() in this one for the cases that span one object.
(n.b the size test I mentioned above didn't actually exist yet, I added this to blob256_tests.cpp in #5487 to verify that there is no unnecessary space in the structures, just in case
BOOST_CHECK(R1S.size() == sizeof(R1S));
BOOST_CHECK(sizeof(R1S) == 32);
BOOST_CHECK(R1L.size() == sizeof(R1L));
BOOST_CHECK(sizeof(R1L) == 20);
)
Yes, since the size is tested, I suppose these are always safe. I'll wait until after #5478 to mess with this.
Do you consider this change to be worth making? Since it boils down to little more than style (as long as no storage is added to the underlying hash container), I'm fine with just letting it go.
Closing this, I agree it is a little bit of a style improvement, but seems too low-priority to bother.