refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

pull maflcko wants to merge 5 commits into bitcoin:master from maflcko:2309-no-ser-hash- changing 38 files +69 −138
  1. maflcko commented at 4:07 PM on September 11, 2023: member

    Seems odd to have code that is completely dead.

    Fix this by removing all of it.

  2. DrahtBot commented at 4:07 PM on September 11, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky, sipa, ajtowns

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28929 (serialization: Support for multiple parameters by ryanofsky)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #28690 (build: Introduce internal kernel library by TheCharlatan)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. maflcko force-pushed on Sep 11, 2023
  4. theuni commented at 4:14 PM on September 11, 2023: member

    Much of this is done in #28438 I think. You and @ajtowns might want to work together on these :)

  5. DrahtBot added the label CI failed on Sep 11, 2023
  6. in src/hash.h:228 in 3333769c83 outdated 
     220 | @@ -223,15 +221,6 @@ class HashedSourceWriter : public HashWriter
 221 |      }
 222 |  };
 223 |  
 224 | -/** Compute the 256-bit hash of an object's serialization. */
 225 | -template<typename T>
 226 | -uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL_VERSION)
    ajtowns commented at 10:20 PM on September 11, 2023:

    Why get rid of this helper function? SerializeHash(obj) seems simpler to read than (HashWriter{} << obj).GetHash().

    maflcko commented at 9:52 AM on September 12, 2023:
    • It is only used in 4 places, everything else uses the "direct" way
    • Seems more confusing to have two ways to do the same thing, than just one way
    ajtowns commented at 10:55 AM on September 12, 2023:

    I guess most of the code is combining a few objects and hashing them, so I guess you're right...

    maflcko commented at 10:59 AM on September 12, 2023:

    Maybe a HashWriter{obj1, obj2, ...}.GetHash() alternative can be used?

    ajtowns commented at 12:42 PM on September 12, 2023: 
        template <typename... Ts>
    explicit HashWriter(const Ts&... obj)
    {
        (*this << ... << obj);
    }

    lets you replace

        uint64_t hash1 = (HashWriter{} << nKey << GetKey()).GetCheapHash();
    uint64_t hash2 = (HashWriter{} << nKey << netgroupman.GetGroup(*this) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetCheapHash();

    with

        uint64_t hash1 = HashWriter{nKey, GetKey()}.GetCheapHash();
    uint64_t hash2 = HashWriter{nKey, netgroupman.GetGroup(*this), (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)}.GetCheapHash();

    Not sure it's worth it though.

  7. maflcko force-pushed on Sep 12, 2023
  8. maflcko commented at 9:59 AM on September 12, 2023: member

    Much of this is done in #28438 I think. You and @ajtowns might want to work together on these :)

    I think the two pulls conflict, but they do different things. This one is removing dead code, the other is changing live code. Happy to have them combined or separate, or drop any or all commits.

    Each commit can be cherry-picked independently, and the CBufferedFile and CAutoFile one is required for other work anyway, so I guess I'll split that one out and leave this pull in draft for now.

  9. maflcko commented at 2:22 PM on September 12, 2023: member

    Split out in https://github.com/bitcoin/bitcoin/pull/28458

  10. fanquake referenced this in commit 8209e86eeb on Sep 14, 2023
  11. DrahtBot added the label Needs rebase on Sep 14, 2023
  12. maflcko force-pushed on Sep 14, 2023
  13. DrahtBot removed the label Needs rebase on Sep 14, 2023
  14. DrahtBot added the label Needs rebase on Sep 15, 2023
  15. maflcko force-pushed on Sep 18, 2023
  16. DrahtBot removed the label Needs rebase on Sep 18, 2023
  17. maflcko force-pushed on Sep 19, 2023
  18. DrahtBot added the label Needs rebase on Sep 20, 2023
  19. maflcko force-pushed on Oct 3, 2023
  20. DrahtBot removed the label Needs rebase on Oct 3, 2023
  21. DrahtBot added the label Needs rebase on Oct 31, 2023
  22. maflcko force-pushed on Nov 1, 2023
  23. maflcko renamed this:
    Remove unused SER_DISK, SER_NETWORK, SER_GETHASH
    refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH
    on Nov 1, 2023
  24. maflcko force-pushed on Nov 1, 2023
  25. DrahtBot removed the label Needs rebase on Nov 1, 2023
  26. DrahtBot removed the label CI failed on Nov 1, 2023
  27. DrahtBot added the label Refactoring on Nov 1, 2023
  28. DrahtBot added the label Needs rebase on Nov 14, 2023
  29. maflcko force-pushed on Nov 16, 2023
  30. DrahtBot removed the label Needs rebase on Nov 16, 2023
  31. in src/net.h:423 in 275480e7ff outdated 
     418 | @@ -419,7 +419,7 @@ class V1Transport final : public Transport
 419 |      size_t m_bytes_sent GUARDED_BY(m_send_mutex) {0};
 420 |  
 421 |  public:
 422 | -    V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noexcept;
 423 | +    V1Transport(const NodeId node_id, int nVersionIn) noexcept;
    maflcko commented at 2:20 PM on November 16, 2023:

    Probably doesn't make much sense to touch the same line of code twice. #28892 is an alternative that removes this in one go. So I'll leave this pull in draft for now.

  32. DrahtBot added the label Needs rebase on Nov 17, 2023
  33. maflcko force-pushed on Nov 17, 2023
  34. DrahtBot removed the label Needs rebase on Nov 17, 2023
  35. DrahtBot added the label Needs rebase on Nov 26, 2023
  36. maflcko force-pushed on Nov 27, 2023
  37. in src/qt/psbtoperationsdialog.cpp:131 in e5402dcf2b outdated 
     127 | @@ -128,14 +128,14 @@ void PSBTOperationsDialog::broadcastTransaction()
 128 |  }
 129 |  
 130 |  void PSBTOperationsDialog::copyToClipboard() {
 131 | -    CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);
 132 | +    CDataStream ssTx{PROTOCOL_VERSION};
    maflcko commented at 10:53 AM on November 27, 2023:

    Same here, might as well do the transition in one hop instead of two commits, see https://github.com/bitcoin/bitcoin/pull/28912

  38. DrahtBot removed the label Needs rebase on Nov 27, 2023
  39. DrahtBot added the label CI failed on Nov 27, 2023
  40. maflcko force-pushed on Nov 28, 2023
  41. maflcko force-pushed on Nov 28, 2023
  42. maflcko renamed this:
    refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH
    refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH, CDataStream
    on Nov 28, 2023
  43. maflcko renamed this:
    refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH, CDataStream
    refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream
    on Nov 28, 2023
  44. maflcko force-pushed on Nov 28, 2023
  45. maflcko force-pushed on Nov 28, 2023
  46. maflcko marked this as ready for review on Nov 28, 2023
  47. DrahtBot removed the label CI failed on Nov 29, 2023
  48. maflcko commented at 7:45 AM on November 29, 2023: member

    rebased and added a commit to rename version.h to node/protocol_version.h

  49. fuzz: Drop unused version from fuzz input format fa7eb4f5c3
  50. Remove unused CDataStream fae00fe9c2
  51. Remove unused SER_NETWORK, SER_DISK fa0ae22ff2
  52. Remove unused version.h include fa4fbd5816
  53. Rename version.h to node/protocol_version.h fa98a097a3
  54. maflcko force-pushed on Nov 30, 2023
  55. fanquake requested review from ajtowns on Nov 30, 2023
  56. fanquake requested review from ryanofsky on Nov 30, 2023
  57. ryanofsky approved
  58. ryanofsky commented at 12:48 PM on November 30, 2023: contributor

    Seems odd to not code review ACK fa98a097a30bc39f2424c0efd28a7979155faae6 (looks good)

  59. sipa commented at 1:15 PM on November 30, 2023: member

    utACK fa98a097a30bc39f2424c0efd28a7979155faae6

  60. ajtowns commented at 2:48 PM on November 30, 2023: contributor

    ACK fa98a097a30bc39f2424c0efd28a7979155faae6

  61. DrahtBot removed review request from ajtowns on Nov 30, 2023
  62. maflcko assigned ryanofsky on Nov 30, 2023
  63. ryanofsky merged this on Nov 30, 2023
  64. ryanofsky closed this on Nov 30, 2023
  65. maflcko deleted the branch on Nov 30, 2023
  66. theuni commented at 5:18 PM on November 30, 2023: member

    rebased and added a commit to rename version.h to node/protocol_version.h

    Nice, thanks, I was going to PR the same thing :)

  67. bitcoin locked this on Nov 29, 2024
Contributors
maflckoDrahtBottheuniajtownsryanofskysipa
Labels
Refactoring
Linked (view graph)
#28458 refactor: Remove unused GetType() from CBufferedFile and CAutoFile
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me