RFC: remove clientversion.h usage from kernel

theuni commented at 7:57 pm on August 22, 2023: member

RFC because this is rather hackish and I’d like to know if anyone has a better idea.

I’m working on trimming out headers that libbitcoinkernel should not export. To be clear: the problem is not what headers libbitcoinkernel itself uses, but what headers it requires users to use. Basically.. the headers used directly and indirectly by bitcoin-chainstate.cpp.

Topping the list of headers we definitely don’t want to require for users are bitcoin-config.h and clientversion.h. The former is somewhat complicated, but clientversion.h only comes from one place: dbwrapper.h.

This PR gets rid of that include by creating a new constructor that sets the version in the cpp file, working around the problem.

Another potential solution would be to create a MakeClientVersionCDataStream() function which just creates a stream and sets the version, and relies on the caller to do the rest.

Any better ideas? Otherwise I’ll undraft and ask for review.

kernel: add constructors for CDataStream that eliminate the need for CLIENT_VERSION

clientversion.h should not be required for users of the kernel. dbwrapper
is a deeply nested header that is unlikely to be eliminated from the kernel
any time soon. These constructors will allow the version to be set from headers
without the include.

253cb3b3b8

kernel: add clientversion include where necessary

Prepare for removing clientversion include from dbwrapper.h. Add it where it is
currently picked up indirectly.

5fdf9cf7ca

kernel: remove clientversion usage from the kernel

Use the new client_version_tag constructors instead.

Also fix the include order in streams.cpp

c65766a446

DrahtBot commented at 7:57 pm on August 22, 2023: contributor

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
Concept ACK	TheCharlatan, hebasto

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

Conflicts

No conflicts as of last run.

theuni commented at 8:00 pm on August 22, 2023: member

Ping @TheCharlatan @MarcoFalke @ryanofsky

sipa commented at 8:02 pm on August 22, 2023: member

Honestly, I think we should just get rid of the serialization types and version numbers. They’re only used in a few places, and mostly to pass some flags around, mostly unrelated to their original design.

Things like #25284 may allow us to do so.

TheCharlatan commented at 9:15 pm on August 22, 2023: contributor

Concept ACK

bitcoinfinancier approved

hebasto commented at 12:29 pm on August 23, 2023: member

Topping the list of headers we definitely don’t want to require for users are bitcoin-config.h and clientversion.h.

Concept ACK on that.

ajtowns commented at 12:34 pm on August 23, 2023: contributor

Sorry if this is naive, but did you consider just introducing a KERNEL_VERSION constant that only gets bumped when it results in meaningful changes (like PROTOCOL_VERSION for p2p)? But @sipa’s suggestion seems better if it’s feasible.

in src/dbwrapper.h:9 in c65766a446

5@@ -6,7 +6,6 @@
6 #define BITCOIN_DBWRAPPER_H
7 
8 #include <attributes.h>
9-#include <clientversion.h>

MarcoFalke commented at 12:44 pm on August 23, 2023:

I see you are removing this header, but I don’t see how this is enforced in the future or currently. For example, the header may still be included indirectly, or added back at any time?

theuni commented at 1:01 pm on August 23, 2023:

ACK. Will add some kind of enforcement once we decide on an approach. We’ll want some generic way of marking a header as unusable by the kernel.

Another approach might be to have a whitelist of valid kernel headers, copy them to a scratch-space, and build bitcoin-chainstate outside of our tree. That becomes easily doable once @TheCharlatan finishes removing bitcoin-chainstate’s boost dependency :)

theuni commented at 8:42 pm on August 29, 2023:

FWIW I hacked this up here, just to see how the list is looking: https://github.com/theuni/bitcoin/commit/60da4fa6190e3a85d094f97dc8e578aeabcd22bf#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R1002-R1096

theuni commented at 1:13 pm on August 23, 2023: member

Sorry if this is naive, but did you consider just introducing a KERNEL_VERSION constant that only gets bumped when it results in meaningful changes (like PROTOCOL_VERSION for p2p)? But @sipa’s suggestion seems better if it’s feasible. @ajtowns Thing is, KERNEL_VERSION would still have a purely internal meaning in this case (and one unrelated to the kernel version :), it would just not be tied to clientversion anymore. That approach would allow a client application to change the value in the header and potentially get some different serialization behavior in their app. Admittedly in this case, from what I can tell, the version is serialized but unused so that’s not a real problem.

The approach in this PR which hard-codes the values into the binary so that the downstream user can’t possibly affect behavior.

But yes, I’m totally up for dropping this and switching to serialization params if that helps with the dependency here.

theuni commented at 4:21 pm on August 23, 2023: member

Hmm, maybe there’s a much simpler solution…

With the offending CDataStreams turned into DataStreams, compile fails in 2 places because version is used:

https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/chain.h#L404-L419 https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/primitives/block.h#L128-L134

Both of those, as far as I can tell, are reading/writing nVersion as a dummy value. The version used by streams is inconsistent.. sometimes it’s CLIENT_VERSION, sometimes it’s PROTOCOL_VERSION…

Is there any reason to keep the GetVersion() calls for these two since we’re not using it at all? As far as I can tell it’d be safe to replace them with zeroes.

MarcoFalke commented at 8:06 pm on August 23, 2023: member

Is there any reason to keep the GetVersion() calls for these two since we’re not using it at all? As far as I can tell it’d be safe to replace them with zeroes.

Good find. Might be better to use the max value previously written, or just no value at all (if possible), than serializing a zero.

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

fanquake referenced this in commit 8e0d9796da on Sep 7, 2023

fanquake commented at 10:38 am on September 7, 2023: member

Note that #25284 has now been merged.

theuni commented at 12:48 pm on September 7, 2023: member

Note that #25284 has now been merged.

Great, thanks for the ping. Going to close this and rework after #25284 since the approach will be completely different now.

theuni closed this on Sep 7, 2023

RFC: remove clientversion.h usage from kernel #28327

Reviews

Conflicts