bitcoin
refactor: move util/pcp and util/netif to common/
Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by fanquake in https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097.
Also make CI fail when the check-deps.sh script fails. Previously it would output errors but not cause the job to fail (which was not intended).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, laanwj, tdb3, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30915636360
Hints
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
-
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
utACK fd38711217cafbd62e8abd22d2b43f85fede8cde
I suppose the alternative would be to move netaddress.h to the util library.
The libraries.md document is a bit vague on what belongs to util vs common. It just says "low level". Since util is also used by libbitcoin_kernel it could make sense to move things out of it that the kernel doesn't need, but #28690 introduces a whole new kernel_util for that purpose.
utACK fd38711
I suppose the alternative would be to move
netaddress.hto the util library.
I think that might work, but I didn't look into it because I don't know if netaddress.h depends on other things in common that would require more dependencies to be moved. Also the pcp/netif files were added recently in #30043, while netaddress.h is older so I assume it is more likely netaddress is in the place where people want it to be.
The
libraries.mddocument is a bit vague on what belongs toutilvscommon. It just says "low level". Since util is also used bylibbitcoin_kernelit could make sense to move things out of it that the kernel doesn't need, but #28690 introduces a whole newkernel_utilfor that purpose.
Both are just collections of library functions shared by various executables. I think it util is a good place for things that are general purpose and can be broadly useful, and common is a better place for things that are hacky or more specific to the bitcoin core codebase. Also if something is used by kernel code it must go in util, not common.
I tend to think the current implementation of #28690 adding a kernel_util library just adds complexity and is not a good idea. I would prefer it if kernel just used the regular util library, and provided a few extra utilities outside developers could use or not use, instead of feeling the need to strip out everything that isn't used internally.
I think it util is a good place for things that are general purpose and can be broadly useful, and common is a better place for things that are hacky or more specific to the bitcoin core codebase.
The PCP code is not Bitcoin specific. Some things in netaddress.h are though, and tearing that file apart doesn't seem worth the effort for now.
Untested ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
The libraries.md document is a bit vague on what belongs to util vs common. It just says "low level"
Yes, i truly had no idea what to do here. Glad the CI catches it after this.
