💬 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

💬 MarcoFalke commented on pull request "test: (refactor) Use datadir from options in chainstatemanager test":
(https://github.com/bitcoin/bitcoin/pull/27876#discussion_r1228144723)
review note, this creates a copy and is not a reference, thus safe

💬 glozow commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1589349516)
Concept ACK

💬 glozow commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1589359272)
https://cirrus-ci.com/task/6053265947754496 ? 😢

💬 dimitaracev commented on pull request "build: suppress external warnings by default":
(https://github.com/bitcoin/bitcoin/pull/27872#discussion_r1228179276)
Did you mean `default is to suppress`?

💬 dergoegge commented on pull request "build: make sure we can overwrite config.{guess,sub} before doing so":
(https://github.com/bitcoin/bitcoin/pull/27875#issuecomment-1589385892)
Concept ACK

I've been having this issue on my machine, will test.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1228189310)
The actual error ends up being generic:

```
Error: A fatal internal error occurred, see debug.log for details
```

and actually before #27708 was merged yesterday, this code was even more messy!


The problem I'm having now is with CI: on only a few of the test platforms bitcoind does not crash as expected. This means that either the python line `os.chmod(...)` is not working to change the permissions of the `.blk` file OR for whatever reason the OS is happy to `open()` files with `rb
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1211719573)
nit: spurious?