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

💬 maflcko commented on issue "Improve/simplify node sync for pruned nodes":
(https://github.com/bitcoin/bitcoin/issues/30220#issuecomment-2146978830)
This is possible to implement, but may be controversial, depending on how it is implemented. You can learn more about the topic by reading the various discussions around assumeutxo.

Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits,
...

💬 hebasto commented on pull request "feefrac: 128-bit multiply support in MSVC":
(https://github.com/bitcoin/bitcoin/pull/29758#issuecomment-2147006298)
I've add a [benchmark](https://github.com/hebasto/bitcoin/commit/0f5abaf980122cce1bd0d405d17db3bfab273afa) for the `FeeRateCompare` function, which calls `Mul` twice.

Here are results on Windows 11, `Release` configuration, which implies `/O2 /Oi` compile flags:
- master branch @ 61de64df6790077857faba84796bb874b59c5d15:
```
> .\build_msvc\master\bench_bitcoin.exe -filter=FeefracMultipication

| ns/op | op/s | err% | total | benchmark
|---------------
...