GetSerializeSize’s return type should not be platform dependent #33709

1. 
   darosior commented at 2:11 pm on October 26, 2025: member

   That GetSerializeSize returns a size_t is the root cause behind low-severity CVE-2025-46597. Now that the advisory is published, we should fix this.
2. 
   mansiverma897993 commented at 4:22 pm on October 26, 2025: none

   Please assign me this issue .
3. maflcko added the label Refactoring on Oct 27, 2025
4. maflcko added the label P2P on Oct 27, 2025
5. maflcko added the label Consensus on Oct 27, 2025
6. 
   sshivanshg commented at 10:30 am on October 27, 2025: none

   GetSerializeSize currently returns size_t, which is platform-dependent. On 32-bit systems, size_t is 32-bit, limiting the maximum serialized size to ~4GB. This can cause integer overflow when multiplying by WITNESS_SCALE_FACTOR during block size validation. Change GetSerializeSize to return uint64_t to ensure it can handle large sizes without overflow on all platforms.

   Resolves: #33709

7. 
   darosior commented at 10:59 am on October 27, 2025: member

   This message from @sshivanshg is obviously LLM-generated. I’m not sure how to go about that. I am not comfortable with the idea of an LLM making changes to consensus-critical code. On the other hand, if the code is correct just closing the PR seems inappropriate.
8. 
   pinheadmz commented at 11:24 am on October 27, 2025: member

   I commented on the PR with a Voight-Kampff test
9. 
   mzumsande commented at 6:44 pm on October 27, 2025: contributor

   Off-topic, but I’ve opened https://github.com/bitcoin-core/meta/issues/35 to discuss the general problem with issues.
10. 
    maflcko commented at 4:38 pm on October 28, 2025: member

    … LLM-generated. I’m not sure how to go about that. I am not comfortable with the idea of an LLM making changes to consensus-critical code. On the other hand, if the code is correct just closing the PR seems inappropriate.

    I looked at the PR, and while it appears “correct”, it is randomly only changing size_t to uint64_t in half the places, even if they are on adjacent lines (https://github.com/bitcoin/bitcoin/pull/33712/files#diff-a9bf14cec48b76a03a785de76b8bc71c5db17d494c7f02c29dfde265c3acaf17). So closing it was the right approach. A minimal patch for the already worked around CVE would be to just patch serialize.h (like https://github.com/bitcoin/bitcoin/pull/33712/files#diff-1c0f9772bbdf8bfc283393a67c305b997a8ab9738d48c160e07d2fa56500a7a0). However, it seems better to consistently use fixed size int types in the affected code area, so I did that instead in #33724, with the patch split up in reviewable commits.

Contributors
darosior mansiverma897993 sshivanshg pinheadmz mzumsande maflcko

Labels
Refactoring P2P Consensus