💬 tdb3 commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2065579070)
> crACK for [21d0e6c](https://github.com/bitcoin/bitcoin/commit/21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86). Changes lgtm. Will follow up with some testing within the next few days as time allows.

re ACK for 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

Retested the following:
- using "ipc:///tmp/mysocket" form in bitcoin.conf causes error and quit (as expected, since it's not unix:/)
- using "notvalid:///tmp/sock" in bitcoin.conf causes error and quit (as expected, since it's not unix:/)
-
...

💬 mzumsande commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#issuecomment-2065662650)
Looks like this has uncovered a bug in lnd, see https://twitter.com/roasbeef/status/1781086945986404559.

🤔 josibake reviewed a pull request: "rename bitcoin.conf in dist tarball"
(https://github.com/bitcoin/bitcoin/pull/29903#pullrequestreview-2010758724)
ACK https://github.com/bitcoin/bitcoin/pull/29903/commits/7ee8680cfd97d55b8dde8df5372fae5d9cef39fa

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#issuecomment-2066106153)
> a bug in btcd

I presume the bug was fixed in https://github.com/btcsuite/btcd/pull/2142

I wonder why this wasn't found earlier in btcd, because if signed integer overflow happened, it could have resulted in rejected transactions as well (or anything else, since it is undefined behavior).

A possible explanation could be that no one sent a large enough transaction, or set a large enough maxfeerate to trigger the UB, or maybe someone did run into it and then re-tried with different valu
...

🤔 Sjors reviewed a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2010862632)
ACK 68bf6c82fee917756b367db4e99a8f6bcb6476bf

Link to the code as of v2.2.12 which we use in depends:

* `evhttp_uridecode`: https://github.com/libevent/libevent/blob/release-2.1.12-stable/http.c#L3225-L3246
* `evhttp_decode_uri_internal`: https://github.com/libevent/libevent/blob/release-2.1.12-stable/http.c#L3169-L3205

💬 Sjors commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572043682)
Was this doing anything, or did we forget to remove it earlier?

💬 Sjors commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1572032575)
caa2040edc46291b3f032c64018eda2f4b31df39 nit: `auto [_, ec]`

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