Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSizeComputer and remove witness size constants #708

Closed
8 tasks
MatthewLM opened this issue Aug 17, 2023 · 3 comments
Closed
8 tasks

Fix CSizeComputer and remove witness size constants #708

MatthewLM opened this issue Aug 17, 2023 · 3 comments
Labels

Comments

@MatthewLM
Copy link
Contributor

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 eb696d7 but re-added in 195b0fa 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 eb696d7 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 MatthewLM added the bug label Aug 17, 2023
@lateminer
Copy link
Contributor

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 added a commit to lateminer/blackcoin-more that referenced this issue Oct 29, 2023
lateminer added a commit to lateminer/blackcoin-more that referenced this issue Nov 4, 2023
@lateminer
Copy link
Contributor

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
Copy link
Contributor

...and in Bitcoin Core 27.0 everything has been deeply refactored. See bitcoin/bitcoin#28438 for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants