kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers #28423

TheCharlatan · 2023-09-06T19:58:47Z

This removes the non-consensus critical protocol.h and netaddress.h headers from the kernel headers. With this patch, they are no longer required to include in order to use the libbitcoinkernel library. This also allows for the removal of the compat.h header from the kernel headers.

As an added future benefit it also reduces the number of of kernel headers that include the platform specific bitcoin-config.h.

For those interested, the currently required kernel headers can be inspected visually with the sourcetrail tool by looking at the required includes of bitcoin-chainstate.cpp.

This is part of the libbitcoinkernel project, namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.

DrahtBot · 2023-09-06T19:58:49Z

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	stickies-v, hebasto, ajtowns, MarcoFalke
Stale ACK	theuni

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:

#bitcoin-core/gui/754 (Add BIP324 "Transport" label to peer details by hebasto)
#bitcoin-core/gui/747 (Open fully encrypted wallets by achow101)
#28331 (BIP324 integration by sipa)
#28248 (p2p: peer connection bug fixes, logic and logging improvements by jonatack)
#28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
#28016 (p2p: gives seednode priority over dnsseed if both are provided by sr-gi)
#27260 (Enhanced error messages for invalid network prefix during address parsing. by russeree)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)

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.

src/messageheader.h

theuni · 2023-09-07T16:11:53Z

This removes the non-consensus critical protocol.h and netaddress.h headers from the kernel headers.

Very nice. Concept ACK.

TheCharlatan · 2023-09-07T21:18:50Z

Updated d4cc87f -> 978baaf (rmNetKernel_0 -> rmNetKernel_1, compare)

Addressed @ajtowns's comment, moving only the required typedef and constant to the kernel includes now.
Reverted the creation of messageheader.h. With the current patch I am not sure what utility this would still provide.

theuni

ACK 978baaf

I didn't mind the additional messageheader.h split before, but without it this way is fine too.

The GetDefaultPort()'s are awkward in their new home, but I don't have a better suggestion.

ajtowns

ACK 978baaf

src/netaddress.cpp

TheCharlatan · 2023-09-09T08:11:58Z

Updated 978baaf -> 197de6a (rmNetKernel_1 -> rmNetKernel_2, compare)

Addressed @ajtowns's comment, moving GetDefaultPort functions to CConnman instead.

ajtowns · 2023-09-10T05:21:53Z

ACK 197de6a

theuni

ACK 197de6a.

I would've preferred to see the Params change in CConnman split into a separate commit, as the implications of that aren't 100% clear to me (Mostly: can we trickle m_params down anywhere else now?).

But it looks correct and is an improvement on top of the original PR.

maflcko

lgtm, left a nit

src/kernel/messagestartmagic.h

maflcko · 2023-09-11T15:22:44Z

src/util/time.h

@@ -6,8 +6,6 @@
 #ifndef BITCOIN_UTIL_TIME_H
 #define BITCOIN_UTIL_TIME_H

-#include <compat/compat.h>


According to iwyu:

util/time.h should add these lines: #include <sys/time.h> // for timeval #include <ctime> // for time_t

Seems the only reason this compiles is because those are exported from chrono, which seems fragile to rely on, no?

Ah yes, nice catch. MillisToTimeval() is the problem here. Ideally that should be moved away as timeval isn't api safe.

Edit: Not the highest prio though, as it's a struct and the forward-declare is fine.

Ah nvm. std::time_t is part of the public interface of chrono, so the include isn't needed. And the timeval type is fwd-declared, so this seems like a bug in iwyu.

stickies-v

re-ACK 4ce0ead

src/protocol.h

stickies-v · 2023-09-12T16:13:49Z

src/serialize.h

