💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1598772485)
done

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1598773210)
probably a good idea, though you might have better opinions on orphanage interface. I tightened the time testing a bit more by setting mocktime at beginning, then checking boundary conditions via subsequent setting of the time.

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2108249084)
Ah, p2p_invalid_tx.py failed because `assert_debug_log` didn't match anymore (those should be unit tests!)

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