💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(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:

```suggestion
if (!((chain_tip->nStatus & BLOCK_HAVE_MASK) == BLOCK_HAVE_MASK)) return chain_tip->nHeight;
```

Can be verified by adding unit test to the first commit, which passes on dca1ca1f56 and fails on current HEAD for `CheckGetPruneHeight(blockman, chain, 100)`:

```
$ ./src/test/test_bitcoin --run_test=blockchain_tests/get_prune_height
...
Running 1 test case..
...

💬 stickies-v commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1624721555)
I don't think this is fully addressed in 0501ec102f3740049bd2e891cd83527c31b29222 - would update to `return Assert(first_unpruned.pprev)->nHeight;`?

⚠️ ffrediani opened an issue: "Improve/simplify node sync for pruned nodes"
(https://github.com/bitcoin/bitcoin/issues/30220)
### Please describe the feature you'd like to see added.

When running a pruned node it means it will keep only last X amount of GB predefined in the configuration, so why is it necessary to download the whole blockchain if only a tiny part will be kept at the end ? Can't it just download the amount of necessary and most recent data or a method developed for it and to speed up adoption of pruned nodes that will never keep all that amount of data ?

### Is your feature related to a problem, if so
...

💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2145640002)
> Any thoughts on splitting the first commit out into a separate pull? Seems to be a requested feature by three people so far, but I am not sure about bundling it for review with the other changes here.

You are right, split out the first commit into: #30219

💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1624753063)
Thanks, I'll address this if I have to force push!

👍 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