💬 maflcko commented on issue "~/.bitcoin (which is a softlink to a separate vmware virtual drive) dir is now almost 1tb":
(https://github.com/bitcoin/bitcoin/issues/29909#issuecomment-2066122539)
Can you share the sizes of the largest folders?

I don't use `txindex` right now, but I could imagine that it requires several GB of space.

💬 maflcko commented on issue "test: SegFault in `ismine_tests` on SunOS / illumos":
(https://github.com/bitcoin/bitcoin/issues/29908#issuecomment-2066136578)
Do the functional tests pass? Does it also happen in valgrind? Does it also happen in ubsan, asan, or tsan? Does it also happen when using a depends `DEBUG=1` and `--enable-debug` build?

💬 maflcko commented on pull request "test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos":
(https://github.com/bitcoin/bitcoin/pull/29907#issuecomment-2066146100)
lgtm ACK 976e5d8f7b2bc77cb1443b8bf0f38cb07db70e9b

`int8_t` is also what real, non-test code should use.

💬 maflcko commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1572076435)
I'd say that a new field, named `node_warnings` would also be acceptable. There shouldn't be a need to keep the name in sync with the wallet warnings, as the wallet warnings are of a different topic than the node warnings.

But happy to re-ACK either approach.

👍 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