💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1228455392)
No, going to change it.

👍 ryanofsky approved a pull request: "kernel: Add interrupt class"
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1477669211)
Code review ACK 39d9b5144410c15385ee48bc886e0dea36f34b6a. I left a suggestion to reduce size of changes to index code, and I still think it is inappropriate to treat flush errors as fatal errors, and would prefer to add a separate flush error method (https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226847296), but I won't get hung up on this.

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1228439282)
In commit "kernel: Add fatalError method to notifications" (39d9b5144410c15385ee48bc886e0dea36f34b6a)

Would suggest reverting all the calls to `FatalErrorf` this commit and just making `FatalErrorf` a member function to minimize the diff and avoid adding lots of new context accesses. Then the only src/index/ changes that would be needed would be:

```diff
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -8,6 +8,7 @@
#include <interfaces/chain.h>
#include <kernel/chain.h>
#inclu
...

💬 ryanofsky commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589734220)
re: https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589076014

> * De-globalized the `exit_status` in `shutdown.*`. My current approach touches quite a few lines though and seems a bit unclean, suggestions for alternative approaches are appreciated.

I'm actually not sure what this is referring to. `exit_status` is not referenced in `shutdown.cpp` at all after this PR: https://github.com/TheCharlatan/bitcoin/blob/39d9b5144410c15385ee48bc886e0dea36f34b6a/src/shutdown.cpp.

Also,
...

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228470071)
Sorry that comment was confusing. I dropped it, and instead change the behavior around managing `setBlockIndexCandidates` in the last two commits.

I think master has a couple bugs. When we start up, in `LoadBlockIndex()` we weren't adding blocks that have been downloaded and have more work than the background chainstate's tip to `setBlockIndexCandidates`, which prevents those blocks from being validated and connected on startup. Also, because `AcceptBlock()` is a member of `Chainstate`, it
...

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228471308)
This LogPrintf() disappeared during my edits, so this is gone now.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228475690)
If you're asking if it's ok to call `ActivateBestChain()` on both chainstates, the answer to that is essentially always "yes". ABC() doesn't do anything if no new candidates for most-work chain have been added to our block index; but if any block that has more work than our tip is now available we want to try to connect it.

When calling `LoadExternalBlockFile()`, we're adding blocks that theoretically could be of interest to either chainstate; so I think it makes sense to separate the block
...

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228476418)
Ah, thanks for noticing this. I believe I had an earlier version of this branch with `assert()`'s added in to ensure that the snapshot base was never `nullptr`, and so this test had been failing. I guess I removed those asserts before pushing... I've re-enabled the test for now, but really this test should be re-written so that snapshots are always at blocks in the block index.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228477327)
I have no idea why I commented this out -- I've re-added as well.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228498661)
Thanks, I took these changes and re-enabled the test.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1589777766)
@sjors @achow101 Thanks for the review! I split up the big commit as @sjors suggested, and with the test fixes I think this is ready to be moved out of Draft status.

👋 sdaftuar's pull request is ready for review: "Draft: rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746)

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

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

> and I still think it is inappropriate to treat flush errors as fatal errors, and would prefer to add a separate flush error method (https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1226847296), but I won't get hung up on this.

ACK, I'll revert to using `AbortNode` then for now, that should keep any potential churn minimal. Once https://github.com/bitcoin/bitcoin/pull/27866 has received sufficient revie
...

💬 dimitaracev commented on issue "Creating too many wallets exhausts file descriptor limit and leads to crash":
(https://github.com/bitcoin/bitcoin/issues/27732#issuecomment-1589856647)
I can take this if still available.

💬 brunoerg commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#issuecomment-1589864028)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442250 and https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1222442326

💬 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 `&` ?)