💬 fanquake commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2114842252)
Now that the research has been done, it should be added to the commit message and PR description, so it's clear why this change might be safe, or the previous thread_local assumptions misguided. The commit message of `The previously mentioned erroneous behavior is not an issue anymore.` explains nothing.

💬 glozow commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603098402)
> although unclear to me why we don't also remove the child transaction from fee estimation to avoid overestimating

A child is already ignored, new mempool txns with unconfirmed parents are never added

https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/validation.cpp#L1274-L1279
https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/policy/fees.cpp#L614-L615

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603108416)
Thanks for your review, Josie. I think you are missing something.

When the child arrives, it will have a mempool parent, and we currently do not consider all transactions with mempool parent for fee estimation.

So when processing a transaction in `processBlockTx` all transactions with mempool parent, will be missing and , and we will skip them.

Child transactions whose parents aren't in our mempool will also be missing, and we will skip them because orphan transactions aren't added to t
...

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1603130595)
Yeah, I can pick this up in a separate PR.

💬 hebasto commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2114912000)
> Now that the research has been done, it should be added to the commit message and PR description, so it's clear why this change might be safe, or the previous thread_local assumptions misguided. The commit message of `The previously mentioned erroneous behavior is not an issue anymore.` explains nothing.

Thanks! Done.

💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1603142578)
With the databases I tested with, this branch is always executed, as `inner_meta.last_page` is 2, and each processed page `curr_page` is >= 3. The reason that we don't notice is that there is a `throw` missing here. I assume we should do this instead:
```suggestion
if (curr_page > outer_meta.last_page) {
throw std::runtime_error("Page number is greater than database last page");
}
```

💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2114972539)
cc @furszy as an author of https://github.com/bitcoin/bitcoin/pull/28894.

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1603176078)
Ok, I think I get the different mechanics involved here now and think if initially wiping an index is going to change, it should be done in a separate pull request. Will revert this change.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2115000215)
> I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.

I (locally) split the commit into (1) update on validation interface callback and asserting that `hashRecentRejectsChainTip` is equal to the chain tip whenever we call `AlreadyHaveTx` + (2) removing `hashRecentRejectsChainTip`. I think if we see that everything runs fine with (1) it's probably correct.

I've (locally) hit a bug though, so will figure out
...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2115009784)
Updated b47bd959207e82555f07e028cc2246943d32d4c3 -> b47bd959207e82555f07e028cc2246943d32d4c3 ([noGlobalReindex](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex) -> [noGlobalReindex](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex..noGlobalReindex))

* Reverted previous change in init code that would no longer wipe the indexes when restarting in the middle of a reindex without the `-reindex` fl
...

💬 dominicusadinfinitum commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2115020459)
@hebasto
where or how do I get to the debug.log?

💬 TheCharlatan commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115032923)
Concept ACK

Could this be a case for a clang-tidy plugin?

💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1603228739)
yes, we should be able to remove that TODO:

I used `n0` instead of `n1` because if we use `n1` it will properly work because `n1` is not synced up yet. So I used `n0` to `loadtxoutset` of a chainstate which is certain to be older than its current chainstate

💬 rkrux commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1603247602)
I see, though it seemed unusual initially that we are loading a snapshot in the same node. But now that I think more about it - this can happen in a real world scenario as well, right? Eg: When the node is re-started after a long time and needs to catch up to the latest blocks.

👍 rkrux approved a pull request: "test: Added test to ensure log and failure happen when work is less than active chainstate"
(https://github.com/bitcoin/bitcoin/pull/30105#pullrequestreview-2060598738)
tACK [ac1b0e3](https://github.com/bitcoin/bitcoin/pull/30105/commits/ac1b0e3ee9045bfa127b7a4bc291cacc98134a18)

There's a lint error though here, but otherwise LGTM.

📝 furszy opened a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122)
Simple fix for #29816.

Instead of re-using a single directory to create a new wallet on each round, use a different one.
The benchmarking framework runs on top of the test framework, which cleans up the entire
directory after completion anyway.

💬 furszy commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115180695)
@hebasto, #30122

💬 apoelstra commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2115191801)
I'd be fine with that, though I'm not sure that there's a privacy loss to just throwing that extra info into the RPC.

At the very least, the RPC ought to indicate whether the wallet is *compiled* since that affects which RPC calls are available.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1603304486)
In "add bench for ShouldFanoutTo" 3f59bf83a41b7787f9c43c277b5e62f327a72c3e: `net.h` include seems unnecessary.

💬 fanquake commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115216469)
The native Windows CI (https://github.com/bitcoin/bitcoin/actions/runs/9112695910/job/25052498545?pr=30122#step:24:64) is failing with:
```bash
Run src\bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: Process completed with exit code 1.
```