peercoin icon indicating copy to clipboard operation
peercoin copied to clipboard

Fix CSizeComputer and remove witness size constants

Open MatthewLM opened this issue 2 years ago • 2 comments

The size and vsize of transactions, and the size and strippedsize of blocks are displayed identically even with witness transactions. When PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS is passed to GetSerializeSize, it has no effect because it is incorrectly passed as the nType and not the nVersion.

This is because nType was removed in eb696d7c66 but re-added in 195b0fac3c without reverting the function calls to include SER_NETWORK.

As witness bytes are no longer discounted due to this bug, the code should be altered to remove references to the WITNESS_SCALE_FACTOR. The nType in CSizeComputer and GetSerializeSize should be removed and all calls to these should be fixed.

My proposed fix:

  • [ ] Ensure there is a test to require that blocks with and without witness data cannot exceed 1MB (current and desired behaviour since this bug).
  • [ ] Change CSizeComputer to match how it was originally intended in commit eb696d7c66 and currently is in Bitcoin with nType removed.
  • [ ] Remove WITNESS_SCALE_FACTOR, MAX_BLOCK_WEIGHT and MAX_BLOCK_SERIALIZED_SIZE.
  • [ ] Revert to using MAX_BLOCK_SIZE as 1000000. Block size checks should be done against that.
  • [ ] MIN_TRANSACTION_WEIGHT and MIN_SERIALIZABLE_TRANSACTION_WEIGHT should reference size and not weight.
  • [ ] Remove GetTransactionWeight and use GetSerializeSize without SERIALIZE_TRANSACTION_NO_WITNESS for both size, weight and vsize.
  • [ ] Any other references to weight and vsize should be removed or made equivalent to size.
  • [ ] strippedsize in blocks should use SERIALIZE_TRANSACTION_NO_WITNESS. stippedsize, weight and vsize are only kept for RPC compatibility.

MatthewLM avatar Aug 17 '23 13:08 MatthewLM

nType is currently used to send SER_POSMARKER in block headers: https://github.com/peercoin/peercoin/blob/master/src/primitives/block.h#L46

If nType is removed, this part should be also somehow adapted.

lateminer avatar Oct 29 '23 22:10 lateminer

In Bitcoin Core 26.0, nType has been removed in many more serialization-related classes (BufferedFile, CVectorWriter, CAutoFile...), so in order to keep maximum compatibility with the Bitcoin codebase, its removal has to be considered.

lateminer avatar Dec 16 '23 17:12 lateminer

...and in Bitcoin Core 27.0 everything has been deeply refactored. See https://github.com/bitcoin/bitcoin/pull/28438 for reference.

lateminer avatar May 16 '24 00:05 lateminer