💬 jonatack commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1761957463)
Missing `#include <unordered_set>` in this file (include what you use)

```
/src/addrman.cpp should add these lines:
#include <algorithm> // for max, min
#include <compare> // for operator<, strong_ordering, operator>, operator<=
#include <iterator> // for advance
#include <ratio> // for ratio
#include <set> // for set, _Rb_tree_const_iterator, operator==
#include <unordered_map> // for un
...

💬 jonatack commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1761961810)
```suggestion
for (Network net : ALL_NETWORKS) {
```

💬 jonatack commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1761960091)
A `Network` is a cheaply-copied int enum; no need to use it by reference, unless I'm missing something.

```suggestion
for (Network network : networks) {
```

💬 willcl-ark commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2354055700)
Concept ACK.

I noticed a bit of a "gap" in our compilation on master the other day."


This PR appears to fix it quite nicely:


I didn't see quite the same speedup as you though, but will investigate and do some more thorough testing/review tomorrow.

These tests were also using a def
...

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354059450)
On my short list to re-review.

💬 achow101 commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1761911213)
In ecbbadf0f79bb32730d9e1400d84b84893e35e56 "[refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters"

Why have this bloom filter's parameters changed? It was previously `48'000` and `0.000'001`. If this was intentional, it should probably be a separate commit and a rationale provided.

💬 achow101 commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1761957878)
In c40cb8565952f62732fa64875c99ef9082b9a6f4 "[refactor] move notfound processing to txdownload"

nit:

```suggestion
for (const auto& txhash : txhashes) {
```

💬 achow101 commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1761978429)
In f3cc6322798ca61a31f90b2ad7a0c32471db0410 "[refactor] move new tx logic to txdownload"

nit:

```suggestion
m_txrequest.ReceivedResponse(nodeid, txid);
if (ptx->HasWitness()) m_txrequest.ReceivedResponse(nodeid, wtxid);
```

💬 1440000bytes commented on issue "Use IPv4-encoded IPv6 address to get IPv4 node address with port number from DNS seeds":
(https://github.com/bitcoin/bitcoin/issues/30900#issuecomment-2354062985)
> The purpose of the DNS seeds is just to find some bootstrap nodes.
> There's no need for the initial bootstrap nodes to be sampled from the entire set of possible nodes.

I disagree. @cdecker's DNS seed was recently [removed](https://github.com/bitcoin/bitcoin/pull/30720) from bitcoin core because it did not return enough nodes for bootstrapping. It was [noticed](https://github.com/bitcoin/bitcoin/pull/29145#pullrequestreview-1797445282) that @luke-jr's DNS seed returned only nodes with old
...

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354074486)
> On my short list to re-review.

Thanks, need to rebase but first I have to remove the validation for the CLI commands that seems it has been improved in #30148 which I missed to review.

📝 mzumsande opened a pull request: "wallet: Fix error messages telling user to specify wallet"
(https://github.com/bitcoin/bitcoin/pull/30912)
I'm not very familiar with the wallet and got confused by the error messages

`Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).`
`Try adding "-rpcwallet=<filename>" option to bitcoin-cli command line.`

not knowing which filename to put here (some `wallet.dat` in the wallet's directory?), when actually, at least for newer wallets, just the wallet name is expected.

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#discussion_r1762002120)
Note: It is okay to do this without worrying about `m_position` because `ftruncate` does not modify the position of the file seek pointer. (https://pubs.opengroup.org/onlinepubs/007904975/functions/ftruncate.html)

💬 jonatack commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354087261)
See https://github.com/bitcoin/bitcoin/pull/26990.

💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354091658)
(I think this could have been merged much more quickly if it only contained 6ccf7cb946bd3772b5fba92246be7e2281f59d09 and proposed the validation in a separate pull.)

💬 maaku commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2354094390)
macOS Sequoia is out, and both right-click gatekeeper override and the master spctl disable are removed.

Since it has been shown that macOS phones home regardless of signed binary status, maybe it is time to revisit this? Right now the user experience is terrible, and for literally no benefit.

💬 mzumsande commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354098897)
> See #26990.

Is this related? My problem was that I was asked to provide a filename when in reality a wallet name or maybe directory name is expected. That PR seems to address a different issue?

💬 jonatack commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354101564)
Yes, this is a duplicate of the first commit in #26990.

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2354110008)
untested reACK https://github.com/bitcoin/bitcoin/pull/30884/commits/a240e150e837b5a95ed19765a2e8b7c5b6013f35
Changes since https://github.com/bitcoin/bitcoin/commit/4cfff4e58c6d806e4bc5a12386f84ff207c83419 should not change any behavior, they just improve the `AutoFile` interfaces to be safer by

1. Ensuring we only perform XOR operations or `AutoFile::tell()` if the file has a sensible seek pointer; e.g. if you try to ftell a pipe: `ftell(stdout)` -1 will be returned, and `errno` will be s
...

💬 petertodd commented on issue "Use IPv4-encoded IPv6 address to get IPv4 node address with port number from DNS seeds":
(https://github.com/bitcoin/bitcoin/issues/30900#issuecomment-2354114841)
@1440000bytes As you can see by clicking through to the removal pull-req, the problem wasn't "insufficient nodes", it was failing to implement filtering properly: https://github.com/bitcoin/bitcoin/issues/29911

Re: censorship, that's just nonsense. If government is blanket censoring specific ports they can pretty much just as easily censor the DNS seeding mechanism itself. There's no need to include complex new code for such a niche use-case - you can easily bootstrap a node by getting the IP
...

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354129815)
> (I think this could be merged much more quickly if it only contained [6ccf7cb](https://github.com/bitcoin/bitcoin/commit/6ccf7cb946bd3772b5fba92246be7e2281f59d09) and proposed the validation in a separate pull.)

I do agree and thought many times about it haha... I'll split it out tomorrow. Thanks.