💬 glozow commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190081684)
Could this increase the number of transactions we're announcing beyond `INVENTORY_MAX_RECENT_RELAY = 3500` so things fall out of `m_recently_announced_invs` (essentially what you said in https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1189250200)?

Maybe we want to increase `INVENTORY_MAX_RECENT_RELAY`, e.g. equal to `MAX_PEER_TX_ANNOUNCEMENTS = 5000` (the max requesting coming from the receiver end if they're Bitcoin Core - imo it could make sense for these to be symmetrical)?

🤔 pinheadmz reviewed a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610#pullrequestreview-1420860416)
Concept ACK

A few questions below.

I have a non-debug mainnet node with a 100% `b-msghandler` thread. I'm going to deploy this branch today and monitor

💬 pinheadmz commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190049503)
Why stop at 1000? If it's the same rationale could you use this constant?

https://github.com/bitcoin/bitcoin/blob/104eed116614996d9724f82d47e373ef420cd372/src/net_processing.cpp#L109-L110

💬 pinheadmz commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#discussion_r1190081977)
Is there a reason why we can't ignore not-in-mempool txs before we even get to this point?

i.e. here:

https://github.com/bitcoin/bitcoin/blob/104eed116614996d9724f82d47e373ef420cd372/src/net_processing.cpp#L5671-L5673

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934)
In commit "zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier" (6247b3c5e3c9e70097243a85b3d883b279a473cc)

It looks like the lambda is taking a reference to the local `get_block_by_index` variable here, which doesn't seem right, since the variable will go out of scope before the lambda is called. So I think `&` should be dropped here.

Not totally sure though, since I would expect there to be problems in CI if there was a bug.

💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1190100674)
Fixed

💬 glozow commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542429830)
Added a release note and addressed https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1190006006

💬 brandonpille commented on issue "rpc: Allow importing wallets by data instead of by filename":
(https://github.com/bitcoin/bitcoin/issues/27597#issuecomment-1542436043)
No not specifically. I was just wondering why there is an import_wallet rpc that accepts a filename where the file is on the system itself.

💬 LarryRuane commented on pull request "add ryanofsky to trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1542437622)
post-merge ACK

💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190113427)
Yes, no need to do change it in this PR.

💬 achow101 commented on pull request "refactor: Replace global find_value function with UniValue::find_value method":
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1542448136)
ACK fa266c4bbf564308ddbc12653527226506902084

💬 michaelfolkson commented on pull request "add ryanofsky to trusted-keys":
(https://github.com/bitcoin/bitcoin/pull/27604#issuecomment-1542478462)
For posterity: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-May/021618.html

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190136977)
`CZMQPublishRawBlockNotifier` takes `get_block_by_index` by value (meaning instead of borrowing, copies its value), so I think that is why this works out.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098)
Updated 886a473fc48f2c7d67436b5d9ac5643cd007e27f -> e3311d649d9785ca7bc3a29ad7470c7cd69951bb ([removeBlockstorageArgs_24](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_24) -> [removeBlockstorageArgs_25](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_25), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_24..removeBlockstorageArgs_25))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discuss
...

🚀 achow101 merged a pull request: "refactor: Replace global find_value function with UniValue::find_value method"
(https://github.com/bitcoin/bitcoin/pull/27605)

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190102588)
In commit "zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier" (6247b3c5e3c9e70097243a85b3d883b279a473cc)

Would probably be a little more efficient to make the get_block_by_index non-const and use `m_get_block_by_index{std::move(get_block_by_index)}` to move the function instead of copying it.

👍 ryanofsky approved a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1420938880)
Code review ACK 886a473fc48f2c7d67436b5d9ac5643cd007e27f

re: https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1542522098

> * Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934), moving the lambda instead of borrowing.

Actually I think my suggestion won't work. There can be multiple notifiers, so the function needs to be copied into all of them not moved into the first one. ACKed previous version of the PR 886a473fc48f2c7d6
...

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190113760)
In commit "refactor/iwyu: Complete includes for blockmanger_args" (c065e093b1c786476d9bc9769e42c2a54ff68816)

"blockmanger_args" spelling in commit title

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190174923)
re: https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934

Yeah sorry, I saw the "factories" map and assumed the factories would be long-lived, not destroyed before the function returns. This code is pretty strange, because it creates a map and then never looks anything up in the map. Could be cleaned up later, though. Probably a more sane way to write it would be to drop the map, change the outer for loop into an `add_notifiers` lambda, and do

```c++
add_notifiers("pubhashb
...

💬 fanquake commented on issue "GCC 13: `-Wdangling-reference` output":
(https://github.com/bitcoin/bitcoin/issues/26926#issuecomment-1542538866)
Closing, now that #27605 has been merged.