💬 Sjors commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361722871)
Since we can longer create a legacy (bdb4) wallets and since older Bitcoin Core versions can't load descriptor (sqlite3) wallets, it should be safe to drop certain older nodes from the _downgrade_ tests.

Will study this PR later.

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1767332507)
> > Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... _shrug_
>
> Fixed in #30485 (but not yet removing the `m_started_new_line` to avoid a merge conflict here). I'll do it in a follow-up

Removed the dead `m_started_new_line` in https://github.com/bitcoin/bitcoin/pull/30929

⚠️ Sjors opened an issue: "cmake: multiprocess guix build broken"
(https://github.com/bitcoin/bitcoin/issues/30931)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Unable to make a guix build with MULTIPROCESS=1.

### Expected behaviour

To be able to.

### Steps to reproduce

Checkout https://github.com/bitcoin/bitcoin/commit/e821f0a37a026fa0480c7f6f6c938da7c77e0d52 (or master).

In guix/libexec/build.sh below `make -C depends` add `MULTIPROCESS=1` (see #30483 for why). Commit it.

Then do:

```
MULTIPROCESS=1 HOSTS=
```

The build will
...

💬 l0rinc commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767355895)
thanks for clarifying

👍 l0rinc approved a pull request: "util: refactor: add and use run-time safe tinyformat::try_format"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2316356184)
utACK 6dc6e4583c02f0d21dffeb6b2c81358b49a1f340

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1767021502)
nit: would it make sense to add a bool comment for these params?
```suggestion
void BaseIndex::FatalErrorf(util::ConstevalFormatString</*force_trailing_newline=*/false, sizeof...(Args)> fmt, const Args&... args)

```

🤔 l0rinc reviewed a pull request: "log: Enforce trailing newline at compile time"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2315860535)
concept ACK

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1767019782)
nice

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1767438100)
Even better

👍 hodlinator approved a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921#pullrequestreview-2316578818)
ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7

Reuses the existing `HasReason`-idiom in readable way while also being somewhat more efficient memory-wise thanks to the `HasReason`-ctor change.

nit is back: https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765024172

💬 pinheadmz commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1767494576)
I'm seeing something a bit unexpected here which is that the IP address passed to `rpcconnect` is still respected even though the port is ignored / overridden by `rpcport`. This came to my attention in the `interface_bitcoin_cli` test while working on another branch where I'd borked ipv6. This line in the test unexpectedly tried to connect to bitcoin on `::1` even though the port passed to `rpcconnect` was junk:

```
assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport
...

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