💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598792320)
it's >= 992

💬 whitslack commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2108281855)
> I had a PR somewhere where I removed the copy constructor and made it mandatory to call a `.copy()`. Then you'd get a compile error wherever a copy happens, and then can either use `std::move` or really use `.copy()` if a copy is needed.

Don't do that. You'll break guaranteed copy elision, which requires the compiler to skip the construction of a temporary object in certain expressions and statements but can only happen if an accessible copy constructor exists (even though it is not called)
...

💬 theuni commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108284477)
Actually, looks like this could use a `txs.reserve(block.vtx.size())` as well.

🤔 instagibbs reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2053233269)
reviewed through 16483fee7c6d93722bfb79fce9efbe841ec13d6a

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598780909)
Hondest

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598790197)
I don't think either of these need to be `PeerTxRelayer`, just use `P2PInterface` and pass in `wtxidrelay` to remove the custom `on_version` handshake?

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598792832)
weight no "size"? :pleading_face:

💬 TheCharlatan commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#discussion_r1598809395)
Do you want to preserve the comment or the scoping? I was on the fence about this, but decided to drop the scoping after the introduced RAII wrappers for the ECC context, which don't scope either: https://github.com/bitcoin/bitcoin/pull/29252/files#diff-dbfadb3e0332664bff298a329b1d27065d2decbbe43fd391388af18f5861c114R17

Besides, I don't think it is likely that his function will grow. It is finally scoped in a reasonably limited way now. If a more functionality is added, it should probably go
...

👍 hebasto approved a pull request: "rpc: move UniValue in blockToJSON"
(https://github.com/bitcoin/bitcoin/pull/30094#pullrequestreview-2053302033)
ACK b77bad309e92f176f340598eec056eb7bff86f5f, I have reviewed the code and it looks OK.

I would be great to check all `UniValue::push_back` and `UniValue::pushKV` invocations in the codebase.

🤔 sr-gi reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-2053304800)
Light code review.

I feel like I'm missing something here though. Before the changes of this pull, the fixed seeds would be pulled after a minute of trying DNS seeds and not reaching the desired peer goal. However, after this pull `ProcessFixedSeeds` is called sequentially after `QueryDNSSeeds`. And the latter has no exit logic based on how long that has been running, meaning that if both are set `ProcessFixedSeeds` will never trigger (?).

💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598813187)
In c1be7952cff9669b87e5002dcbc990dbf726c06f

Isn't `add_fixed_seeds` unconditionally true now? This is called only by:

```
if (gArgs.GetBoolArg("-fixedseeds", DEFAULT_FIXEDSEEDS)) {
ProcessFixedSeeds(start_time);
}
```

💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598815010)
In c1be7952cff9669b87e5002dcbc990dbf726c06f

Given this is called right after `QueryDNSSeeds()` conditionally to `-dnsseed` being set, we could make this method take it instead of re-parsing it here.

💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598833951)
In c1be7952cff9669b87e5002dcbc990dbf726c06f

This can also be passed as a param. We are parsing it both here and in `CConnman::QueryDNSSeeds()`

💬 sr-gi commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1598838227)
In c1be7952cff9669b87e5002dcbc990dbf726c06f

A timer is used both for `QueryDNSSeeds` and `ProcessFixedSeeds` (to wait for 30 secs on the former, and 1 min on the latter). The timer is implicitly shared, we are setting it before entering `QueryDNSSeeds` and passing it to `ProcessFixedSeeds`. However, for `QueryDNSSeeds` we parse the time again inside the method. This should not be necessary. `start_time` could also be passed here.

📝 theuni opened a pull request: "crypto: disable asan for sha256_sse4 with clang and -O0"
(https://github.com/bitcoin/bitcoin/pull/30097)
Clang is unable to compile the Transform function for that combination of options.

Fixes #29801.

💬 theuni commented on issue "Compilation failure with `-O0` + `-fsanitize=address` due to inline asm":
(https://github.com/bitcoin/bitcoin/issues/29801#issuecomment-2108512757)
@ajtowns Thanks for the reminder here. See #30097 for a function-level application of the above.

💬 theuni commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2108514077)
Ping @achow101. Sorry this took so long!

💬 edilmedeiros commented on issue "contrib/signet/miner: miner won't consider --nbits parameter":
(https://github.com/bitcoin/bitcoin/issues/30091#issuecomment-2108523883)
@ajtowns Thanks for the explanation, the miner behavior makes a lot more sense now.

I did a "quick" experiment this morning: started a new signet with the first block starting a month ago. My CPU went close to 100% using all available cores.

<details>
<summary>At first, it was mining a few blocks a second.</summary>

```
2024-05-13 13:06:47 INFO Mined block at height 2; next in -717h26m32s (mine)
2024-05-13 13:06:47 INFO Mined block at height 3; next in -717h24m2s (mine)
2024-05-13
...

💬 ryanofsky commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652)
re: https://github.com/bitcoin/bitcoin/pull/29346#issue-2105856059

> We could decide to not support Noise encryption at all and require users to install separate software for that. Code review of this PR could help inform that decision.

I'm just starting to look at this PR and #29432, and having separate software to handle the encryption and networking does seem like a pretty appealing approach.

However, even if the goal is to directly add support for the stratum protocol to bitcoin cor
...

💬 hebasto commented on issue "make cov fails with lcov-2":
(https://github.com/bitcoin/bitcoin/issues/28468#issuecomment-2108530787)
> > Mind sharing it?
>
> https://github.com/fanquake/bitcoin/tree/support_lcov_2.

To work without `--enable-lcov-branch-coverage`, it should be:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -842,12 +842,14 @@ if test "$use_lcov" = "yes"; then
[AC_MSG_ERROR([lcov testing requested but --coverage linker flag does not work])])
AX_CHECK_COMPILE_FLAG([--coverage],[CORE_CXXFLAGS="$CORE_CXXFLAGS --coverage"],
[AC_MSG_ERROR([lcov testing requested but --coverage flag does n
...