Skip to content

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 30, 2023
Merged

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

merged 5 commits into from
Nov 30, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 11, 2023

Seems odd to have code that is completely dead.

Fix this by removing all of it.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 11, 2023 •
edited
Loading

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 maflcko mentioned this pull request Sep 11, 2023
Merged
@theuni
Copy link
Member

theuni commented Sep 11, 2023

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

This was referenced Sep 11, 2023
Merged
Merged
Merged
Merged
Merged
ajtowns
ajtowns reviewed Sep 11, 2023
View reviewed changes
This was referenced Sep 11, 2023
Draft
Open
@maflcko
Copy link
Member Author

maflcko commented Sep 12, 2023

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 maflcko mentioned this pull request Sep 12, 2023
Merged
@DrahtBot DrahtBot mentioned this pull request Sep 12, 2023
Merged
@maflcko
Copy link
Member Author

maflcko commented Sep 12, 2023

Split out in #28458

This was referenced Sep 13, 2023
Merged
Merged
fanquake added a commit to bitcoin-core/gui that referenced this pull request Sep 14, 2023
@fanquake
Merge bitcoin/bitcoin#28458: refactor: Remove unused GetType() from C…
8209e86 
…BufferedFile and CAutoFile

fa19c91 scripted-diff: Rename CBufferedFile to BufferedFile (MarcoFalke)
fa2f241 Remove unused GetType() from CBufferedFile and CAutoFile (MarcoFalke)
5c2b3cd dbwrapper: Use DataStream for batch operations (TheCharlatan)

Pull request description:

  This refactor is required for bitcoin/bitcoin#28052 and bitcoin/bitcoin#28451

  Thus, split it out.

ACKs for top commit:
  ajtowns:
    utACK fa19c91
  TheCharlatan:
    ACK fa19c91

Tree-SHA512: d9c232324702512e45fd73ec3e3170f1e8a8c8f9c49cb613a6b693a9f83358914155527ace2517a2cd626a0cedcada56ef70a2a7812edafb1888fd6765eebba2
@DrahtBot DrahtBot mentioned this pull request Sep 18, 2023
Merged
@DrahtBot DrahtBot mentioned this pull request Sep 19, 2023
Merged
@maflcko maflcko force-pushed the 2309-no-ser-hash- branch 2 times, most recently from 228df16 to 94a078c Compare November 28, 2023 15:07
@DrahtBot DrahtBot mentioned this pull request Nov 28, 2023
Open
@maflcko maflcko changed the title refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH, CDataStream Nov 28, 2023
@maflcko maflcko changed the title refactor: Remove unused SER_DISK, SER_NETWORK, SER_GETHASH, CDataStream refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream Nov 28, 2023
@DrahtBot DrahtBot mentioned this pull request Nov 28, 2023
Closed
@maflcko maflcko marked this pull request as ready for review November 28, 2023 17:54
This was referenced Nov 28, 2023
Closed
Open
@maflcko
Copy link
Member Author

maflcko commented Nov 29, 2023

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

ryanofsky
ryanofsky approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sipa
Copy link
Member

sipa commented Nov 30, 2023

utACK fa98a09

@ajtowns
Copy link
Contributor

ajtowns commented Nov 30, 2023

ACK fa98a09

@DrahtBot DrahtBot removed the request for review from ajtowns November 30, 2023 14:49
@ryanofsky ryanofsky merged commit ffb0216 into bitcoin:master Nov 30, 2023
@maflcko maflcko deleted the 2309-no-ser-hash- branch November 30, 2023 16:31
@theuni
Copy link
Member

theuni commented Nov 30, 2023

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

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

@bitcoin bitcoin locked and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@ajtowns ajtowns ajtowns left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@ryanofsky ryanofsky

Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maflcko @DrahtBot @theuni @sipa @ajtowns @ryanofsky