👍 Dezzj approved a pull request: "ci: enable AArch64 target in MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27824#pullrequestreview-1476494235)
Approved

💬 hebasto commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1227719150)
> Are we actually bumping into overflow issues with `int32_t` somewhere for size with descendants?

I don't think so.

However, UBSan raises "signed-integer-overflow" warnings about `nSizeWithAncestors += modifySize;` etc.

📝 fanquake converted_to_draft a pull request: "net: introduce block tracker to retry to download blocks after failure"
(https://github.com/bitcoin/bitcoin/pull/27837)
Built on top of #27836. Coming from #27652. Implementing the second part of it.

The general idea is to keep track of the user requested blocks so, in
case of a bad behaving peer or a network disconnection, they can be
fetched from another one automatically without any further user interaction.

This was requested by users because the `getblockfrompeer` RPC command
lacks the functionality to notify them about block request failures or peer
disconnections (which is expected due to the asy
...

💬 hebasto commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588809598)
Friendly ping @MarcoFalke and @sipsorcery :)

✅ fanquake closed an issue: "tsan: failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/issues/27860)

🚀 fanquake merged a pull request: "test: fix intermittent failure in p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/27864)

💬 MarcoFalke commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588852287)
No objection to using the cache to avoid intermittent network issues. But the speedup was only ~2 minutes on a total runtime of 1 hour?

💬 hebasto commented on pull request "ci: Use docker image cache for "Win64 native [vs2022]" task":
(https://github.com/bitcoin/bitcoin/pull/27771#issuecomment-1588861202)
> No objection to using the cache to avoid intermittent network issues. But the speedup was only ~2 minutes on a total runtime of 1 hour?

You're right. I've added "avoid intermittent network issues" to the PR description.

💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1227790565)
shouldn't the messages by checked by `assert_start_raises_init_error` instead of reading the debug log?

🤔 MarcoFalke reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1476651234)
left a question / test nit for follow-up

💬 MarcoFalke commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1227794716)
In 3b2c61e8198bcefb1c2343caff1d705951026cc4:

Any reason to make `expected_ret_code` optional? Seems easier to just make the default value `0` and drop this whole `else` block.

💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227810641)
Thank you. Yes, I agree that adding it at the end of `fee_estimates.dat` would indeed reduce redundancy. I would update that.

Regarding making it conditional to read the persisted `mempoolminfee`, could you please clarify the scenarios in which we might want to use the default `mempoolminfee` value? In this, it is already conditional to remember the `mempoolminfee` as stale `mempoolminfee` will not be read at all.

💬 MarcoFalke commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1588895527)
lgtm ACK 76c5ea703e77d580b6962e60398f4988cbd9b58b

💬 furszy commented on pull request "net: introduce block tracker to retry to download blocks after failure":
(https://github.com/bitcoin/bitcoin/pull/27837#issuecomment-1588916036)
Oh, didn't know about the CI failure. Seems to be just an unused variable. But sure, moved to draft until https://github.com/bitcoin/bitcoin/pull/27836 get merged.

💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1227846644)
It appears that taking the approach of dumping to `fee_estimates.dat` and implementing a method to set `mempoolminfee` based on the persisted value is a better option.

Your perspective on persisting the mempool periodically. I understand the concern about remembering the fee estimates and `mempoolminfee` without the mempool itself. However, I would like to seek further insight into the potential consequences that could arise from this, as dumping the mempool periodically might be unappealing

💬 TheCharlatan commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1589076014)
Rebased d60ab3b5099d3a348f3122b5d3207ac12f4a751c -> 3e60c02ef3759e16a762bd193e0b9291bf46b73c ([kernelInterrupt_0](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_0) -> [kernelInterrupt_1](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInterrupt_0..kernelInterrupt_1)).
* Added `src/node/abort.*` to allow both the notification code and `index/base.cpp` to use a common abort definition as introduce
...

💬 fanquake commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1589076049)
Concept ACK

📝 fanquake opened a pull request: "lint: suppress pip spam"
(https://github.com/bitcoin/bitcoin/pull/27871)
The logs are [currently full of](https://cirrus-ci.com/task/5380096597426176?logs=lint#L240):
```bash
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
WARNING: You are using pip version 22.0.4; however, version 23.1.2 is available.
You should consider upgrading via the '/tmp/python/bin/python3.8 -m pip install --upgrade p
...

📝 fanquake opened a pull request: "build: suppress external warnings by default"
(https://github.com/bitcoin/bitcoin/pull/27872)
I think we are at the point where it make more sense to make this the default, than not. It's already used in the CI, and I assume most building locally are also utilising it.

💬 fanquake commented on pull request "doc: clarify full Xcode download is not needed":
(https://github.com/bitcoin/bitcoin/pull/27835#issuecomment-1589083587)
> Some background, it should indeed not be necessary to install Xcode:

It hasn't been required for at least 7 years, that's why we removed the instructions to install in: https://github.com/bitcoin/bitcoin/commit/2692e1b10bd7a0be644ed8a69c54152bc741e855.

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1227953145)
@ajtowns Right, this is the example I thought of as well. But I don't understand why you think the check is unnecessary. From my understanding, this is the point where the subpackage A+B would fail then so I think it is necessary. It just depends on the fact that the function is called with each subpackage.