👍 hebasto approved a pull request: "util: remove unused cpp-subprocess options"
(https://github.com/bitcoin/bitcoin/pull/29865#pullrequestreview-2010965540)
ACK 0f847336428153042dbf226caf52ecd43188f22e.

I'm happy to re-ACK if the https://github.com/bitcoin/bitcoin/pull/29865#discussion_r1571141464 will be addressed.

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572049607)
it seems to me that `&& i + 2 < url_encoded.size()` part isn't checked by the tests, consider adding a few more corner cases, e.g. this is what I tested it with:
```C++
BOOST_AUTO_TEST_CASE(decode_malformed_test) {
BOOST_CHECK_EQUAL(urlDecode("%%xhello th+ere \xff"), "%%xhello th+ere \xff");

BOOST_CHECK_EQUAL(urlDecode("%"), "%");
BOOST_CHECK_EQUAL(urlDecode("%%"), "%%");
BOOST_CHECK_EQUAL(urlDecode("%%%"), "%%%");
BOOST_CHECK_EQUAL(urlDecode("%%%%"), "%%%%");


...

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572069107)
`ptr` seems unused, we might as well inline it to:
```C++
std::from_chars(&url_encoded[i + 1], &url_encoded[i + 3], decoded_value, 16).ec == std::errc{})
```
(note, I've used `std::errc{}` instead, seems we're using that more often in the codebase)

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572096517)
nit: it used to be a dependency of `urlDecode`, not `UrlDecode`, but I guess it's fine

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572070476)
we could unify these two `res += c` cases by merging the two conditions:
```C++
std::string UrlDecode(const std::string& url_encoded) {
std::string res;
res.reserve(url_encoded.size());

for (size_t i = 0; i < url_encoded.size(); ++i) {
int decoded_value = 0;
if (url_encoded[i] == '%' &&
i + 2 < url_encoded.size() &&
std::from_chars(&url_encoded[i + 1], &url_encoded[i + 3], decoded_value, 16).ec == std::errc{}) {
res += s
...

💬 paplorinc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572090699)
is this passing for the old implementation?
Isn't this a change in behavior?

💬 Sjors commented on pull request "[RFC] guix: remove bzip2 (and almost xz) from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2066199026)
> Something a bit more concrete. We could consolidate all packages in depends to .tar.gz (which is also what we use for releases), and drop `bzip2` and eventually `xz`.

Concept ACK. From your commits I gather that most stuff we use, provides `.tar.gz` at the same location we got the source from before.

Only QT and libxkbcommon require fetching code from Github. If they ever move away from Github to a platform that doesn't automatically generate `.tar.gz` files (Gitlab does), we could eithe
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#issuecomment-2066207955)
debugging the p2p_opportunistic_1p1c.py failure. I think the wallet one is unrelated.

💬 Satoshin-Btc commented on pull request "[RFC] guix: remove bzip2 (and almost xz) from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2066213899)
I think we as a community have proven to be resilient. However the time has
come to fix some things that we do. Such as using Bitcoin as a way to work
against one another. Don't destroy what was built over jealousy or anger.
Stop trying to manipulate the system for your own gains. Just a word of
advice

On Fri, Apr 19, 2024, 4:35 AM Sjors Provoost ***@***.***>
wrote:

> Something a bit more concrete. We could consolidate all packages in
> depends to .tar.gz (which is also what we use f
...

📝 hebasto opened a pull request: "Refactor `subprocess.hpp` to follow our conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR:
1. Replaces a custom `__USING_WINDOWS__` macro with the `WIN32`.
2. Renames the header `*.hpp` --> `*.h` and adjust the header guard name.

A draft for now as it is based on https://github.com/bitcoin/bitcoin/pull/29865 to avoid conflicts.

💬 fanquake commented on pull request "Refactor `subprocess.hpp` to follow our conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066228898)
Why not put the windows change with the windows change: https://github.com/bitcoin/bitcoin/pull/29868 ?
Looks like the other change could just be PR'd on it's not, and doesn't need to be based on anything?

👋 hebasto's pull request is ready for review: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)

💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066248047)
> Why not put the windows change with the windows change: #29868 ? Looks like the other change could just be PR'd on it's own, and doesn't need to be based on anything? Avoiding conflicts doesn't seem necessary given this branch already conflicts as-is?

Thanks! Reworked per your suggestions.

💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066251518)
Drafted again until resolving a linter warning.

📝 hebasto converted_to_draft a pull request: "refactor: Rename `subprocess.hpp` to follow our header name conventions"
(https://github.com/bitcoin/bitcoin/pull/29910)
This PR renames the header `*.hpp` --> `*.h` and adjusts the header guard name.

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1572143420)
fixed

💬 hebasto commented on pull request "refactor: Rename `subprocess.hpp` to follow our header name conventions":
(https://github.com/bitcoin/bitcoin/pull/29910#issuecomment-2066305987)
> Drafted again until resolving a linter warning. (The header is processed by linters now).

Linter warnings have been resolved. But the PR remains as a draft because the https://github.com/bitcoin/bitcoin/pull/29865 will make the diff smaller.

⚠️ kcalvinalvin opened an issue: "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does"
(https://github.com/bitcoin/bitcoin/issues/29911)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

It's stated in `chainparams.cpp` that "seed.bitcoinstats.com" supports filtering. However, this isn't true.

https://github.com/bitcoin/bitcoin/blob/c05c214f2e9cfd6070a3c7680bfa09358fd9d97a/src/kernel/chainparams.cpp#L137

### Expected behaviour

Since "seed.bitcoinstats.com" supports filtering for `NODE_NETWORK`, when asked for `NODE_NETWORK`, it should return archival nodes. It doesn't
...

🤔 glozow reviewed a pull request: "test: p2p: add test for rejected tx request logic (`m_recent_rejects` filter)"
(https://github.com/bitcoin/bitcoin/pull/29827#pullrequestreview-2011134310)
code review ACK 60ca5d55081275a011ccfc9546e0c4a8c4030493

💬 vasild commented on pull request "addrman: improve performance of `GetAddr` when specifying network":
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066352274)
The development of Bitcoin Core is decentralized because every developer decides for themselves what is important and what not.

Asserting one's opinion on others is more in line with centralized corporate management.