🤔 TheCharlatan reviewed a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2828221038)
lgtm, waiting for guix build.
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2828221038)
lgtm, waiting for guix build.
🚀 fanquake merged a pull request: "depends: Avoid using helper variables in toolchain file"
(https://github.com/bitcoin/bitcoin/pull/31360)
(https://github.com/bitcoin/bitcoin/pull/31360)
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081627012)
> since it's relied on by getblocktemplate in proposal mode
That was a mistake on my end, because `checkBlock` calls `connectBlock` which does the full check.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081627012)
> since it's relied on by getblocktemplate in proposal mode
That was a mistake on my end, because `checkBlock` calls `connectBlock` which does the full check.
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081628843)
Great catch! I found [this:](https://docs.python.org/3/library/socket.html#socket.timeout)
> `exception socket.timeout`
> A deprecated alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).
> Changed in version 3.10: This class was made an alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).
Since Python 3.10 is minimum version required in dependencies.md (and the test passes!) I think it's ok to leave as-is
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2081628843)
Great catch! I found [this:](https://docs.python.org/3/library/socket.html#socket.timeout)
> `exception socket.timeout`
> A deprecated alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).
> Changed in version 3.10: This class was made an alias of [TimeoutError](https://docs.python.org/3/library/exceptions.html#TimeoutError).
Since Python 3.10 is minimum version required in dependencies.md (and the test passes!) I think it's ok to leave as-is
💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2081633399)
I guess it could make sense to use a previous release where bdb wasn't yet deprecated, but not sure if this is worth it, and it could be a follow-up. Removed this commit for now, thanks.
(https://github.com/bitcoin/bitcoin/pull/32438#discussion_r2081633399)
I guess it could make sense to use a previous release where bdb wasn't yet deprecated, but not sure if this is worth it, and it could be a follow-up. Removed this commit for now, thanks.
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#issuecomment-2866480355)
> Maybe the three commits could be squashed?
Done! thanks for reviewing
(https://github.com/bitcoin/bitcoin/pull/32408#issuecomment-2866480355)
> Maybe the three commits could be squashed?
Done! thanks for reviewing
👍 theStack approved a pull request: "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows"
(https://github.com/bitcoin/bitcoin/pull/32419#pullrequestreview-2828263249)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
The introduced comments match my understanding of the deserialization of PSBT types and will hopefully help reviewing future code introduced/touched in this area. It's not ideal that the same comments are copied at three places, but don't have a better idea how to avoid this (still better to have duplicated documentation than no documentation).
(https://github.com/bitcoin/bitcoin/pull/32419#pullrequestreview-2828263249)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
The introduced comments match my understanding of the deserialization of PSBT types and will hopefully help reviewing future code introduced/touched in this area. It's not ideal that the same comments are copied at three places, but don't have a better idea how to avoid this (still better to have duplicated documentation than no documentation).
💬 TheCharlatan commented on pull request "guix: move `*-check.py` scripts under contrib/guix/":
(https://github.com/bitcoin/bitcoin/pull/32458#issuecomment-2866517322)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32458#issuecomment-2866517322)
Concept ACK
💬 Sjors commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866544280)
ACK fa061bfcdb0caea240fd15bcc309e7847132a4ff if CI is also happy
I didn't attempt a vcpkg build (fa5f3e62c8801cca80997cfb046c13983e0876e7), but I assume the Windows CI does that?
I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866544280)
ACK fa061bfcdb0caea240fd15bcc309e7847132a4ff if CI is also happy
I didn't attempt a vcpkg build (fa5f3e62c8801cca80997cfb046c13983e0876e7), but I assume the Windows CI does that?
I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
🤔 vasild reviewed a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2828006168)
Almost ACK 3acfc071c3445e943069b2778bbc5c74f702ef36
Only unsure about `The configured rpcuser or rpcpassword cannot contain a ":"`, see comment below.
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2828006168)
Almost ACK 3acfc071c3445e943069b2778bbc5c74f702ef36
Only unsure about `The configured rpcuser or rpcpassword cannot contain a ":"`, see comment below.
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081681184)
This looks like a new restriction. Could it introduce a breakage for users that have `-rpcpassword=contains:colon`?
It is sub-optimal to join/craft the string "user:pass" and to split/parse it a few lines later. Better store the user and pass in separate variables, like this:
<details>
<summary>[patch] use separate variables for user and pass</summary>
```diff
diff --git i/src/httprpc.cpp w/src/httprpc.cpp
index ed5a0253c6..493f1b0320 100644
--- i/src/httprpc.cpp
+++ w/src/httprpc.
...
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081681184)
This looks like a new restriction. Could it introduce a breakage for users that have `-rpcpassword=contains:colon`?
It is sub-optimal to join/craft the string "user:pass" and to split/parse it a few lines later. Better store the user and pass in separate variables, like this:
<details>
<summary>[patch] use separate variables for user and pass</summary>
```diff
diff --git i/src/httprpc.cpp w/src/httprpc.cpp
index ed5a0253c6..493f1b0320 100644
--- i/src/httprpc.cpp
+++ w/src/httprpc.
...
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081509904)
Maybe also worth adding the warning in the descriptions of `-rpcuser` and `-rpcpassword` in `init.cpp`:
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L668
and
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L664
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081509904)
Maybe also worth adding the warning in the descriptions of `-rpcuser` and `-rpcpassword` in `init.cpp`:
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L668
and
https://github.com/bitcoin/bitcoin/blob/5b8752198e979ea4987d8b6238f42f8ed9a38846/src/init.cpp#L664
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081503770)
It is like this in `master`, but the meaning of `share/rpcauth` may not be obvious. Especially considering that some distros may not install it in their packages. Is not installed by our `cmake --install BUILDDIR` either. Maybe change to "See share/rpcauth in the source directory for more information".
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081503770)
It is like this in `master`, but the meaning of `share/rpcauth` may not be obvious. Especially considering that some distros may not install it in their packages. Is not installed by our `cmake --install BUILDDIR` either. Maybe change to "See share/rpcauth in the source directory for more information".
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081550947)
nit:
```suggestion
std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
```
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081550947)
nit:
```suggestion
std::array<unsigned char, CHMAC_SHA256::OUTPUT_SIZE> out;
```
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081564262)
Just noting: the way this works (unchanged by this PR) is that it allows duplicate usernames with different passwords (e.g. `joe:123` and `joe:456`) and would allow login with either password (`123` or `456`).
(just a comment, feel free to resolve right away)
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081564262)
Just noting: the way this works (unchanged by this PR) is that it allows duplicate usernames with different passwords (e.g. `joe:123` and `joe:456`) and would allow login with either password (`123` or `456`).
(just a comment, feel free to resolve right away)
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081596938)
I did not test to confirm this, just read the code, but before this PR if `-rpcuser=joe -rpcpassword=abc:def` was configured, it would have been possible to login with user `joe:abc` and password `def`.
(just a comment, feel free to resolve right away)
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2081596938)
I did not test to confirm this, just read the code, but before this PR if `-rpcuser=joe -rpcpassword=abc:def` was configured, it would have been possible to login with user `joe:abc` and password `def`.
(just a comment, feel free to resolve right away)
💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866555065)
> I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
Yes, see https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2866555065)
> I didn't check for _additional_ things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.
Yes, see https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413
💬 sipa commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2866560897)
Unsure if this matters in your case, but the cache being warm or not impacts validation speed significantly. If you have a node that is running for a long time, and then stop and start it again, the speed will be slower for a while until the cache warms up again.
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2866560897)
Unsure if this matters in your case, but the cache being warm or not impacts validation speed significantly. If you have a node that is running for a long time, and then stop and start it again, the speed will be slower for a while until the cache warms up again.
💬 fanquake commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2081724770)
> They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49,
> @fanquake, can you help us out here, do you remember why these were added?
You can recreate the failures by checking out to that commit (fd30e9688e15fe6e0f8b64202a6e9c7d96333394), applying this diff (which includes the supression being done here):
```diff
diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_f
...
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2081724770)
> They were introduced in https://github.com/bitcoin/bitcoin/pull/28865/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248R45-R49,
> @fanquake, can you help us out here, do you remember why these were added?
You can recreate the failures by checking out to that commit (fd30e9688e15fe6e0f8b64202a6e9c7d96333394), applying this diff (which includes the supression being done here):
```diff
diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_f
...
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081728831)
Oh I see I misunderstood. For now I want LineReader to return new string objects because it reads directly from the HTTPClient's `m_recv_buffer` which is then erased. I know we can prevent that copy by being more clever with `m_recv_buffer` and I still plan on exploring that, maybe as a follow up? It's nice and simple now and there is a clean separation between the socket layer receiving data and the `HTTPRequest`
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2081728831)
Oh I see I misunderstood. For now I want LineReader to return new string objects because it reads directly from the HTTPClient's `m_recv_buffer` which is then erased. I know we can prevent that copy by being more clever with `m_recv_buffer` and I still plan on exploring that, maybe as a follow up? It's nice and simple now and there is a clean separation between the socket layer receiving data and the `HTTPRequest`