CFeeRate serialization has several issues:
Fix both by removing the methods.
Code review ACK fa26f9d8319b4d508f85731a35ae17524b0465bc. But I’m not sure I agree with rationale that serialization method is “confusing”. The implementation is:
0SERIALIZE_METHODS(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); }
It saves the object to the stream, and it loads the object from the stream. It doesn’t serve as protocol documentation, but that doesn’t matter because the serialization isn’t used in a public protocol or disk format. The serialization is used in #10102, so if you remove it here, I will need to add it back there are or replace it with a different serialization. This isn’t hard, but you can see why these “remove serialization” commits are not my favorite.
Maybe we can come up with a different strategy if the real goal here is to reduce fuzzing costs. Maybe some serialize methods could be marked internal-only. Maybe internal serialize methods could be fuzzed less heavily? We could disable streaming << >> operators for these and require them to be serialized with SerializeInternal/UnserializeInternal functions if there is concern about things being serialized accidentally.
I don’t know. I’m not too concerned about this PR, but if there will be more PRs like this one, this would be good to think about.
I don’t think there will be any/many similar changes. Though, I don’t see how #10102 is an argument against this change. There is also no serialize method for many other classes, like CAmount or WitnessUnknown. #10102 doesn’t solve this by adding the serialize methods, but by defining a capnp struct to use internally. Defining the capnp struct (internally to multiprocess) seems preferable to me than to mark the public serialize method “internal” in some way.
This isn’t hard, but you can see why these “remove serialization” commits are not my favorite.
The only other example I could find was #10102 (review), where I mentioned that the serialize method could be kept (if simplified).
I understand that this may mean more work for you, but I don’t think we should use effort as an excuse to keep stinky code.
Though, I don’t see how #10102 is an argument against this change.
I reviewed and acked this PR. I’m definitely not saying #10102 is an argument against this change. All I’m saying is if there will be more PRs like this, it would be good to know what exactly the goal is and make sure #10102 will not get in the way of that goal.
There is also no serialize method for many other classes, like
CAmountorWitnessUnknown. #10102 doesn’t solve this by adding the serialize methods, but by defining a capnp struct to use internally. Defining the capnp struct (internally to multiprocess) seems preferable to me than to mark the public serialize method “internal” in some way.
This preference is strange to me. These are just two different methods of “internal” serialization. I have been trying to get #10102 reviewed for years now, so my preference is to have less code in #10102 not more code. My preference is to keep existing code instead deleting it, rewriting it, and adding it back again. My preference is to use standard serialization code instead of using new serialization methods.
This isn’t hard, but you can see why these “remove serialization” commits are not my favorite.
The only other example I could find was #10102 (comment), where I mentioned that the serialize method could be kept (if simplified).
Right. And in that case, the existing serialization seemed broken so it did seem better remove. The serialization here seems perfectly simple and not broken, so I’m ask whether this serialization would be better not to remove. I am asking what problem removing serialization solves and whether different solution, like marking it internal and deprioritizing fuzzing that would solve the problem. I don’t know the answer to this question. And again, I don’t think this PR is a big deal. You can ignore these questions unless you think there will be 5 more PRs like this one.
I understand that this may mean more work for you
The work for me is trivial and not the issue. The issue is how much more custom serialization code has to be added #10102 making it more work to review. If there is one “remove serialization” PR. I don’t care. If there will be 5 more PR’s, let’s figure out what problem they solve and solve it some way that doesn’t blow up #10102.
but I don’t think we should use effort as an excuse to keep stinky code.
I’m literally just adding back the same serialization code in #10102 with less fuzz coverage, so I think I am missing something here.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
LGTM
ack https://github.com/bitcoin/bitcoin/pull/22962/commits/fa26f9d8319b4d508f85731a35ae17524b0465bc
P.S: @MarcoFalke I think your GH signature has some problem
The serialization is used in #10102
I don’t see the point of doing this if it’s going to be added back later.
To be fair, adding this back later in #10102 is just the easiest option, not the only option. #10102 requires some kind of serialize/unserialize functions for this class to be written. It could be stream serialization, it could be json serialization, it could be field-by-field capnp struct serialization. Using the standard steam serialization is just nice because it’s one line of code and works out of the box with no need for any CFeeRate code in src/ipc/capnp.
I’m literally just adding back the same serialization code in #10102 with less fuzz coverage, so I think I am missing something here.
I’d say that the CFeeRate fuzz target is improving the fuzz coverage of 10102 at most by epsilon. It would be better to write a proper fuzz target for multiprocess (that ideally also covers serialization).
Closing for now. Can be reopened after 10102
I’d say that the CFeeRate fuzz target is improving the fuzz coverage of 10102 at most by epsilon.
Agree with this. That is why I was suggesting marking serialization internal-only, so you could drop the fuzz target while I could still keep one-line serialization:
0UNSAFE_INTERNAL_SERIALIZE(CFeeRate, obj) { READWRITE(obj.nSatoshisPerK); }
It would be better to write a proper fuzz target for multiprocess (that ideally also covers serialization).
It should be pretty easy to add a multiprocess fuzz target I think, depending on what you want it to do. Opened issue #23015 to figure out what would be ideal.