💬 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

💬 dergoegge commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459317676)
oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:

```
$ FUZZ=txdownloadman_impl out/libfuzzer/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.
```

[clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt](https://github.com/user-att
...

💬 dergoegge commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2459379945)
Perhaps relevant for the discussion around removing `testBlockValidity`: https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947

💬 laanwj commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#discussion_r1830802184)
Might want to do some basic checks on the pagesize; eg
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0):
```

💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459428224)
`99b1f88fe8...cf83f0c14c`: rebase due to conflicts and mock the sleeps

> > I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.

> Sounds good to me.

Done. `CThreadInterrupt` can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!

💬 dergoegge commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830823046)
Perhaps in a follow up we can make mocktime accurate to the millisecond and then advance it here to simulate an actual sleep.