💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1228571577)
Question: isn't `bool` small enough to pass by value instead of reference? (i.e. remove the `&` ?)

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1228568682)
I think if you retouch there is some style nit too:

```diff
- /** fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,
+ /**
+ * fee_estimates.dat that are more than 60 hours (2.5 days) will not be read,
```

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1228573519)
Does this need to be a class property since we only use it one time, during object construction?

🤔 ryanofsky reviewed a pull request: "validation: Stricter assumeutxo error handling when renaming chainstates"
(https://github.com/bitcoin/bitcoin/pull/27862#pullrequestreview-1477890938)
Updated 7564b87ebab27bea76efe5dd54d3f6f41cc78a1b -> 1ac09b93cdb41eb7dbc1a62364363e59507da1af ([`pr/assumeabort.1`](https://github.com/ryanofsky/bitcoin/commits/pr/assumeabort.1) -> [`pr/assumeabort.2`](https://github.com/ryanofsky/bitcoin/commits/pr/assumeabort.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/assumeabort.1..pr/assumeabort.2)) splitting this into two commits and making more changes to `InvalidateCoinsDBOnDisk` to normalize its error handling

💬 ryanofsky commented on pull request "validation: Stricter assumeutxo error handling when renaming chainstates":
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1228586551)
re: https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1227196719

> This seems like a significant behavior change.

It does only happen when snapshot validation fails (so the node would be shutting down anyway), and renaming the snapshot also fails. Just reraising the exception seemed like this simplest possible approach to take (copied from code in `ValidatedSnapshotCleanup`). But it is simple enough to clean up the code more. I pushed a new version which gets rid of the AbortNode c
...

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1589897031)
I fixed the CI failures by skipping tasks that are run as root. Root privileges override the read-only file permissions and the test fails because we expect an error that doesn't throw (can not open read-only file in write mode).

There are 7 tasks that skip this test for this reason:

[Win64 native [vs2022]](https://cirrus-ci.com/task/6272496312254464?logs=functional_tests#L2742)
[[ASan + LSan + UBSan + integer, no depends, USDT] [lunar]](https://cirrus-ci.com/task/5287333893767168?logs=ci
...

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

💬 TheCharlatan commented on pull request "validation: Stricter assumeutxo error handling when renaming chainstates":
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1228615119)
Nice, this is exactly what I had in mind! Can be resolved.

📝 achow101 opened a pull request: "gui: Disable and uncheck blank when private keys are disabled"
(https://github.com/bitcoin-core/gui/pull/739)
Unify the GUI's create wallet with the RPC createwallet so that the blank flag is not set when private keys are disabled.

🤔 pablomartin4btc reviewed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1477911157)
I think you'd need to check some comments.

💬 pablomartin4btc commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1228627690)
It would if -> Default to "help generate" when wrong number of parameters are given (line 524), but it's the same case anyway, it's the condition after the`or`operator,

💬 pablomartin4btc commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1228605741)
I agree.

💬 pablomartin4btc commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1228620295)
I agree, I thought before that could be a pseudo-implementation of a handler pattern but the evaluation is being done checking the key of the map is being the specific command in the command-line.

💬 pablomartin4btc commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1228606120)
I agree.

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1228631706)
Removed this test so this is no longer relevant.

💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1589931354)
> The GUI would be simpler and clearer if it made blank and watch-only wallets separate choices instead of overlapping options.

I've done that in https://github.com/bitcoin-core/gui/pull/739

> So I'd be happier if the PR kept all the new test coverage and bugfixes but dropped the second commit "Set blank when disable private keys in createwallet" [37dd054](https://github.com/bitcoin/bitcoin/commit/37dd054d9c7d58e829338c88df836241c0259f99)

Dropped it.

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1225923606)
in 29814149f29ce1858ee3f6ea2fbd37ec411070d6: for primitive types like bools, it makes more sense to pass by value

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1225924258)
29814149f29ce1858ee3f6ea2fbd37ec411070d6
The comment from L33 seems to belong better above here. Also, please make doxygen compatible.

```suggestion
/** Whether to allow importing a fee_estimates file older than MAX_FILE_AGE */
const bool m_read_stale_estimates;
```

💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1225926128)
in efe5f373e12329104f1c706145eee75c2d2079a7

Is this 61 hours? Or 1 second past the 60hour mark?

👍 TheCharlatan approved a pull request: "validation: Stricter assumeutxo error handling when renaming chainstates"
(https://github.com/bitcoin/bitcoin/pull/27862#pullrequestreview-1477986292)
ACK 1ac09b93cdb41eb7dbc1a62364363e59507da1af

That windows build failure is weird though. How can there be a conflict between `std::format` and `tfm::format`, if we are only in the latter's namespace? Why is this only popping up now?

💬 achow101 commented on pull request "test: (refactor) Use datadir from options in chainstatemanager test":
(https://github.com/bitcoin/bitcoin/pull/27876#issuecomment-1589975135)
ACK d54819d74e04e6105c1f0362755f5bcfa845eefd