💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687159045)
While reviewing (and before reading this discussion) I also got stuck on this. Because of the expectations the naming raises, it seems like a potential footgun (future code may try to iterate over the linked list without intending to erase entries).
I think the side effect should be documented more clearly for the `Next()` function, maybe even rename to `NextAndMaybeErase()` or something similar.

💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687135107)
I think that a call to `coins_view_cache.clear()` is necessary here since `BatchWrite()` doesn't erase entries anymore when `will_erase` is set.

💬 mzumsande commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2243874664)
CI failure seems unrelated (#29897)

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687182754)
It would have to be `coins_map.clear()`, which is passed up in the cursor to `coins_view_cache`. This block is equivalent to `Flush`, and `coins_view_cache` is the `base`.
However, `coins_map` goes out of scope right after this block and is not reused as in `Flush`, so clearing it is not necessary.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687184627)
An earlier version of this PR that relied on checking for null did do this. I just think it's unnecessary work to set them to null, because access is guarded by `m_flags`.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1687185915)
This comment thread is pointing to an out of date version of the PR. There's more documentation on this struct and methods now, as well as changing a variable and field name. Maybe it is more clear now?

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2243900791)
> This is a helpful summary, but not 100% accurate. `uint256S()` does currently require a terminator, but it doesn't have to be a `\0` terminator. Any character other than `01234567890ABCDEFabcdef` will work as a terminator and stop reading the string.

(Updated summary slightly).

👍 ryanofsky approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2192663043)
Code review ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2

Main change since last review is fixing an escaping bug https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742656, but also includes a few other review suggestions

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1687214774)
In commit "logging: Apply formatting to early log messages" (558df5c733d31456faf856d44f7037f41981d797)

Probably better to leave alone since this affects an edge case of an edge case, but just observing that this code could increment m_cur_buffer_memusage to count the usage more accurately.

🤔 theStack reviewed a pull request: "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending"
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2192667951)
Concept ACK

💬 theStack commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687218507)
nit: these two lines could just be removed, as the solutions array's purpose is to contain the extracted non-fixed parts of an output script. For a P2A output, no such parts exist, and there is imho no value in returning constants.

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687314098)
done

💬 davidgumberg commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2244097764)
Re-reviewed and manually tested ACK https://github.com/bitcoin/bitcoin/commit/45958d595b70222c89c25768f357a2ebbafac2b1

It should be easy to grep for the reason that a peer got disconnected, and logging the peer's address when `fLogIPs` seems helpful.

Tested by running:

```bash
./bitcoind -datadir=/tmp/trash -debug=net -logips
```

made `fNetworkActive == false` by:

```bash
./bitcoin-cli -datadir=/tmp/trash setnetworkactive false
```

<details>

<summary>Got the following ou
...

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328626)
added some documentation

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328664)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328677)
switched to txid since that's how it's being tracked

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328696)
documented

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328715)
done

💬 davidgumberg commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1687338802)
nit: Maybe this should move to the top of the function and be used in the other three disconnect logs here
```diff
if (!ShouldRunInactivityChecks(node, now)) return false;
+const std::string disconnect_msg{...};
```

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687367154)
ended up adding it since its required if we aren't returning the "solutions" vector