💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1228398085)
I don't think it's unnecessary - the diamond package would be accepted if we remove this check, the subpackage algorithm doesn't help with this. However, I'm also not sure if it's sufficient - e.g. what if we'd add another "grandchild" to the child of the diamond package, with a feerate slightly above the previous package feerate (e.g. 5.1 sat/vB). Then this check wouldn't fail anymore, but the descendant feerate of the child (i.e. the combined fees of the child and the grandchild) could be bel
...

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1228287010)
after c44df9b0bfa66c77e5cbbff6688ea73cf39ec1d3, the `packages.md` explanation "Packages must be child-with-unconfirmed-parents packages." is out of date. It would be good to document the definition of an "ancestor package" there instead.

🤔 Xekyo reviewed a pull request: "wallet: tx creation, don't select outputs from txes that are being replaced"
(https://github.com/bitcoin/bitcoin/pull/26732#pullrequestreview-1477680060)
Concept ACK

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228448590)
Split this out into its own commit.

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1228449099)
I think this should be a bit better now?

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