💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149454952)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149161983

> Nit: This is currently printing the line instructing the user to set `warningnoredconf=1` even when it is already set.

Thanks, changed "resolve" to "address" and reworded the text to make it clear a warning will still occur.

💬 jonatack commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149603943)
> "bunch of addresses from each network", how much?

If helpful, my node knows 15k Tor, 1.2k I2P and 8 CJDNS recently active peers ATM for a bit more than 16k total non-clearnet peers.

💬 1440000bytes commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1485580415)
> > > Show an error on startup if a bitcoin datadir that is being used contains a bitcoin.conf file that is ignored
> >
> >
> > ```
> > 1. Why is it ignored?
> > ```
>
> It's debatable whether it should be ignored, but the reason it is ignored is that one configuration file has already been parsed and trying to merge in a second configuration file in another datadir would be new and potentially confusing behavior. Merging in another configuration file could lead to unexpected conflicts
...

💬 pinheadmz commented on pull request "system: cache config file path before potentially updating datadir":
(https://github.com/bitcoin/bitcoin/pull/27303#issuecomment-1485585085)
closing for #27302

✅ pinheadmz closed a pull request: "system: cache config file path before potentially updating datadir"
(https://github.com/bitcoin/bitcoin/pull/27303)

👍 pinheadmz approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302)
ACK ac9fee615a4f0c4d1bbed0d69486c54be4860dcb

Reviewed code and ran tests. Confirmed tests fail on master, not on branch. I believe this PR also closes https://github.com/bitcoin/bitcoin/issues/19990 as a nice added bonus.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK ac9fee615a4f0c4d1bbed0d69486c54be4860dcb
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQh374ACgkQ5+KYS2KJ
yTqaMRAAzyu11syUK7aDjZBg
...

💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149642067)
Not a blocker for me, but curious if this kind of function should go in a broader utility package like `test_node.py` ?

💬 ryanofsky commented on issue "Relative paths named in the -conf parameter reset when parsing datadir in named config":
(https://github.com/bitcoin/bitcoin/issues/19990#issuecomment-1485660303)
> This issue is solved by #27303

Good find, agree it solves originally reported issue.

It might still be a little confusing that relative -conf paths would be allowed on the command line, and wouldn't be interpreted relative to the current working directory or to the _final_ datadir location, but relative to the _default_ datadir location. But arguably there are uses cases for this behavior (like putting multiple `1.conf`, `2.conf` files in the default datadir location, and having them spe
...

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1485670242)
> NACK

I guess you want multiple bitcoin.conf files if detected to be merged together. But I'm not sure you should NACK this PR because they aren't merged together currently. This PR just warns the user about multiple files being present, and should make it easier and safer to merge them later if someone wants to implement that.

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1149662848)
As far as I can tell the bcrypt.lib dependency was introduced into libevent in 2020. https://github.com/libevent/libevent/commit/138a408c771c21668bcc605c2df00d0384d476d8

💬 mzumsande commented on issue "test_bitcoin: ./chain.h:261: uint256 CBlockIndex::GetBlockHash() const: Assertion `phashBlock != nullptr' failed.":
(https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1485694691)
I think the following may be happening:
In the failed run, there is an `UpdatedBlockTip` scheduled in line [3169](https://cirrus-ci.com/task/6024293113397248?logs=ci#L3169) that has not been processed yet:
```
2023-03-23T20:11:01.813759Z (mocktime: 2020-08-31T15:36:01Z) [test] [validationinterface.cpp:201] [UpdatedBlockTip][validation] Enqueuing UpdatedBlockTip: new block hash=4fe08ff1d78843f1b6f0d2b80fee9af49b3c945368e585b8aed1807c89d14804 fork block
```
Next, the test calls `SimulateNodeR
...

💬 1440000bytes commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1485706572)
> > NACK
>
> I guess you want multiple bitcoin.conf files if detected to be merged together. But I'm not sure you should NACK this PR because they aren't merged together currently. This PR just warns the user about multiple files being present, and should make it easier and safer to merge them later if someone wants to implement that.

This PR confirms we are okay with buggy behaviour defined in https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1485326965

I am not okay with that
...

💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149661885)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149193825

Thanks, switched to brace initialization like you suggested in many places now. Should be clearer and safer

💬 Ayush170-Future commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485729334)
ACK on the concept.

I've been following this since the start and have reviewed the changes once again. The approach followed looks good to me. If the checks fail again, I'd be happy to re-review (they are running currently).

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1485740590)
Rebased 7fd1401d8116323adfa2a87bbcf6ea41437cd0fa -> 927a910cbe6c96d60c6a8b05baad179ef3f61f1c ([`pr/nonly.11`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.11) -> [`pr/nonly.12`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.11-rebase..pr/nonly.12)) due to conflict with #26642.

📝 mzumsande opened a pull request: "test: fix intermittent failure in ChainStateManager tests"
(https://github.com/bitcoin/bitcoin/pull/27348)
Before wiping a `ChainStateManager` and creating a new one, the `validationinterface` queue should be emptied to avoid the possibility of accessing deleted memory.
This could lead to very rare CI failures reported in #26613 and #27320 (see [here](https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1485694691) for a more detailed explanation).

Fixes #27320

💬 jamesob commented on pull request "test: fix intermittent failure in ChainStateManager tests":
(https://github.com/bitcoin/bitcoin/pull/27348#issuecomment-1485769153)
crACK https://github.com/bitcoin/bitcoin/pull/27348/commits/f8abcb3e3b2e731c002ec88f7559c21e26a2c079

Thank you for fixing this! During *actual* node restart (which this unittest is trying to model) the validationinterface queue will of course have been flushed, so this change makes the test affected more realistic.

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149737863)
> Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.

agreed that this is the fundamental thing we are trying to evaluate - is the performance difference significant & acceptable

> I will try to compare on a snapshot of an addrman from my public full node, not on artificially filled addrman from the bench. Last time I chec
...

💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149790004)
> What is your "Test 5" doing? Does it call 20 times `Select(network)`?

No, it calls `Select(network)` for an addrman that has 20 addresses of the selected network.

I'll try to explain the idea behind my benchmarks better:
Starting point is the observation that the suggested change makes `Select` slower if we already know a lot of addresses of the desired type, while making it faster if addrman has very few addresses of this type. This is true for botch `Select()` and `Select(network)`,
...

💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149811002)
I don't know if I'm doing something supported here, but this doesn't seem to work for me:

```fish
./src/bitcoind -datadir=/tmp/bitcoin_27302/ -rpcport=18555 -port=18556
```

```log
2023-03-27T21:22:09Z Default data directory /home/will/.bitcoin
2023-03-27T21:22:09Z Using data directory /tmp/bitcoin_27302/regtest
2023-03-27T21:22:09Z Config file: /tmp/bitcoin_27302/bitcoin.conf
2023-03-27T21:22:09Z Config file arg: datadir="/tmp/bitcoin_27302/datadir2"
2023-03-27T21:22:09Z Config file
...