Hard-code version number value for CBlockLocator and CDiskBlockIndex #28428
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Ping @sipa @MarcoFalke |
|
utACK c1aa0af |
1 similar comment
|
utACK c1aa0af |
c1aa0af to
e73d2a8
Compare
|
Resolved formatting nit. Reviewers can confirm locally that the diff from before amounts only to whitespace: |
|
Re-ACK e73d2a8 |
|
reACK e73d2a8 |
| @@ -167,7 +166,7 @@ class CDBIterator | |||
|
|
|||
| template<typename V> bool GetValue(V& value) { | |||
| try { | |||
| CDataStream ssValue{GetValueImpl(), SER_DISK, CLIENT_VERSION}; | |||
| DataStream ssValue{GetValueImpl()}; | |||
There was a problem hiding this comment.
Just saw that this did replace the CDataStream with a DataStream here in CDBIterator and CDBWrapper, but it is still using a CDataStream for CDBBatch. Should that be replaced too, as I've done here: TheCharlatan@95405b8?
There was a problem hiding this comment.
Yes, if it compiles, then it should be fine to replace.
There was a problem hiding this comment.
Yes please, thanks for catching this. IIRC I couldn't do this with my original approach of converting to SerParams, but forgot about it after switching. As @MarcoFalke says, this must be correct if it compiles.
I think it would have been easier to review if Concept ACK either way |
|
Fixed in #28451, I guess. |
… and CDiskBlockIndex e73d2a8 refactor: remove clientversion include from dbwrapper.h (Cory Fields) 4240a08 refactor: Use DataStream now that version/type are unused (Cory Fields) f15f790 Remove version/hashing options from CBlockLocator/CDiskBlockIndex (Cory Fields) Pull request description: This is also a much simpler replacement for bitcoin#28327. There are version fields in `CBlockLocator` and `CDiskBlockIndex` that have always been written but discarded when read. I intended to convert them to use SerParams as introduced by bitcoin#25284, which [ended up looking like this](theuni@3e3af45). However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them. If we ever need to assign meaning in the future, we can introduce `SerParams` as was done for `CAddress`. As for the dummy values chosen: `CDiskBlockIndex::DUMMY_VERSION` was easy as the highest ever client version, and I don't expect any objection there. `CBlockLocator::DUMMY_VERSION` is hard-coded to the higest _PROTOCOL_ version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher. While reviewing, I suggest looking at the throwaway `SerParams` commit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used. ACKs for top commit: TheCharlatan: Re-ACK e73d2a8 ajtowns: reACK e73d2a8 Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba
…client version in CKeyPool serialize fac29a0 Remove SER_GETHASH, hard-code client version in CKeyPool serialize (MarcoFalke) fa72f09 Remove CHashWriter type (MarcoFalke) fa4a9c0 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader (MarcoFalke) Pull request description: Removes a bunch of redundant, dead or duplicate code. Uses the idea from and finishes the idea bitcoin/bitcoin#28428 by theuni ACKs for top commit: ajtowns: ACK fac29a0 kevkevinpal: added one nit but otherwise ACK [fac29a0](bitcoin/bitcoin@fac29a0) Tree-SHA512: cc805e2f38e73869a6691fdb5da09fa48524506b87fc93f05d32c336ad3033425a2d7608e317decd3141fde3f084403b8de280396c0c39132336fe0f7510af9e
This is also a much simpler replacement for #28327.
There are version fields in
CBlockLocatorandCDiskBlockIndexthat have always been written but discarded when read.I intended to convert them to use SerParams as introduced by #25284, which ended up looking like this. However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them.
If we ever need to assign meaning in the future, we can introduce
SerParamsas was done forCAddress.As for the dummy values chosen:
CDiskBlockIndex::DUMMY_VERSIONwas easy as the highest ever client version, and I don't expect any objection there.CBlockLocator::DUMMY_VERSIONis hard-coded to the higest PROTOCOL version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher.While reviewing, I suggest looking at the throwaway
SerParamscommit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used.