💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1200786113)
Changed to increment unconditionally, but these counters all get dropped in a later commit anyways.

📝 theuni opened a pull request: "depends: remove redundant stdlib option"
(https://github.com/bitcoin/bitcoin/pull/27721)
Like #27628, this is another dependency of #21778, though it doesn't become obvious until used with a newer clang.

This should be a no-op.

Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1557593304)
> So I guess we'll need to patch it out of guix.

Suggestions for how to go about this are welcome. It's not at all straightforward to me how to patch out.

The llvm toolchain bumps are currently blocked on this :(

🤔 stickies-v reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1436822917)
Code review ACK 96b513f605fb2df441b66da583056b1c8acd4dbc, but I think the first commit should be removed from this PR, I think it's an accident?

I think the code is good and suits the intent of the PR well. My only concern is about the potential footgun introduced, even if it is relatively mild. Anyone running bitcoind with `whitebind=0.0.0.0:<port>` will now be vulnerable to having all their inbound non-whitelisted slots taken over by an attacker that has figured out what the whitelisted por
...

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200632788)
nit: if `vEvictionCandidates` is empty, there's no point (I think, couldn't see any side effects in `EraseLastKElements` or `ProtectEvictionCandidatesByRatio`) executing the next lines so might as well return early? Unless you think this makes the code less clear?

```suggestion
if (vEvictionCandidates.empty()) return std::nullopt;
std::optional<NodeId> force_evict;
if (force) {
```

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1200865042)
As you [mentioned](https://bitcoincore.reviews/27600#l-165) in the review club, whitelisting just based on ports enables an attacker that discovered which port(s) are whitelisted to take over all (non-whitelisted) inbound connections slots.

Therefore, I think we have to be careful not to accidentally lead people to this footgun. Binding to `127.0.0.1` seems much more prudent to not set a bad example.

```suggestion
self.restart_node(0, extra_args=['-maxconnections=12', '-whitebind=
...

💬 furszy commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1200902347)
little topic:
it seems odd to re-use this two calls in other bench contexts.

`NotifyWalletLoaded` is looping over the wallet load callbacks, and `postInitProcess` syncs the wallet with the mempool, so guess that both will be no-op in most cases?.

💬 fanquake commented on pull request "depends: remove redundant stdlib option":
(https://github.com/bitcoin/bitcoin/pull/27721#issuecomment-1557725108)
Concept ACK

💬 theStack commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200954342)
Nice indeed, will take it if I have to retouch.

💬 theStack commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200970095)
As per #19469 and #20755 it is not possible any more to inspect a peer's ban score via RPC. Could either assert for a more concrete debug log message (containing the expected '(0 -> 20)' banscore before/after string), or even assert for a disconnect after send the problematic message 5 times (100/20), but that all seems excessive and we also don't do that in other functional tests checking misbehavior. As for "assering about not disconnecting", I think that's already covered by using the `send_a
...

💬 achow101 commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1201120763)
Yes, I think they'll be no-op for the most part.

👍 theStack approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1437730435)
re-ACK 4eee95e57bb6f773bcaeb405bca949f158a62134

Left two comments below, one nit and one "thinking out loud what could happen in the worst case", both no stoppers.

💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201199915)
nit, if you need to retouch:
```suggestion
const std::optional<CNetAddr> addr{LookupHost(request.params[0].get_str(), false)};
```

💬 theStack commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1201230428)
No stopper, but I think there should be really a good reason to remove existing unit-test checks. For example, if (for whatever reason) `LookupHost` fails and hence returns std::nullopt, we'd now get a more cryptic error message without seeing the direct line number of the cause:
```
unknown location(0): fatal error: in "net_tests/cnetaddr_basic": std::bad_optional_access: bad_optional_access
test/net_tests.cpp(133): last checkpoint: "cnetaddr_basic" test entry
```
compared to an explicit

...

⚠️ Crypt-iQ opened an issue: "bitcoind hangs waiting for `g_requests.empty()`"
(https://github.com/bitcoin/bitcoin/issues/27722)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

bitcoind hangs

### Expected behaviour

bitcoind shouldn't hang

I was able to dump the running threads and it was stuck waiting for `g_requests` to be empty. The issue seems to be that this callback may not actually be called in case of error: https://github.com/bitcoin/bitcoin/blob/22139f6e83a2bedd2dad9f280567d2c76c54252f/src/httpserver.cpp#L222-L227

See this issue https://github.c
...

💬 theStack commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1558205497)
>>So far so good, everything went quite smooth and nothing unexpected happened! The only disappointment was finding that apparently the provided prune target of 800MB was not respected:
>
>Yes, this is unfortunately expected behavior. Since you have -blockfilterindex enabled and the background sync hasn't yet completed, prune locks are preventing any pruning from happening on the snapshot chainstate. I mentioned this obliquely here: https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-15
...

💬 BebeSparkelSparkel commented on issue "Cannot sync with limited RAM and Swap":
(https://github.com/bitcoin/bitcoin/issues/24700#issuecomment-1558482031)
I will try again when I get the chance and let you know.

💬 chippsmith commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1558495158)
I downloaded the binaries on a System76 machine running Pop!OS 22.04 LTS. I ran all the tests in the testing guide(except the last one about verifying binaries) and got expected results. More detailed notes can be found here. https://github.com/chippsmith/bitcoinnotes/tree/main/bitcoin25.0rc.

⚠️ CryptoManiac opened an issue: "Validation of malformed address fails with peculiar message"
(https://github.com/bitcoin/bitcoin/issues/27723)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Open RPC console and execute this command:

`validateaddress bc1qqrq69gfzvvqcxs6rgg3crqjzcw369sxzyp3v9sspursx9gmzyv32x7xa5z`

Following message is the result:

```
Internal bug detected: 'isValid == error_msg.empty()'
rpc/misc.cpp:75 (operator())
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
(code -1)
```


### Expected behaviour

Well it should tel
...

💬 de-served commented on issue "Bitcoin ignores datadir and blocksdir parameter in .conf":
(https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1558537251)
> My recommendation for you personally would be to configure the GUI to use the default datadir and place your `bitcoin.conf` file in `C:\Users\USERNAME\AppData\Roaming\Bitcoin` instead of

It's already here... for a "debug" what's happening:
`
datadir =W:/BitcoinCore/DataDir_/_User
blocksdir =W:/BitcoinCore/BlocksDir_/_User
walletdir =W:/BitcoinCore/WalletDir_/_User
debuglogfile=H:/_Logs_/BitcoinCore/_debug.default-appdata.no_params.log

[main]
datadir =W:/BitcoinCore/Data
...

💬 MarcoFalke commented on pull request "test: Wallet interactions with rescanning wallet":
(https://github.com/bitcoin/bitcoin/pull/26700#discussion_r1201575251)
This will race against the thread `t`, which may have already concluded before this point