@@ -203,6 +203,7 @@ template<typename Stream> inline void Serialize(Stream& s, int64_t a ) { ser_wri
 template<typename Stream> inline void Serialize(Stream& s, uint64_t a) { ser_writedata64(s, a); }
 template<typename Stream, int N> inline void Serialize(Stream& s, const char (&a)[N]) { s.write(MakeByteSpan(a)); }
 template<typename Stream, int N> inline void Serialize(Stream& s, const unsigned char (&a)[N]) { s.write(MakeByteSpan(a)); }
+template <typename Stream, typename B, std::size_t N> void Serialize(Stream& s, const std::array<B, N>& a) { (void)/* force byte-type */UCharCast(a.data()); s.write(MakeByteSpan(a)); }


nit: ser, that's a long line

Seemed awkward to format this line, but leave the others. I think I'll leave this as is.

Might be clearer as { s.write(AsBytes(UCharSpanCast(Span(a)))); } ? I think that has all the same restrictions as you get with the dummy UCharCast.

Sure, as long as you keep the comment. Otherwise I fear that someone will remove the UChar(Span)Cast because "it isn't needed" and "everything still compiles".

Yeah. Might be another place where C++20 concepts would be good -- concept BytesLike = requires (B& b) { UCharCast(b) } and template<typename Stream, BytesLike B, ..> or so.

Yes, should be possible, see #27927 (comment), but IIRC it also changes the compiler error message

src/wallet/db.cpp

TheCharlatan · 2023-09-12T20:31:12Z

Updated 4ce0ead -> d4170ca (rmNetKernel_3 -> rmNetKernel_4, compare)

Addressed @stickies-v's comment, but took another approach by reverting back to the original name. This should keep things consistent and churn low.
Addressed @stickies-v's comment, removing useless Span wrapping. I had the impression this was some minimal required sugar to get msvc to accept the type.

This is already possible for C-style arrays, so allow it for C++11 std::array as well.

The protocol.h file contains many non-consensus related definitions and should thus not be part of the libbitcoinkernel. This commit makes protocol.h no longer a required include for users of the libbitcoinkernel. This commit is part of the libbitcoinkernel project, namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel. Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>

This is done in preparation to the next commit, but has the nice effect of removing one further data structure relying on the global `Params()`.

Move functions requiring the netaddress.h include out of libbitcoinkernel source files. The netaddress.h file contains many non-consensus related definitions and should thus not be part of the libbitcoinkernel. This commit makes netaddress.h no longer a required include for users of the libbitcoinkernel. This commit is part of the libbitcoinkernel project, namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.

This commit makes compat.h no longer a required include for users of the libbitcoinkernel. Including compat.h imports a bunch of platform-specific definitions. This commit is part of the libbitcoinkernel project, namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.

TheCharlatan · 2023-09-12T20:57:04Z

Rebased d4170ca -> d506765 (rmNetKernel_4 -> rmNetKernel_5, compare)

Fixed silent merge conflict with BIP324 connection support #28196

stickies-v · 2023-09-13T00:41:30Z

re-ACK d506765

hebasto

ACK d506765.

hebasto · 2023-09-13T14:23:20Z

src/serialize.h

@@ -301,6 +302,7 @@ template<typename Stream> inline void Unserialize(Stream& s, int64_t& a ) { a =
 template<typename Stream> inline void Unserialize(Stream& s, uint64_t& a) { a = ser_readdata64(s); }
 template<typename Stream, int N> inline void Unserialize(Stream& s, char (&a)[N]) { s.read(MakeWritableByteSpan(a)); }
 template<typename Stream, int N> inline void Unserialize(Stream& s, unsigned char (&a)[N]) { s.read(MakeWritableByteSpan(a)); }
+template <typename Stream, typename B, std::size_t N> void Unserialize(Stream& s, std::array<B, N>& a) { (void)/* force byte-type */UCharCast(a.data()); s.read(MakeWritableByteSpan(a)); }


37e2b01

Add std::array objects to serialize_tests.cpp?

Would this be enough for you? Can otherwise do it in a follow-up.

diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp index 2f2bb6698c..28d276076e 100644 --- a/src/test/serialize_tests.cpp +++ b/src/test/serialize_tests.cpp @@ -87,2 +87,4 @@ BOOST_AUTO_TEST_CASE(sizes) BOOST_CHECK_EQUAL(GetSerializeSize(bool(0), 0), 1U); + BOOST_CHECK_EQUAL(GetSerializeSize(std::array<uint8_t, 1>{0}, 0), 1U); + BOOST_CHECK_EQUAL(GetSerializeSize(std::array<uint8_t, 2>{0, 0}, 0), 2U); }

Sorry, I had to be clear in my initial comment that it was a suggestion for a follow-up.

src/kernel/messagestartchars.h

ajtowns

utACK d506765

src/protocol.h

ajtowns · 2023-09-13T23:34:10Z

src/serialize.h

@@ -203,6 +203,7 @@ template<typename Stream> inline void Serialize(Stream& s, int64_t a ) { ser_wri
 template<typename Stream> inline void Serialize(Stream& s, uint64_t a) { ser_writedata64(s, a); }
 template<typename Stream, int N> inline void Serialize(Stream& s, const char (&a)[N]) { s.write(MakeByteSpan(a)); }
 template<typename Stream, int N> inline void Serialize(Stream& s, const unsigned char (&a)[N]) { s.write(MakeByteSpan(a)); }
+template <typename Stream, typename B, std::size_t N> void Serialize(Stream& s, const std::array<B, N>& a) { (void)/* force byte-type */UCharCast(a.data()); s.write(MakeByteSpan(a)); }


Might be clearer as { s.write(AsBytes(UCharSpanCast(Span(a)))); } ? I think that has all the same restrictions as you get with the dummy UCharCast.

maflcko

left some style nits, better to be left as-is for a follow-up

lgtm ACK d506765 🍛

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK d5067651991f3e6daf456ba13c7036ddc4545352 🍛
lStXnLnO5f5mgzKS9vTXRcRE1E6bMezm8Whu8+/ejq9XWm3C/d0zha2XRO2Zh+Qup7wt0W56hpPWA2AteHA/CQ==

maflcko · 2023-09-14T09:07:33Z

src/net.cpp

@@ -730,7 +730,7 @@ V1Transport::V1Transport(const NodeId node_id, int nTypeIn, int nVersionIn) noex
    m_node_id(node_id), hdrbuf(nTypeIn, nVersionIn), vRecv(nTypeIn, nVersionIn)
 {
    assert(std::size(Params().MessageStart()) == std::size(m_magic_bytes));
-    std::copy(std::begin(Params().MessageStart()), std::end(Params().MessageStart()), m_magic_bytes);
+    m_magic_bytes = Params().MessageStart();


nit in 9be330b: Not sure if it compiles, but could use an constructor initializer for this?

maflcko · 2023-09-14T09:09:08Z

src/protocol.cpp

@@ -92,7 +92,7 @@ const static std::vector<std::string> g_all_net_message_types{

 CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)
 {
-    memcpy(pchMessageStart, pchMessageStartIn, MESSAGE_START_SIZE);
+    pchMessageStart = pchMessageStartIn;


This would allow to make it const, I guess. Which makes sense to do, because it is public. But that seems unrelated and better put into a follow-up.

maflcko · 2023-09-14T09:22:06Z

src/kernel/chainparams.h

 #include <consensus/params.h>
+#include <kernel/messagestartchars.h>


iwyu nit in 534b314: Can probably export those two and chaintype.h ?

This would allow dropping the include from stuff like blockstorage.cpp and validation.cpp again, because that already includes chainparams.

fanquake · 2023-09-14T09:44:16Z

Going to merge this shortly. The 324 integration PR will have to be rebased, but it looks like that will be pushed to again in any case (to address comments). The comments left here can be addressed in a followup PR.

TheCharlatan · 2023-09-14T19:56:56Z

Added the following comments to the kernel project tracking issue TODOs:
#28423 (comment)
#28423 (comment)
#28423 (comment)
#28423 (comment)
#28423 (comment)

…from kernel headers d506765 [refactor] Remove compat.h from kernel headers (TheCharlatan) 36193af [refactor] Remove netaddress.h from kernel headers (TheCharlatan) 2b08c55 [refactor] Add CChainParams member to CConnman (TheCharlatan) f0d1d8b [refactor] Add missing includes for next commit (TheCharlatan) 534b314 kernel: Move MessageStartChars to its own file (TheCharlatan) 9be330b [refactor] Define MessageStartChars as std::array (TheCharlatan) 37e2b01 [refactor] Allow std::array<std::byte, N> in serialize.h (MarcoFalke) Pull request description: This removes the non-consensus critical `protocol.h` and `netaddress.h` headers from the kernel headers. With this patch, they are no longer required to include in order to use the libbitcoinkernel library. This also allows for the removal of the `compat.h` header from the kernel headers. As an added future benefit it also reduces the number of of kernel headers that include the platform specific `bitcoin-config.h`. For those interested, the currently required kernel headers can be inspected visually with the [sourcetrail](https://github.com/CoatiSoftware/Sourcetrail) tool by looking at the required includes of `bitcoin-chainstate.cpp`. --- This is part of the [libbitcoinkernel project](bitcoin#27587), namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel. ACKs for top commit: stickies-v: re-ACK d506765 hebasto: ACK d506765. ajtowns: utACK d506765 MarcoFalke: lgtm ACK d506765 🍛 Tree-SHA512: 6f90ea510a302c2927e84d16900e89997c39b8ff3ce9d4effeb8a134bd29cc52bd9e81e51aaa11f7496bad00025b78a58b88c5a9e0bb3f4ebbe9a76309215fb7

1e5b861 test: Add test for array serialization (TheCharlatan) d49d198 refactor: Initialize magic bytes in constructor initializer (TheCharlatan) Pull request description: This is a followup-PR for #28423 * Initialize magic bytes in constructor * Add a small unit test for serializing arrays. ACKs for top commit: sipa: utACK 1e5b861 maflcko: lgtm ACK 1e5b861 Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7

DrahtBot added the Validation label Sep 6, 2023

TheCharlatan marked this pull request as ready for review September 6, 2023 20:24

ajtowns reviewed Sep 6, 2023

View reviewed changes

src/messageheader.h Outdated Show resolved Hide resolved

This was referenced Sep 7, 2023

BIP324 integration #28331

Merged

p2p: peer connection bug fixes #28248

Closed

Enhanced error messages for invalid network prefix during address parsing. #27260

Open

TheCharlatan force-pushed the rmNetKernel branch from 8e2f988 to 9861c7a Compare September 7, 2023 05:20

bitcoin deleted a comment from BlazerX98 Sep 7, 2023

TheCharlatan force-pushed the rmNetKernel branch from 8556f1f to d4cc87f Compare September 7, 2023 07:44

DrahtBot added CI failed and removed CI failed labels Sep 7, 2023

TheCharlatan changed the title ~~kernel: Remove protocol.h/netaddress.h from kernel headers~~ kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers Sep 7, 2023

TheCharlatan force-pushed the rmNetKernel branch from d4cc87f to 5bebabd Compare September 7, 2023 21:18

TheCharlatan force-pushed the rmNetKernel branch from 5bebabd to 978baaf Compare September 7, 2023 21:22

theuni approved these changes Sep 8, 2023

View reviewed changes

ajtowns reviewed Sep 9, 2023

View reviewed changes

src/netaddress.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the rmNetKernel branch from 978baaf to 197de6a Compare September 9, 2023 07:59

This was referenced Sep 9, 2023

p2p: gives seednode priority over dnsseed if both are provided #28016

Merged

net: Make AddrFetch connections to fixed seeds #26114

Open

DrahtBot requested a review from theuni September 10, 2023 05:21

DrahtBot mentioned this pull request Sep 10, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

theuni approved these changes Sep 11, 2023

View reviewed changes

maflcko reviewed Sep 11, 2023

View reviewed changes

src/kernel/messagestartmagic.h Outdated Show resolved Hide resolved

maflcko reviewed Sep 11, 2023

View reviewed changes

DrahtBot mentioned this pull request Sep 11, 2023

Add BIP324-specific labels to peer details bitcoin-core/gui#754

Merged

TheCharlatan force-pushed the rmNetKernel branch from ba37340 to 4ce0ead Compare September 12, 2023 14:42

DrahtBot requested review from theuni and ajtowns September 12, 2023 16:43

stickies-v approved these changes Sep 12, 2023

View reviewed changes

This was referenced Sep 12, 2023

Open fully encrypted wallets bitcoin-core/gui#747

Closed

wallet: Allow users to create a wallet that encrypts all database records #28142

Closed

TheCharlatan force-pushed the rmNetKernel branch from 4ce0ead to d4170ca Compare September 12, 2023 20:31

[refactor] Allow std::array<std::byte, N> in serialize.h

37e2b01

This is already possible for C-style arrays, so allow it for C++11 std::array as well.

DrahtBot added the CI failed label Sep 12, 2023

TheCharlatan and others added 6 commits September 12, 2023 22:49

[refactor] Define MessageStartChars as std::array

9be330b

[refactor] Add missing includes for next commit

f0d1d8b

[refactor] Add CChainParams member to CConnman

2b08c55

This is done in preparation to the next commit, but has the nice effect of removing one further data structure relying on the global `Params()`.

TheCharlatan force-pushed the rmNetKernel branch from d4170ca to d506765 Compare September 12, 2023 20:56

DrahtBot removed the CI failed label Sep 12, 2023

hebasto approved these changes Sep 13, 2023

View reviewed changes

ajtowns reviewed Sep 13, 2023

View reviewed changes

maflcko approved these changes Sep 14, 2023

View reviewed changes

fanquake merged commit 1e9d367 into bitcoin:master Sep 14, 2023
15 checks passed

TheCharlatan mentioned this pull request Sep 14, 2023

Libbitcoinkernel Project Tracking #27587

Open

27 tasks

TheCharlatan mentioned this pull request Nov 13, 2023

test, refactor: Magic bytes array followup #28857

Merged

		#include <consensus/params.h>
		#include <kernel/messagestartchars.h>

kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers #28423

kernel: Remove protocol.h/netaddress.h/compat.h from kernel headers #28423

Conversation

TheCharlatan commented Sep 6, 2023 • edited

DrahtBot commented Sep 6, 2023 • edited

Reviews

Conflicts

theuni commented Sep 7, 2023

TheCharlatan commented Sep 7, 2023 • edited

theuni left a comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

TheCharlatan commented Sep 9, 2023

ajtowns commented Sep 10, 2023

theuni left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni Sep 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCharlatan commented Sep 12, 2023

TheCharlatan commented Sep 12, 2023

stickies-v commented Sep 13, 2023

hebasto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajtowns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanquake commented Sep 14, 2023

TheCharlatan commented Sep 14, 2023

TheCharlatan commented Sep 6, 2023 •

edited

DrahtBot commented Sep 6, 2023 •

edited

TheCharlatan commented Sep 7, 2023 •

edited

theuni Sep 11, 2023 •

edited

maflcko Sep 14, 2023 •

edited