💬 hodlinator commented on pull request "p2p: Correct unrealistic headerssync unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3287698877)
Latest push resolves conflict with #33332.

(Also changed PR-title (category) from "headerssync:" to "p2p:" based off https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/config.yml and the fact that #25717 used that topic. Maybe one could advocate for expanding the number of categories, but I'll punt on it for now. Hat-tip to l0rinc for the DrahtBot reference).

💬 fanquake commented on pull request "Bash completion for the new bitcoin wrapper command":
(https://github.com/bitcoin/bitcoin/pull/33383#issuecomment-3287721141)
@caesrcd It's no-longer possible to reopen this PR, as you've pushed to the branch since it was closed. You'll need to open a new (third) one. Apologies about the bot. You can see the kind of heuristics it's using to determine PR closures, here: https://github.com/maflcko/DrahtBot/blob/14ccfd94bc7cc584e547e09582373cb87a0e4553/webhook_features/src/features/spam_detection.rs#L118.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346045827)
Initially it was not needed, because we always retrieve it from a chainstate manager with a loaded chainstate, meaning the chain will never be empty, so there is no possible race condition. I think it might be more defensive to take the lock now, since we could permit `Chain` data structures that are empty in the future. In the `get_by_height` case we take the lock such that there is no race condition between the assert and retrieving the entry.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346045962)
This should just be removed, because as you point out in the comment below, it already is atomic.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2346046086)
Thanks for noticing this! I think it is indeed wrong to add the lock in `enable_category`. For the other logging related functions: I'd also rather use a dedicated logging global mutex for the call to `set_level_category`, but also don't feel keen on introducing another global for this purpose.

💬 fanquake commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3287890168)
https://github.com/bitcoin/bitcoin/actions/runs/17680889551/job/50254583517?pr=33378#step:12:357:
```bash
D:\a\bitcoin\bitcoin\src\test\sock_tests.cpp(37): Entering test case "constructor_and_destructor"
2025-09-12T17:23:00.992709Z [unknown] [D:\a\bitcoin\bitcoin\src\test\util\random.cpp:48] [void __cdecl SeedRandomStateForTest(enum SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=936d3154550e893420e302712f2c7f1031a35cebf124c36896c0cee3e5c2ece2
2025-09-12T17:23:00.994377Z
...

💬 cedwies commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#issuecomment-3288020105)
re-ACK 88b0647

💬 BrandonOdiwuor commented on pull request "ci: use Mold linker for asan-lsan-ubsan-integer-no-depends-usdt workflow":
(https://github.com/bitcoin/bitcoin/pull/33370#issuecomment-3288151583)
@fanquake I updated the PR and moved the changes to `00_setup_env_native_asan.sh`

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2346506939)
I think this type of p2p change is something we should discuss separately, outside of this PR, because it's not clear to me that there's a simple way to make a strict improvement in relay behavior. And the interpretation of what the `feefilter` message means/should mean is something we should document and coordinate across implementations as well.

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3288247205)
> Approach ACK on using siphash and `FlatFilePos`.
>
> Do you want to squash the commits? This just feels like an improvement in every regard.

Rebased and squashed in https://github.com/bitcoin/bitcoin/commit/845b54defebc1987b9d654ea7b611a4ddebe345a. I also updated the PR description.

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2346522367)
@instagibbs @sipa I think switching from `TxGraph::Ref()` to something like `/* ref = */ {}` is not worth the change, as the existing code seems just as good from a self-documentation standpoint. Will plan to leave as-is unless there's some benefit from invoking `{}` that I'm missing?

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2346524631)
It was just a nit, in case you weren't aware that it was possible to do this more succinctly. Agreed that it's more self-documenting as is.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2346526733)
Sorry, ignore this. I misread `ConsumeTxMemPoolEntry` as being the constructor, rather than a separate function.

💬 sipa commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2346533797)
I don't think it is; TxGraph should be pretty efficient at gathering all ancestors.

📝 pythcoiner opened a pull request: "descriptor: fix comments in descriptor.cpp::DescriptorImpl"
(https://github.com/bitcoin/bitcoin/pull/33384)
`P2TR` was missing in `m_pubkey_args` description and there was a typo in `doc/descriptors.md`

💬 pythcoiner commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2346604199)
nit: should we keep this debug log?

💬 pythcoiner commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2346586549)
nit: not sure if those includes are stricly required?

💬 pythcoiner commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2346503278)
nit (for consistency)
```suggestion
/** Semantic/safety warnings (includes subdescriptors). */
```

💬 HowHsu commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#discussion_r2346751467)
Hi l0rinc,
I’ve updated the code. Since I’m not very familiar with the Bitcoin Core workflow, I wasn’t sure if it’s appropriate for me to mark this conversation as resolved, but I went ahead and did so. Please feel free to reopen it if that’s not the right process.

💬 Raimo33 commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3288478820)
it's not "a little bit of faithfulness" that you sacrifice. you're ignoring the cache completely.