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:
#26859 (fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses by vasild)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#24034 (p2p: delete anchors.dat after trying to connect to that peers by brunoerg)
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 force-pushed
on Jun 11, 2022
maflcko force-pushed
on Jun 12, 2022
luke-jr
commented at 4:12 pm on June 18, 2022:
member
It seems confusing to have consensus (weakly) depend on low level BIP 155 addrv2 serialization.
You mean the “consensus” module, right, not actual consensus?
DrahtBot added the label
Needs rebase
on Jul 12, 2022
fanquake referenced this in commit
895937edb2
on Jul 20, 2022
maflcko force-pushed
on Jul 22, 2022
DrahtBot removed the label
Needs rebase
on Jul 22, 2022
fanquake referenced this in commit
510ac41eac
on Jul 22, 2022
maflcko force-pushed
on Jul 22, 2022
DrahtBot added the label
Needs rebase
on Jul 27, 2022
maflcko force-pushed
on Aug 1, 2022
DrahtBot removed the label
Needs rebase
on Aug 1, 2022
DrahtBot added the label
Needs rebase
on Sep 16, 2022
maflcko force-pushed
on Dec 15, 2022
DrahtBot removed the label
Needs rebase
on Dec 15, 2022
DrahtBot added the label
Needs rebase
on Jan 19, 2023
fanquake referenced this in commit
79e007d1d6
on Jan 26, 2023
maflcko force-pushed
on Jan 26, 2023
DrahtBot removed the label
Needs rebase
on Jan 26, 2023
maflcko force-pushed
on Jan 26, 2023
fanquake referenced this in commit
0a1d372ad0
on Jan 30, 2023
sidhujag referenced this in commit
789b476bab
on Jan 30, 2023
maflcko force-pushed
on Jan 31, 2023
maflcko renamed this:
[WIP] consensus: Remove dependency on net (BIP 155 / ADDRV2_FORMAT)
consensus: Remove dependency on ADDRV2_FORMAT
on Jan 31, 2023
maflcko force-pushed
on Jan 31, 2023
maflcko renamed this:
consensus: Remove dependency on ADDRV2_FORMAT
net: Use serialization parameters for CAddress serialization
on Jan 31, 2023
DrahtBot renamed this:
net: Use serialization parameters for CAddress serialization
net: Use serialization parameters for CAddress serialization
on Jan 31, 2023
maflcko marked this as ready for review
on Jan 31, 2023
DrahtBot added the label
Needs rebase
on Jan 31, 2023
maflcko force-pushed
on Feb 1, 2023
DrahtBot removed the label
Needs rebase
on Feb 1, 2023
achow101 requested review from ryanofsky
on Apr 25, 2023
DrahtBot added the label
Needs rebase
on May 30, 2023
maflcko force-pushed
on Jul 6, 2023
maflcko force-pushed
on Jul 6, 2023
DrahtBot added the label
CI failed
on Jul 6, 2023
maflcko force-pushed
on Jul 6, 2023
DrahtBot removed the label
Needs rebase
on Jul 6, 2023
maflcko force-pushed
on Jul 6, 2023
DrahtBot removed the label
CI failed
on Jul 6, 2023
DrahtBot added the label
Needs rebase
on Jul 13, 2023
maflcko force-pushed
on Jul 14, 2023
DrahtBot removed the label
Needs rebase
on Jul 14, 2023
jonatack
commented at 2:20 am on July 28, 2023:
contributor
Concept ACK, will have a look.
DrahtBot added the label
Needs rebase
on Aug 3, 2023
maflcko force-pushed
on Aug 4, 2023
maflcko force-pushed
on Aug 4, 2023
DrahtBot added the label
CI failed
on Aug 4, 2023
DrahtBot removed the label
Needs rebase
on Aug 4, 2023
in
src/serialize.h:137
in
6a99da9fa4outdated
133@@ -133,12 +134,29 @@ enum
134 SER_GETHASH = (1 << 2),
135 };
136137-//! Convert the reference base type to X, without changing constness or reference type.
138-template<typename X> X& ReadWriteAsHelper(X& x) { return x; }
139-template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
140+/* Convert any argument to a reference to X, maintaining constness.
0test/serialize_tests.cpp:281:22: error: use of undeclared identifier 'ParseUint32'; did you mean 'ParseUInt32'?
1 ok = ParseUint32(str, &data);
2 ^~~~~~~~~~~
3 ParseUInt32
4./util/strencodings.h:225:20: note: 'ParseUInt32' declared here
5[[nodiscard]] bool ParseUInt32(std::string_view str, uint32_t *out);
Good catch. The test was written by @vasild. I’ve removed the code, since there is already another unit test checking SERIALIZE_METHODS_PARAMS.
Also, I’ve replaced DEC by RAW to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core’s own util functions.
jonatack
commented at 7:26 pm on August 4, 2023:
contributor
Initial tested approach ACK6a99da9fa4a66e343df0910918da0ba9686d0fde. Quite a lot going on in these changes. The template changes in b03572d8381 are nicely simplified from the original in #19503.
DrahtBot removed the label
CI failed
on Aug 4, 2023
maflcko force-pushed
on Aug 9, 2023
maflcko force-pushed
on Aug 9, 2023
maflcko force-pushed
on Aug 9, 2023
in
src/serialize.h:156
in
667cf7e210outdated
154+ *
155+ * static_cast cannot easily be used here, as the type of Obj will be const Child&
156+ * during serialization and Child& during deserialization. AsBase will convert to
157+ * const Base& and Base& appropriately.
158+ */
159+template <typename X> X& AsBase(X& x) { return x; }
Would there be a reason to check std::is_base_of? IIUC the compiler already checks this internally on the argument when the function is called. And if the function call argument is a violating reinterpret_cast we can’t protect against this anyway inside this function or the function signature.
theuni
commented at 7:36 pm on August 23, 2023:
member
Edit: Most of the below was written with a misunderstanding of how these params actually work. I was trying (unsuccessfully) to shoe-horn params into the existing stream behavior. I’ll leave my clueless comments in case they help anyone else with the same misunderstanding. See this follow-up comment for a more reasonable take.
What is the future of s.GetType() & SER_GETHASH and s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS ?
I’m working on trying to switch #28327 to this approach, but I’m running into some issues.
CBlockLocator and CDiskBlockIndex both check for SER_GETHASH and conditionally skip some serializing if we’re hashing. So i’ve introduced a bool hashing in SerParams for those classes.
SER_NETWORK and GlobalParams::hashing::off are redundant
Now CTransaction fails to serialize because ParamsStream deletes GetVersion().
Finally.. that could be fixed by turning SERIALIZE_TRANSACTION_NO_WITNESS into a global param as well. But that seems like it puts us back where we started, with everything jumbled up in the same place.
So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.
theuni
commented at 8:05 pm on August 23, 2023:
member
In that case we’ll need to set some params for some types of V but not others. How to do that generically?
sipa
commented at 8:28 pm on August 23, 2023:
member
@theuni This is very generic, and I haven’t tried any of this, but roughly my thinking is the following.
For transactions (CTransaction, CMutableTransaction), I’d expect a TransactionParams with a boolean that indicates whether the witness is to be serialized or not (and whether it’s accepted on deserialization). I’d think we’d want that passed through when serializing blocks, so CBlock would also want a TransactionParams. But “higher” than that (e.g. in CNetMsgMaker or database logic) it would feel wrong to try to support having TransactionParams, because that code shouldn’t know or care about what blocks or transactions are. Instead, whenever there is code that tries to give (say) a transaction to a database writing, that code should instead serialize a WithParams(TransactionParams{.segwit=...}, tx) rather than a tx, because presumably that code knows whether witness serialization is supposed to be used. In a sense, my view is that the real problem is trying to even have a way to “globally” set witness or not - it should be confined to logic that knows what that means.
For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code.
in
src/serialize.h:1189
in
fa7d40795boutdated
1184+ * Return a wrapper around t that (de)serializes it with specified parameter params.
1185+ *
1186+ * See FORMATTER_METHODS_PARAMS for more information on serialization parameters.
1187+ */
1188+template <typename Params, typename T>
1189+static auto WithParams(const Params& params, T&& t)
EDIT: On second thoughts, it doesn’t seem very workable – inheriting CNetAddr options through its child classes and across vectors of CNetAddrs and such kinda sucks.
theuni
commented at 2:00 pm on August 25, 2023:
member
For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code.
@sipa Thanks for the explanation. After playing with this some more I realized that I had a pretty significant misunderstanding, having a hard time not still thinking in terms of setting options for the entire stream. I see what you mean now about setting the param at the appropriate level in the stack.
I took another stab at a POC for using params for CBlockLocator as you described, which I think turned out looking quite reasonable (ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.
I take that back. Having now implemented some of those quirks in the commit above, it interacts just fine.
maflcko
commented at 3:14 pm on August 25, 2023:
member
(ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the GetType definition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interface CHashWriter are not needed at all, see https://github.com/bitcoin/bitcoin/pull/28341
maflcko force-pushed
on Aug 25, 2023
maflcko
commented at 4:02 pm on August 25, 2023:
member
Added a commit to remove code
theuni
commented at 4:45 pm on August 25, 2023:
member
(ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the GetType definition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interface CHashWriter are not needed at all, see #28341
It’s worth pointing out that, as you mentioned, it’s hard to trace this path in the current code. But a nice feature of the approach in the PR is that the params are now enforced at compile time and closer to the callsite, so it’s trivial to see what’s being used.
So after converting CBLockLocator, compile fails for every serialization because params are missing. That’s fixed by switching CDataStream -> DataStream and handling the params. If none of the deleted CDataStream usages were SER_GETHASH, I think it’s safe to say we don’t need the hashing param at all?
And yes, from a quick check, that’s the case everywhere but CTransaction. So specifically, I propose that we nuke the hashing checks in CDiskBlockIndex/CBLockLocator/CKeyPool. Presumably the lack of usage means that a hash has never had meaning for those. There’s still the question about whether or not to use dummy nVersions though.
theuni approved
theuni
commented at 7:13 pm on August 25, 2023:
member
ACKfa452431db62f780abd3e4cb34ab9523214b9d52.
DrahtBot requested review from jonatack
on Aug 25, 2023
theuni
commented at 9:42 pm on August 26, 2023:
member
In this PR, the feature is only used for CAddress serialization, but I plan to also convert SERIALIZE_TRANSACTION_NO_WITNESS in a follow-up.
@sipa follow-up reminder in case this gets merged :p
Combined with #28341 and #25284 (comment), that would go a long way towards (,if not completely) eliminating the legacy flags.
SerParams a; is illegal due to const, so I guess you mean SerParams b{};. But that type of bug seems impossible to fix, because one can just write NoDefaultInitializer<Encoding> d{{}};, or SerParams b{{}};.
I was more thinking just as a way to avoid missing entries by accident, so writing {.addrv2=true} and forgetting that you should be specifying .disk=false as well. If you’re writing .disk=NoDefaultInitializaer<bool>{{}} then you’re still at least explicitly aware of the parameter…
It’s not tested against all forms of init/inheritance, so we’d likely need to fix it up a bit.
Also the max params are hard-coded because of an AST traversal quirk. That limit could be removed with a less strict match and fancier code in SerParamsCheck::check.
You construct V1 params a couple of times in net_processing as well, just thought having them already done might be easier? You’d be saying WithParams(CAddress::V1_DISK, info) rather than WithParams(ser_params, info) or whatever, so it’d be slightly clearer inline as well as maybe saving a redeclaration?
seems a bit nicer. It seems like there should be some way to have ::Serialize() call this with the parameters directly via some template magic, but I can’t see what it would be.
Yeah; with C++20 concepts I could get ::Serialize to call obj.Serialize(s, s.GetParams()) directly which seemed nicer, but with just C++17 I got myself too confused.
If you want to use designated initializers, a la {.addrv2=false, .disk=false}, then you can’t use inheritance here, but could instead add an operator CNetAddr::SerParams() const function for pretty much the same result.
Yeah, inheritance is a bit verbose, but I like that it assigns each parameter the exact class name it is used for. Maybe if you post your full diff, I can consider it?
Essentially ended up reinventing this PR from scratch trying to figure out a nicer way of doing things
Replace READWRITEAS macro with AsBase wrapping function
Co-authored-by: Pieter Wuille <pieter@wuille.net>
aaaa3fa947
Rename CSerAction* to Action*
This allows new code, added in the next commit, to conform to the coding
guideline: No C-prefix for class names.
fac42e9d35
Support for serialization parameters
The moved part can be reviewed with the git options
--ignore-all-space --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
(Modified by Marco Falke)
Co-authored-by: Pieter Wuille <pieter@wuille.net>
faec591d64
maflcko force-pushed
on Aug 28, 2023
maflcko
commented at 3:53 pm on August 28, 2023:
member
Rebased and changed a variable name. Should be trivial to re-ACK.
maflcko force-pushed
on Aug 28, 2023
maflcko
commented at 7:11 pm on August 28, 2023:
member
Reworked the main commit
in
src/net_processing.cpp:1419
in
fa0b988a15outdated
1413@@ -1414,9 +1414,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1414 uint64_t your_services{addr.nServices};
14151416 const bool tx_relay{!RejectIncomingTxs(pnode)};
1417+ const auto ser_params{CNetAddr::V1};
1418 m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
1419- your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
1420- my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
1421+ your_services, WithParams(ser_params, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
Yes, good point. It makes little sense to assign an existing short alias to a const auto alias when it is only used once or twice.
Fixed in all files in this commit.
theuni approved
theuni
commented at 8:41 pm on August 28, 2023:
member
Re-ACKfafca6b73310a4449918b95e3895ac434a2e096a
Didn’t re-review tests, only the main updates.
maflcko force-pushed
on Aug 29, 2023
in
src/serialize.h:1137
in
faec591d64outdated
1133@@ -1080,4 +1134,61 @@ size_t GetSerializeSizeMany(int nVersion, const T&... t)
1134 return sc.size();
1135 }
11361137+/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
TheCharlatan
commented at 11:32 am on September 1, 2023:
Where is this GetParams defined that the method here overrides? Does this refer to the underlying type instead?
maflcko
commented at 8:19 am on September 5, 2023:
Good question. I wonder if it makes sense to override the GetParams() function at all. Generally, no underlying stream type has a GetParams() method, so “overriding” really means “defines”.
And if the GetParams() method is truly overridden, it should be rare, and it could make more sense to first require the stream to reveal its underlying unwrapped stream, without the GetParams() method?
maflcko
commented at 8:22 am on September 5, 2023:
(My plan was to look into this after C++20, to avoid the risk of writing clunky C++17 code.)
ajtowns
commented at 1:48 pm on September 5, 2023:
I think you could do:
0#ifdef HAVE_CXX20
1template<typename Stream> 2concept ParamsStreamConcept =requires(Stream& s) { s.GetParams(); };
3template<typename Stream> 4concept PlainStreamConcept =!ParamsStreamConcept<Stream>;
5#else
6template<typename Stream> 7bool ParamsStreamConcept = true;
8template<typename Stream> 9bool PlainStreamConcept = true;
10#endif
1112/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */13template<typename Params, typename SubStream>14classParamsStream15 {
16static_assert(PlainStreamConcept<SubStream>, "avoid wrapping a ParamsStream in a ParamsStream");
17 SubStream& GetSubStream() { return m_substream; }
or similar if you wanted, which would catch this sort of case via the C++20 CI builds. At present the only lines that violate this rule are READWRITE(WithParams(ser_params, AsBase<CService>(obj))); in protocol.h and Derived::SERIALIZE_METHOD_PARAMS in serialize_tests.cpp; you need a GetSubStream getter to fix those up.
maflcko
commented at 3:05 pm on September 5, 2023:
This feels unnecessary. Addrman’s serialization controls how the addresses in it are serialized (only V1_DISK or V2_DISK should be used), so it shouldn’t be necessary to provide it with a CAddress parameter (and doing so is misleading, as it’d be overridden anyway).
ajtowns
commented at 11:59 am on September 4, 2023:
maflcko
commented at 7:54 am on September 5, 2023:
I agree this is misleading, but the unit and fuzz tests would still set Network, as opposed to Disk sometimes. I agree with the cleanup, but I think this is a slight change (improvement) in what the unit and fuzz tests are testing.
Though, as it only affects tests, I think the refactoring label can be kept on this pull request.
maflcko
commented at 8:20 am on September 5, 2023:
This line and the lines above could be simplified, as it’s only choosing between options V1_DISK and V2_DISK.
Use serialization parameters for CAddress serialization
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
fac81affb5
test: add tests that exercise WithParams()
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
fafa3fc5a6
Remove unused legacy CHashVerifierfa626af3ed
maflcko force-pushed
on Sep 5, 2023
maflcko
commented at 8:21 am on September 5, 2023:
member
TheCharlatan
commented at 2:53 pm on September 5, 2023:
contributor
ACKfa626af3edbe8d98b2de91dd71729ceef90389fb
DrahtBot requested review from theuni
on Sep 5, 2023
in
src/serialize.h:960
in
fac42e9d35outdated
959+ * Support for all macros providing or using the ser_action parameter of the SerializationOps method.
960 */
961-struct CSerActionSerialize
962-{
963+struct ActionSerialize {
964 constexpr bool ForRead() const { return false; }
ajtowns
commented at 5:29 pm on September 5, 2023:
These could be static constexpr bool ForRead() { return ...; }
Actually, ::SerRead etc could be static member functions of these classes rather than being the global namespace too:
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me