Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 25, 2023

HashWriter is a slim and less confusing version of CHashWriter, so use it in all places where it compiles.

This should be correct, if it compiles.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 25, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theuni, TheCharlatan, sipa

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:

  • #22563 (addrman: treat Tor/I2P/CJDNS as a single group by vasild)

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.

@DrahtBot DrahtBot changed the title refactor: Use HashWriter over legacy CHashWriter refactor: Use HashWriter over legacy CHashWriter Aug 25, 2023
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Concept ACK. Changes look good.

Why not get test/addrman_tests.cpp at the same time?

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

I don't know if I just missed it when I first looked or if you did some ninja force push, but addrman_tests.cpp is updated here so ignore my previous comment.

ACK 99995cf

Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK 99995cf

@sipa
Copy link
Member

sipa commented Aug 28, 2023

I'm not sure this improves much, unless we can actually get rid of CHashWriter entirely?

That said, code review ACK 99995cf

@maflcko
Copy link
Member Author

maflcko commented Aug 28, 2023

I'm not sure this improves much, unless we can actually get rid of CHashWriter entirely?

It is still used in CHashVerifier, so it can not be removed right now. But CHashVerifier is removed in #25284, so CHashWriter can be removed after that as well.

@fanquake fanquake merged commit 1c1a02b into bitcoin:master Aug 28, 2023
@maflcko maflcko deleted the 2308-remove-code- branch August 28, 2023 15:22
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
99995cf refactor: Use HashWriter over legacy CHashWriter (via SerializeHash) (MarcoFalke)
5555aa2 refactor: Use HashWriter over legacy CHashWriter (MarcoFalke)

Pull request description:

  `HashWriter` is a slim and less confusing version of `CHashWriter`, so use it in all places where it compiles.

  This should be correct, if it compiles.

ACKs for top commit:
  sipa:
    That said, code review ACK 99995cf
  theuni:
    ACK 99995cf
  TheCharlatan:
    ACK 99995cf

Tree-SHA512: fc967a18379bd00bd334ac3d50beb5435b65ca66a48f72623f1dcdbbce3292fd91839160cd0e69b8f4f3d98e258dcbbc6f73f5e91345f938898ee39c903a442b
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 16, 2024
…ible

Summary:
```
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for HashWriter. CHashWriter remains in places where it is not yet possible.
```

Backport of [[bitcoin/bitcoin#25331 | core#25331]] and [[bitcoin/bitcoin#28341 | core#28341]].

Test Plan:
  ninja all check-extended

Reviewers: #bitcoin_abc, roqqit, PiRK

Reviewed By: #bitcoin_abc, roqqit, PiRK

Subscribers: roqqit

Differential Revision: https://reviews.bitcoinabc.org/D16472
roqqit pushed a commit to doged-io/doged that referenced this pull request Aug 1, 2024
…ible

Summary:
```
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for HashWriter. CHashWriter remains in places where it is not yet possible.
```

Backport of [[bitcoin/bitcoin#25331 | core#25331]] and [[bitcoin/bitcoin#28341 | core#28341]].

Test Plan:
  ninja all check-extended

Reviewers: #bitcoin_abc, roqqit, PiRK

Reviewed By: #bitcoin_abc, roqqit, PiRK

Subscribers: roqqit

Differential Revision: https://reviews.bitcoinabc.org/D16472
@bitcoin bitcoin locked and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants