👍 ryanofsky approved a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2094387578)
Code review 0501ec102f3740049bd2e891cd83527c31b29222. Code looks very good, but the `nStatus & BLOCK_HAVE_MASK` bug found by stickies-v https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624683314 needs to be fixed, and the other suggested changes could be considered too

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624762665)
In commit "refactor, blockstorage: Generalize GetFirstStoredBlock" (dca1ca1f56d8e63929eb7aea64f8fb3e01752357)

I think the code would be simpler (and have fewer cases to think) about if this function just returned -1 when no blocks were pruned, instead of nullopt:

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -782,8 +782,8 @@ static RPCHelpMan getblock()
};
}

-//! Return height of highest block that has been pruned, or std::nullopt if no blocks have bee
...

💬 ryanofsky commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624770978)
re: https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624683314

> I think this is buggy, chain tip should be considered pruned if any of the `BLOCK_HAVE_MASK` flags are missing:

Nice catch! Would be great to add the test to the first commit.

💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2145726315)
@maflcko happy to keep the job together as it is. I will look into the functional tests that require IPV6.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1624812617)
Thanks @furszy

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1620960269)
can you motivate this seed in a comment?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1621275746)
Could we add `Assume()`/`Assert()`s rather than comments here and above?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1621072544)
If this is true, this helps my understanding of the code to follow, knowing that each loop below will eventually terminate.
```Suggestion
unsigned aux = (((args >> 6) & 3) * sim.size()) >> 2;
// Args are in range for non-empty sim, or sim is completely empty and will be grown
assert((sim.empty() && dest == 0 && src == 0 && aux == 0) ||
(!sim.empty() && dest < sim.size() && src < sim.size() && aux < sim.size()));

```

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1624946699)
did you consider using `std::popcount` instead?

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1624933811)
In commit "kernel: De-globalize fReindex" (b47bd959207e82555f07e028cc2246943d32d4c3)

> I think it would be less fragile to assign `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindex`.

This suggestion was not implemented in this PR, but is implemented in followup PR #30132.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1624912618)
In commit "kernel: De-globalize fReindex" (b47bd959207e82555f07e028cc2246943d32d4c3)

It turns out there was a bug introduced here. Setting `chainman.m_blockman.m_reindexing = true` has no effect because chainman is destroyed and recreated on the next loop iteration on line 1537 above. So after this change, reindexing no longer occurs when the user answers "Yes" above. This is fixed in #30132

👍 ryanofsky approved a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2094875458)
Code review ACK eeea0818c1a20adc5225b98b185953d386c033e0. All the changes here look good.

But I also think it was a mistake in #29817 to add the `BlockManagerOpts::reindex` option, and think it would be better to have a less confusing set of options. The following change built on top of this PR could provide a simpler alternative: 9c643e7ddd82523b84f65b614d3e6c1f640b23c7. Feel free to use any of these changes, or they could be a separate followup.

📝 achow101 opened a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221)
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484

Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.

This PR changes the those last block fields to actually be in sync with the record stored on disk. This requires adding
...

💬 achow101 commented on pull request "wallet: Avoid potentially writing incorrect best block locator":
(https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2146197896)
> I'm now wondering whether the wallet should even be doing anything on `ChainStateFlushed` since that doesn't seem like it should have any bearing on what the wallet knows about. [...]

I've implemented this idea in #30221

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1625154834)
I followed IWYU here and added kernel/mempool_removal_reason.h to wallet.h

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1625155299)
@josibake Deleting the nullptr constructor worked.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2146288055)
Thanks for the reviews @josibake and @furszy I have implemented your suggested changes

📝 Eunovo converted_to_draft a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)
This PR implements a fix for the issue described in https://github.com/bitcoin/bitcoin/issues/29435.

The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here, augments the mempool transaction `REPLACED` signal with the double-spending transaction which the wallet stores and watches for in Block notifications. A map is added to the wallet to track conflicting tx ids and their child transactions. The entry is erased wh
...

🚀 fanquake merged a pull request: "depends: Update Boost download link"
(https://github.com/bitcoin/bitcoin/pull/30217)

✅ maflcko closed an issue: "Improve/simplify node sync for pruned nodes"
(https://github.com/bitcoin/bitcoin/issues/30220)