💬 fanquake commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589115669)
> I think there is still a benefit to developers being notified about warnings in headers.

They can still be notified, by disabling the suppression.

> Otherwise no one may notice the false warnings and report them upstream.

Then we should stop suppressing them globally in the CI, so we are more likely to see (and then report) the warnings across all relevant platforms / arches. Although then we can't use `-Werror` (which is also why some devs would always be suppressing locally).

💬 MarcoFalke commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589136728)
Also, it may be good to give some more background. IIRC this is only needed for bdb4 and qt5, so someone compiling bitcoind with sqlite shouldn't have to suppress warnings, no?

⚠️ 0xB10C opened an issue: "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied"
(https://github.com/bitcoin/bitcoin/issues/27873)
https://github.com/bitcoin/bitcoin/issues/26422 broke the `autogen.sh` part of my development builds on NixOS - found #26422 by bisecting. The error is:

```
$ ./autogen.sh
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'build-aux/m4'.
libtoolize: copying file 'build-aux/m4/libtool.m4'
libtoolize: copying file 'build-aux/m4/ltoptions.m4'
libtoolize: copying file 'bu
...

💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589157862)
https://github.com/bitcoin/bitcoin/blob/8de9bb7a5ab04669369e2bb59ea92a5c1a91a8d2/autogen.sh#L18 evaluates to true for me. The `depends/config.guess` file has a timestamp of `2023-01-01` while the generated `build-aux/config.guess` file has a timestamp of `2021-06-03`.

The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.

```
$ autoreconf --version
autoreconf (GNU Autoconf) 2.71
```

💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589198065)
> The generated `build-aux/config.guess` file has the file permissions `-r-xr-xr-x` so overwriting it fails.

`autoreconf --install --force --warnings=all` copies from the nix-store (in this case `/nix/store/3bvdlza5sxy6kjs9y7v0kbx4gxgp0wg8-autoconf-2.71/share/autoconf/build-aux/config.guess`) to `build-aux/config.guess`. The file has `-r-xr-xr-x` permissions in the nix-store. These permissions are preserved when copying. So the problem is a combination of how the nix-store works, how `autorec
...

💬 0xB10C commented on issue "Can't copy to 'build-aux/config.guess' in autoconf.sh: Permission denied":
(https://github.com/bitcoin/bitcoin/issues/27873#issuecomment-1589220664)
Changing the permissions for build-aux/config.{guess, sub} with `chmod ug+w` allows us to overwrite the files. Will PR this.

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228062756)
nit:

```suggestion
// How often to flush fee estimates to disk.
```

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228063658)
nit: Perhaps we can give this a slightly more descriptive name? `FEE_ESTIMATE_MAX_AGE` or something?

Would need to update comment below if you do change

💬 fanquake commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#issuecomment-1589239464)
> IIRC this is only needed for bdb4 and qt5,

There are likely warnings produced for all dependencies (includeing Boost and libevent),
because warning output is dependant on:
* architecture
* operating system (& version)
* compiler (& version)
* compiler flags being used
* other compile options i.e LTO
* packages being used (& version) (& system vs depends)

Where a number of the above continue to change over time.

> so someone compiling bitcoind with sqlite shouldn't have to sup
...

📝 0xB10C opened a pull request: "build: make sure we can overwrite config.{guess,sub} before doing so"
(https://github.com/bitcoin/bitcoin/pull/27875)
Since ea7b8528 (#26422), `autogen.sh` overwrites the `build-aux/config.{guess, sub}` files (installed there by `autoreconf`) with the `depends/config.{guess, sub}` files if these are newer.

The `autoreconf` tool copies them from it's `share/autoconf/build-aux/` directory. Specifically on NixOS, the `share/autoconf/build-aux/` files are located in the nix-store and are read-only. `autoreconf` preserves the read-only permissions when copying. Overwriting them with our `depends/config.{guess, su
...

💬 hebasto commented on pull request "build: make sure we can overwrite config.{guess,sub} before doing so":
(https://github.com/bitcoin/bitcoin/pull/27875#issuecomment-1589290446)
Concept ACK. However, I'm not a NixOS user and cannot test this PR, unfortunately.

💬 Xekyo commented on pull request "[coinselection] Increase SRD target by change_fee":
(https://github.com/bitcoin/bitcoin/pull/27846#issuecomment-1589306983)
I cannot reproduce the issue with `test/functional/interface_usdt_mempool.py` locally. Any idea what’s up here?

📝 TheCharlatan opened a pull request: "refactor(test): Use datadir from options in chainstatemanager test"
(https://github.com/bitcoin/bitcoin/pull/27876)
This should make the test less reliant on argument state from the test setup. This is a follow-up PR as requested in https://github.com/bitcoin/bitcoin/pull/27576#discussion_r1224638890.

💬 TheCharlatan commented on pull request "kernel: Remove args, settings, chainparams, chainparamsbase from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27576#discussion_r1228126265)
https://github.com/bitcoin/bitcoin/pull/27876

🤔 willcl-ark reviewed a pull request: "Mempool: persist mempoolminfee accross restarts"
(https://github.com/bitcoin/bitcoin/pull/27859#pullrequestreview-1477104823)
I like the approach here and think persisting mempool min fee is useful. I also agree that re-using fee_estimates.dat would make sense for it, as it likely doesn't need its own file.

Left a few nits and suggestions inline.

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228086493)
An alternative could be to also set `rollingMinimumFeeRate`, and then also set`lastRollingFeeUpdate` to the timestamp of the min fee dump, and then just let `GetMinFee() run its exponential backoff algorithm as per normal?

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228088807)
nit: same here re. comments (and a few more places).

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228088500)
nit: I think the code is clear enough here that comments aren't really needed?

💬 willcl-ark commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1228094327)
I would agree that it makes sense to dump into fee_estimates.dat.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1228135411)
I don't understand what this scenario has to do with this case. A will be accepted, then B rejected, then B+C rejected, all for having package feerates lower than 10 sat/vbyte.

👍 MarcoFalke approved a pull request: "test: (refactor) Use datadir from options in chainstatemanager test"
(https://github.com/bitcoin/bitcoin/pull/27876#pullrequestreview-1477197363)
lgtm ACK d54819d74e04e6105c1f0362755f5bcfa845eefd