🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2316706191)
Code review ACK 38648f4940eb13ebc858081c63e587156428107a

💬 furszy commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1767571027)
nit: would be good to mention this backup is from before the snapshot in the logging line. And maybe perform few extra checks after wise? maybe the resulting balance (this last point could be left for another PR too)

💬 jonatack commented on pull request "blockstorage: Avoid potential Memory Leak":
(https://github.com/bitcoin/bitcoin/pull/30932#issuecomment-2362282633)
@LMAO798 the validation_chainstatemanager_tests unit test file is failing with your patch. You should be able to reproduce the issue locally by building and then running:

```
./build/src/test/test_bitcoin --run_test=validation_chainstatemanager_tests
```

Would the following work instead?

```cpp
const auto [mi, inserted]{m_block_index.try_emplace(hash)};
if (!inserted) {
return &mi->second;
}
CBlockIndex* pindex = &(*mi).second;
pindex->phashBlock = &(
...

📝 hodlinator opened a pull request: "test: Prove+document ConstevalFormatString/tinyformat parity"
(https://github.com/bitcoin/bitcoin/pull/30933)
Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don't.

Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.

💬 mzumsande commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2362307226)
> IIUC(? could be off-base), this could also fire at tip if there are no hb peers and the first peer requested stalls out for 30s? Once parallel compact blocks is warmed up, the slots will likely be taken by those efforts.

Yes, I agree that it should also happen at the tip, both in the cases of low-bandwidth compact relay and in the case of no compact block relay - which seems like a good thing in most cases?! However, one possible downside might be that if you run with an extremely slow conn
...

💬 pablomartin4btc commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1767686583)
I'll do this on the follow-up where I'll be adding the validation from the 2nd commit that was dropped from this PR.

👍 pablomartin4btc approved a pull request: "Fix linking when configured with `-DENABLE_WALLET=OFF` (Qt 6)"
(https://github.com/bitcoin-core/gui/pull/837#pullrequestreview-2316891331)
tACK 5be34bacf6d07fb73a0cedfb63a384968d538f3e

It builds correctly on Qt5 and qt runs fine, with both `-DENABLE_WALLET=OFF` & `-DENABLE_WALLET=ON`. When `wallet support` is `off` also tested the rpc console and several rpc commands.

💬 pablomartin4btc commented on pull request "Fix linking when configured with `-DENABLE_WALLET=OFF` (Qt 6)":
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1767704977)
I think because the compiler only needs to know WalletModel, it doesn't need the full definition unless you dereference the pointer (eg as in `RPCConsole::RPCParseCommandLine` -> `wallet_model->getWalletName()` - which is guarded).

🤔 andrewtoth reviewed a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2317044728)
I think the commit title `test: Migrate GetCoinsMapEntry to return std::optional<CoinState>` is no longer accurate.

I didn't find splitting the changes in `coins_tests` up into that many commits to be helpful for review. I ended up reviewing the combined diff for that file. Will review in more depth later.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767813420)
I think this should be `Assume`. We should prefer it since it will be compiled out for release builds and this is a hot path.

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767815543)
Hmm not sure we should remove the extra context here about what exactly `self` and `sentinel` are and where they come from.

🤔 tdb3 reviewed a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2317070761)
Approach ACK

Nice improvement.
Performed a quick initial code review and exercised the changes interactively running on signet. I plan to circle back.

✅ maflcko closed a pull request: "blockstorage: Avoid potential Memory Leak"
(https://github.com/bitcoin/bitcoin/pull/30932)

💬 maflcko commented on pull request "blockstorage: Avoid potential Memory Leak":
(https://github.com/bitcoin/bitcoin/pull/30932#issuecomment-2362882553)
Closing as a low-quality (and obviously wrong) LLM generated bot/spam patch, with an (obviously wrong) hallucinated explanation, without any test coverage or otherwise steps to test, and finally test failures in the existing tests.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048796)
Not sure about adding those. The whole point of the previous pull requests was to remove this linter (https://github.com/bitcoin/bitcoin/issues/30530). Now having follow-ups (https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547) that make it harder, albeit minimally, seems a step in the wrong direction.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048525)
not sure about removing this. Apart from Wunused, it is also required to be detected in coverage reports (which will obviously omit consteval stuff)

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768047784)
style nit (feel free to ignore): I suspect you could reduce verbosity and having to hand-format all of this by using `tuple_cat`, see https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304

This should also ensure that all cases are covered and wouldn't require to change the tests vectors.

👍 vasild approved a pull request: "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)"
(https://github.com/bitcoin-core/gui/pull/836#pullrequestreview-2317404454)
ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2

👍 zaidmstrr approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2317472339)
Code review ACK [e6994ef](https://github.com/bitcoin/bitcoin/commit/e6994efe08b282dd9e46602bcbad69567fe91dcd)
All the things seem correct as before.

📝 maflcko opened a pull request: "ci: Approximate MAKEJOBS in image build phase"
(https://github.com/bitcoin/bitcoin/pull/30935)
The `MAKEJOBS` env var is the default in image builds, which is fine, because it is only relevant when building msan (or iwyu) and only differs when setting MAKEJOBS to something other than `nproc` (currently used as an approximation).

So the normal workflow of `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` already works today.

However, `MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` does not.

This is
...