💬 TheCharlatan commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589867124)
Re https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589833222

> ACK, I'll revert to using `AbortNode` then for now, that should keep any potential churn minimal. Once #27866 has received sufficient review, we can always decide to add special cases (e.g. flush error notifications) afterwards.

Ugh, just noticed this gets annoying with the exit status - we probably don't want to introduce that in the blockstorage scope. Leaving as is then :/.

📝 achow101 opened a pull request: "Add menu option to migrate a wallet"
(https://github.com/bitcoin-core/gui/pull/738)
GUI users need to be able to migrate wallets without going to the RPC console.

💬 jonathanbier commented on pull request "doc: update comment in policy.cpp to refer to virtual bytes":
(https://github.com/bitcoin/bitcoin/pull/27726#discussion_r1228585996)
Thanks @Xekyo

I was just trying to understand this policy

Next time I have a question like this I will ask on stackexchange rather than creating pull requests here.

👍 pinheadmz approved a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1477862322)
ACK efe5f373e12329104f1c706145eee75c2d2079a7

Great work on this! A few non-breaking questions below.
Ran all tests locally, ensured all 3 new tests fail on `master`.
Reviewed each commit, confirmed regtest-only argument is only accepted on regtest!

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

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

ACK efe5f373e12329104f1c706145eee75c2d2079a7
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmSIvz0ACgkQ5+KYS
...

💬 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