💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r1829944320)
Good catch! Fixed.

📝 secp512k2 opened a pull request: "doc: Fix grammatical errors in multisig-tutorial.md"
(https://github.com/bitcoin/bitcoin/pull/31225)
This pull request fixes grammatical errors in the 'multisig-tutorial.md' document.

Corrections:

1. Incorrect Phrase "As can been seen":

- Before:

There are discussions about eliminating this redundancy, as can been seen in the issue #17190 (https://github.com/bitcoin/bitcoin/issues/17190).

- After:

There are discussions about eliminating this redundancy, as can be seen in the issue #17190 (https://github.com/bitcoin/bitcoin/issues/17190).

2. Clarity Improveme
...

💬 hebasto commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2458093729)
My Guix build:
```
aarch64
67b7e95e0f1d45debad8a8cac4c2d349e3e01efcf62e0b109345896f296758d9 guix-build-788c1324f3d8/output/aarch64-linux-gnu/SHA256SUMS.part
cd3c67eaa5f6fadf0a73cdb1aa03563d720fc7fdae1b59aa495a0eda9d2695af guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu-debug.tar.gz
5c1eb4ff827336a9d113497459981e213844b87e232c33fc00317ce20302f761 guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu.tar.gz
dd78923f
...

👍 hebasto approved a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216#pullrequestreview-2416632014)
ACK 97235c446e9986ecca09c2a4b78d6c6239853fdb, verified by updating the secp256k1 subtree locally.

💬 murchandamus commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2458206441)
It looks like all three dependencies got merged, is this ready for review?

💬 achow101 commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2458297336)
> It looks like all three dependencies got merged, is this ready for review?

Currently it is still dependent on #30328 but I suppose it doesn't have to be.

🤔 tdb3 reviewed a pull request: "net, init: derive default onion port if a user specified a -port"
(https://github.com/bitcoin/bitcoin/pull/31223#pullrequestreview-2417053413)
Approach ACK

Seemed to work nicely with two node instances run as follows:
```
build/src/bitcoind -port=11000 -rpcport=11100 -datadir=/mnt/tmp/n1/
```
```
build/src/bitcoind -port=12000 -rpcport=12100 -datadir=/mnt/tmp/n2/
```

```
ss -tulpn
...

... 127.0.0.1:11100 0.0.0.0:* users:(("bitcoind",pid=192480,fd=12))
... 127.0.0.1:11001 0.0.0.0:* users:(("bitcoind",pid=192480,fd=29))
... 127.0.0.1:12100
...

💬 tdb3 commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1830257430)
`std::optional::value_or()` might work nicely here.

e.g.
```diff
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index d56216a7cb0..b518cf1bb93 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -715,5 +715,5 @@ CService DefaultOnionServiceTarget(std::optional<uint16_t> port)
{
struct in_addr onion_service_target;
onion_service_target.s_addr = htonl(INADDR_LOOPBACK);
- return {onion_service_target, port.has_value() ? port.value() : BaseParams().OnionServ
...

💬 tdb3 commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2458574314)
> Not sure about this. This seems to be adding coverage for code that shouldn't exist in the first place.
>
> The integer is passed to `COutPoint`, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.

Good catch. It seems a bit odd that the RPC is trying to interpret the `vout` arg as a *signed* int in the first place (`const int nOutput = o.find_value("vout").getInt<int>();`), then check for ne
...

💬 vasild commented on pull request "Fee Estimation via Fee rate Forecasters":
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2458703251)
Concept ACK

> Allow node users to be self-sovereign by using their node's estimates, reducing reliance on third-party fee estimations.

> having users feel the need to use external fee estimation services that they could equally have served to them by their own node, feels sub-optimal

Indeed, I do that :face_with_head_bandage:

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2458962465)
@stickies-v i think you might be looking at the wrong commit. The hash you acked is outdated.

📝 hebasto opened a pull request: "cmake: Fix `IF_CHECK_PASSED` option handling"
(https://github.com/bitcoin/bitcoin/pull/31231)
`IF_CHECK_PASSED` is a multi-value keyword, resulting in a list value. Convert it to a string before applying any `string()` command.

Split from https://github.com/bitcoin/bitcoin/pull/30861.

No current CMake code is affected by this bug.

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1830626118)
I've failed to suggest a simple code-change to avoid double-validation of a single tx.

The code is really about multi-tx. Even if we add a bunch of conditions around `auto multi_submission_result`, stuff below (`individual_results_nonfinal`) makes you think about multi-tx again.

The least thing I'm worried about is double-validation on itself.
I'm worried about code comprehension complexity: reading multi-tx code against a single-tx scenario.

At this point I agree with Gloria to handle
...

👍 i-am-yuvi approved a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2417660993)
ACK d22a234ed270286b483aec2db1e2f716b9756231

🤔 naumenkogs reviewed a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2417664548)
ACK 8351562bec6081eda2a600bfe4edeb264a9dee0b

I haven't verified what happens if `time` goes below 0

🚀 fanquake merged a pull request: "ci: Use clang-19 from apt.llvm.org"
(https://github.com/bitcoin/bitcoin/pull/30634)

👍 fanquake approved a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811#pullrequestreview-2417738480)
ACK 788c1324f3d840f7a39b8bc3537dcff26ca0b552

✅ fanquake closed an issue: "Unify -logsourcelocations format"
(https://github.com/bitcoin/bitcoin/issues/30799)

🚀 fanquake merged a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)

💬 laanwj commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2459213686)
Concept ACK