💬 laanwj commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1767502902)
Would be nice to add an assertion / check here to make that fileOutPos is within range of unsigned int before casting it, to avoid silent truncation. But yes, out of scope of this PR as the conversion isn't introduced here.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767504341)
Okay, so `FRESH` and `DIRTY_FRESH` will merge into one. :+1:

💬 pinheadmz commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1767508661)
on second thought, this behavior does make sense if a user is crazy enough to pass `IP:port` and then a second `port`

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767510588)
Like it! Moves related things closer together.

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1767512060)
I'll do it with the next push, thanks!

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767514943)
rather `FRESH` will be removed in the mentioned follow-up, since it's not actually possible (unless in case of reorgs, which I don't fully understand yet, but not important for this PR)

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767516058)
The usages are a lot more verbose, but I've extracted everything important now, so the tables are still readable

📝 LMAO798 opened a pull request: "blockstorage: Avoid potential Memory Leak"
(https://github.com/bitcoin/bitcoin/pull/30932)
Issue:
-> Potential Memory Leak in BlockManager::InsertBlockIndex
Reason:
-> The InsertBlockIndex function creates a new CBlockIndex when try_emplace is successful, but doesn't guarantee that the object will be released if an error occurs later. This can lead to memory leaks potentially.

📝 andrewtoth converted_to_draft a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673)
Following up from https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509, which suggested a revival of #18746.

`GetCoin` will never return `true` for a spent entry, so we can safely assume that any entry we fetch will not be spent. This lets us remove the only non-test code path which adds a FRESH-but-not-DIRTY entry to the flagged linked list. This in turn ensures all entries being sent to `BatchWrite` are `DIRTY` entries.

A corollary is that all spent coins must be DIRTY. T
...

🤔 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.