💬 fanquake commented on pull request "fuzz: Remove legacy int parse fuzz tests":
(https://github.com/bitcoin/bitcoin/pull/27344#issuecomment-1485478244)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27344#issuecomment-1485478244)
Concept ACK
✅ fanquake closed an issue: "depends does not compile with clang-16 (fontconfig)"
(https://github.com/bitcoin/bitcoin/issues/27299)
(https://github.com/bitcoin/bitcoin/issues/27299)
🚀 fanquake merged a pull request: "depends: make fontconfig build under clang-16"
(https://github.com/bitcoin/bitcoin/pull/27301)
(https://github.com/bitcoin/bitcoin/pull/27301)
💬 vstoyanov commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485513160)
Apologies, could we reopen this one? I tried rebasing to retrigger Cirrus CI
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485513160)
Apologies, could we reopen this one? I tried rebasing to retrigger Cirrus CI
📝 vstoyanov reopened a pull request: "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)"
(https://github.com/bitcoin/bitcoin/pull/27333)
Basically it removes the above-mentioned env-vars as per @MarcoFalke's instructions. The only deviation from the plan laid out there was that I double-quoted the last instance of $ANDROID_HOME for the sake of consistency and future-proofing and the rest of the non-quoted vars due to lint failing the build.
References #27321
(https://github.com/bitcoin/bitcoin/pull/27333)
Basically it removes the above-mentioned env-vars as per @MarcoFalke's instructions. The only deviation from the plan laid out there was that I double-quoted the last instance of $ANDROID_HOME for the sake of consistency and future-proofing and the rest of the non-quoted vars due to lint failing the build.
References #27321
💬 vstoyanov commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1149558891)
It is basically the same trick described in #27321 to be used for `apt`, `dnf` is basically the Redhat package manager
The other option would be to arrayify PACKAGES and CI_BASE_PACKAGES. I have that in my git stash too, but even though it is a cleaner, it actually adds a couple extra lines when initialising the defaults. I value both cleanness and simplicity, so I cannot choose myself which way is better.
The interesting question is whether I should do the same for pip3 - otherwise the guy
...
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1149558891)
It is basically the same trick described in #27321 to be used for `apt`, `dnf` is basically the Redhat package manager
The other option would be to arrayify PACKAGES and CI_BASE_PACKAGES. I have that in my git stash too, but even though it is a cleaner, it actually adds a couple extra lines when initialising the defaults. I value both cleanness and simplicity, so I cannot choose myself which way is better.
The interesting question is whether I should do the same for pip3 - otherwise the guy
...
💬 josibake commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1149573218)
iirc, `./depends` was the only folder affected, so removing the second user account and running as root seems fine.
(https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1149573218)
iirc, `./depends` was the only folder affected, so removing the second user account and running as root seems fine.
💬 josibake commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485545697)
Concept ACK
lgtm (although, CI is failing). happy to re-review once passing. also happy to review any follow-ups to remove the extra user account, since that was only introduced to solve the problem of CI changing file permissions on `depends` when running CI locally (which seems to no longer be an issue per https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148898438)
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485545697)
Concept ACK
lgtm (although, CI is failing). happy to re-review once passing. also happy to review any follow-ups to remove the extra user account, since that was only introduced to solve the problem of CI changing file permissions on `depends` when running CI locally (which seems to no longer be an issue per https://github.com/bitcoin/bitcoin/pull/27333#discussion_r1148898438)
💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149579411)
> Last time I checked it had ~70k addresses and also it will have at least a bunch of addresses from each network, not just 1 address.
"bunch of addresses from each network", how much? Last time I checked my node (running with multiple networks) I remembered that the number of i2p addresses were so small compared to clearnet, for example.
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149579411)
> Last time I checked it had ~70k addresses and also it will have at least a bunch of addresses from each network, not just 1 address.
"bunch of addresses from each network", how much? Last time I checked my node (running with multiple networks) I remembered that the number of i2p addresses were so small compared to clearnet, for example.
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149570742)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697
> > allowignoredconf
>
> yeah
Renamed warnignoredconf to allowignoredconf
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149570742)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147716697
> > allowignoredconf
>
> yeah
Renamed warnignoredconf to allowignoredconf
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149569979)
> re: [#27302 (comment)](https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940)
>
> I don't think the PRs should be combined though. Better for them to stay targeted and simple. If there are any conflicts they should be trivial.
It turns out it is not possible to actually write a meaningful test for #27303 without some of the test code added in this PR. This PR adds test coverage for starting bitcoind without `-conf` or `-datadir` arguments, just with a `bitcoin.conf` file in
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149569979)
> re: [#27302 (comment)](https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1147719940)
>
> I don't think the PRs should be combined though. Better for them to stay targeted and simple. If there are any conflicts they should be trivial.
It turns out it is not possible to actually write a meaningful test for #27303 without some of the test code added in this PR. This PR adds test coverage for starting bitcoind without `-conf` or `-datadir` arguments, just with a `bitcoin.conf` file in
...
💬 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.
(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.
(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
...
(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
(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)
(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
...
(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` ?
(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
...
(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.
(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.