rpc: Add missing #include #15435

bd0dbe8763fc3029cf96531c9ccaba280b939445 introduced a dependency of rpc/util.h on RPCErrorCode, defined in rpc/protocol.h. The latter file is only included from rpc/util.cpp, though. This commit fixes the missing include, by moving the #include of rpc/protocol.h to rpc/util.h.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15427 (Add support for descriptors to utxoupdatepsbt by sipa)
• #15288 (Remove wallet -> node global function calls by ryanofsky)

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.

Why is this specific change needed if master compiles? I mean, there's probably a lot of other missing includes right? Have you used iwyu?

Why is this specific change needed if master compiles? I mean, there's probably a lot of other missing includes right? Have you used iwyu?

Master compiles because apparently rpc/util.h is only included together with rpc/protocol.h anyway, so it is fine in that situation. I personally noticed it because of tinkering with the code in a separate effort, where this is apparently not the case.

I think this is a clear bug with a straight-forward fix, so even if it is not strictly necessary to make master compile, it is good to apply.

Sorry but I don't see this as a bug fix.

Sorry but I don't see this as a bug fix.

Why not? It is clearly a coding mistake to rely on symbols from a header that is not included. This is even explicitly stated in the developer notes:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

That said, I agree that this is of course not a very important fix - but it is also trivial and straight-forward, so has hardly any costs either. But if you think it is not useful to merge simple patches like this, I'm happy to close the PR instead (I've fixed the issue for my own projects already, so I don't need it).

It would be nice to have a tool to check for those, otherwise it seems like a one-off fix

It would be nice to have a tool to check for those, otherwise it seems like a one-off fix

I agree, but I don't have such a tool. As mentioned above, I found this issue because it broke some code I'm working with (based on Bitcoin Core), and I thought it would be useful to fix this, once found, upstream.

That's my point, there's no bug or code to fix. If you submit a pull that needs this "fixed" then let's review it there.

There'd be iwyu, but it isn't a trivial process to set up: #15442

That's my point, there's no bug or code to fix. If you submit a pull that needs this "fixed" then let's review it there.

There is code to fix, since the current code is simply not correct and just compiles by accident. It also violates the style explicitly described in the developer notes as noted above.

But if you don't think it is worthwhile to push a fix like this, then let's just close the PR. But to be honest, I don't understand what the potential downside of a trivial change like this is (even if the benefits are limited as well from a practical point of view).

utACK 39e20fc54ff1b57d6e046fc1215e2c2f7f89a97e

Solves a problem for @domob1812: no need to fight this fix.

I mean this opens the flood-gates to a torrent of similar PRs, which doesn't really solve a problem with any known compiler, so mild NACK from me.

I mean this opens the flood-gates to a torrent of similar PRs, which doesn't really solve a problem with any known compiler, so mild NACK from me.

I understand if that's the general view. I agree that this PR has a very small (although positive) impact. Once found, I wanted to contribute this fix back upstream, and expected it to be trivially to review and quick to merge, thus I didn't see any downsides. But given how controversial this seems to be, it is obviously wasting more developer attention and time than I expected it to do.

If there is a general "bar" for not accepting too-small PRs (even if positive), perhaps that should be defined explicitly and stated somewhere in the developer notes?

As for this particular PR - I think it would be nice to merge it, as it is an improvement (although very small). But the real issue that led me to this on another project is obviously not relevant for Bitcoin's decisions, and already fixed. So I don't need it merged if you'd rather not set a precedent. Feel free to close in that case.

Yea as said I'm not strongly against this, and when this can be automatically checked by some tool I'd be in favor of doing that. It's just that doing this include by include, seems inefficient, so I'd prefer if you group multiple similar changes, that's all.

If there is a general "bar" for not accepting too-small PRs (even if positive), perhaps that should be defined explicitly and stated somewhere in the developer notes?

We do have this line in contributing.md:

Trivial pull requests or pull requests that refactor the code with no clear benefits may be immediately closed by the maintainers to reduce unnecessary workload on reviewing.

I mean "no clear benefits" can be arguable in this case. And if it helps you with whatever upstream project I'll utACK this for this time. But please don't make a habit out of it.

utACK 39e20fc54ff1b57d6e046fc1215e2c2f7f89a97e

utACK 39e20fc

Hopefully discussion in #15442 might result in something automated to keep on top of these changes.

Thanks everyone! I'm sorry for wasting everyone's time, I didn't intend that - thought it would just be quick to review and merge, as the change is trivial. I'll not submit things like that in the future.