💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1598738005)
Thanks all for the suggestions. Pushed an update.
Since locally I have x86_64 (and not arm), I tried configuring/compiling with `--enable-werror` added to `./configure` (to minimize excess CI churn). Interestingly enough, this didn't result in the same warning as seen on arm CI.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598741307)
I'll try that with 992 once I rebuilt the database. Is `dustLimit` `>= 992` or `> 992`?

💬 theuni commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#discussion_r1598744518)
A few suggestions:
- Looks to me like `txNew.vout` is always empty here, so I'm not sure why we'd bother to add `txNew.vout.size()` unless I'm missing something ?
- There's an additional likely insert for change. Since the reserved size here is exact, that could trigger a reallocation up to the next power-of-two. Makes sense to make this `vecSend.size()+1` just in case.
- I think the txouts could be `std::move`d (in both places) instead to save a copy similar to #30094?
- Any reason not `.re
...

🤔 theuni reviewed a pull request: "refactor: reserve memory allocation for transaction outputs"
(https://github.com/bitcoin/bitcoin/pull/30093#pullrequestreview-2053171781)
Concept ACK

👍 ryanofsky approved a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2053188813)
Code review ACK 04ffe4061da2d0135f206032e167703772b5da78, this is an obvious improvement

💬 ryanofsky commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#discussion_r1598754079)
In commit "kernel: Remove batchpriority from kernel library" (04ffe4061da2d0135f206032e167703772b5da78)

I think it might be useful to keep the `// End scope of ImportNow` comment here. If someone is adding new code at this point, it's possible they might want to add it with importing set to false, or importing set to true, so the comment would be a reminder that the value will change at this point.

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

💬 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 (?).