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

💬 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
...