refactor: Remove unused SER_DISK, SER

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.

DrahtBot commented at 4:07 pm on September 11, 2023: contributor

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

Code Coverage

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

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.

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.

maflcko force-pushed on Sep 11, 2023

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 :)

DrahtBot added the label CI failed on Sep 11, 2023

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:

0    template <typename... Ts>
1    explicit HashWriter(const Ts&... obj)
2    {
3        (*this << ... << obj);
4    }

lets you replace

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

with

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

Not sure it’s worth it though.

maflcko force-pushed on Sep 12, 2023

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.

maflcko commented at 2:22 pm on September 12, 2023: member

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

fanquake referenced this in commit 8209e86eeb on Sep 14, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

maflcko force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

DrahtBot added the label Needs rebase on Sep 15, 2023

maflcko force-pushed on Sep 18, 2023

DrahtBot removed the label Needs rebase on Sep 18, 2023

maflcko force-pushed on Sep 19, 2023

DrahtBot added the label Needs rebase on Sep 20, 2023

maflcko force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

DrahtBot added the label Needs rebase on Oct 31, 2023

maflcko force-pushed on Nov 1, 2023

maflcko renamed this:
~~Remove unused SER_DISK, SER_NETWORK, SER_GETHASH~~
refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH
on Nov 1, 2023

maflcko force-pushed on Nov 1, 2023

DrahtBot removed the label Needs rebase on Nov 1, 2023

DrahtBot removed the label CI failed on Nov 1, 2023

DrahtBot added the label Refactoring on Nov 1, 2023

DrahtBot added the label Needs rebase on Nov 14, 2023

maflcko force-pushed on Nov 16, 2023

DrahtBot removed the label Needs rebase on Nov 16, 2023

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.

DrahtBot added the label Needs rebase on Nov 17, 2023

maflcko force-pushed on Nov 17, 2023

DrahtBot removed the label Needs rebase on Nov 17, 2023

DrahtBot added the label Needs rebase on Nov 26, 2023

maflcko force-pushed on Nov 27, 2023

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

DrahtBot removed the label Needs rebase on Nov 27, 2023

DrahtBot added the label CI failed on Nov 27, 2023

maflcko force-pushed on Nov 28, 2023

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

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

maflcko force-pushed on Nov 28, 2023

maflcko marked this as ready for review on Nov 28, 2023

DrahtBot removed the label CI failed on Nov 29, 2023

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

fuzz: Drop unused version from fuzz input format fa7eb4f5c3

Remove unused CDataStream fae00fe9c2

Remove unused SER_NETWORK, SER_DISK fa0ae22ff2

Remove unused version.h include fa4fbd5816

Rename version.h to node/protocol_version.h fa98a097a3

maflcko force-pushed on Nov 30, 2023

fanquake requested review from ajtowns on Nov 30, 2023

fanquake requested review from ryanofsky on Nov 30, 2023

ryanofsky approved

ryanofsky commented at 12:48 pm on November 30, 2023: contributor

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

sipa commented at 1:15 pm on November 30, 2023: member

utACK fa98a097a30bc39f2424c0efd28a7979155faae6

ajtowns commented at 2:48 pm on November 30, 2023: contributor

ACK fa98a097a30bc39f2424c0efd28a7979155faae6

DrahtBot removed review request from ajtowns on Nov 30, 2023

maflcko assigned ryanofsky on Nov 30, 2023

ryanofsky merged this on Nov 30, 2023

ryanofsky closed this on Nov 30, 2023

maflcko deleted the branch on Nov 30, 2023

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 :)

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

Code Coverage

Reviews

Conflicts