💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1228387547)
Nevermind that. I ran over about 500k executions on the coinselection fuzz test and could not substantiate my concern.

🤔 hebasto reviewed a pull request: "ci: enable AArch64 target in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27824#pullrequestreview-1477606221)
Post-merge ACK 2ebeb421dd21a69344a32c681ccbfcf5e5c6dab9. Tested on Ubuntu 23.04, `aarch64`.

FWIW, the [default](https://github.com/bitcoin/bitcoin/pull/27824#discussion_r1217967152) values, i.e., not setting `LLVM_TARGETS_TO_BUILD` altogether, works as well.

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1477609782)
Code review ACK ce88124bf499ae9608a2607ccddc00df3d7439bc. This looks great and I would encourage others to review. It should be much simpler to review now. Basically none of the commits are adding new code anymore, just moving code from one place to another.

Since my last review https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917 there were a lot of internal changes to make commits self-contained, but the only changes to the final code were making `LoadHDChain` catch ex
...

💬 mzumsande commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227463946)
`IsChildWithParents()` is unused after this outside of tests - can we remove it?

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