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 (comment), 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 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 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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22951 (consensus: move amount.h into consensus by fanquake)

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.

The serialization is used in #10102

I don't see the point of doing this if it's going to be added back later.

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:

UNSAFE_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.