👍 ryanofsky approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2058024841)
Code review ACK 27af3910e5fa9915d4913b82443db216d1b1d61b

💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601650396)
In commit "[[refactor]] Check CTxMemPool options in constructor" (27af3910e5fa9915d4913b82443db216d1b1d61b)

I think it would be clearer to avoid using && and std::move, since using these doesn't actually make things more efficient. In 27af3910e5fa9915d4913b82443db216d1b1d61b, one copy and one move of the option struct will happen. The copy happens when calling the CTxMemPool constructor (since the parameter type is a value parameter), the move happens when returning from SetOptionLimits.

B
...

💬 theuni commented on pull request "crypto: disable asan for sha256_sse4 with clang and -O0":
(https://github.com/bitcoin/bitcoin/pull/30097#issuecomment-2112603762)
> Could you like to that thread from the comment added to the source, then this is probably good to go.

Done

✅ willcl-ark closed a pull request: "Use shared_ptr for CNode inside CConnman"
(https://github.com/bitcoin/bitcoin/pull/28222)

💬 willcl-ark commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-2112616409)
Closing for now

🚀 ryanofsky merged a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000)

💬 ryanofsky commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2112626070)
I merged this as-is since it has so many acks, but it probably makes sense to follow up on the last few comments and suggestions.

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601722461)
I was just re-using the pattern used for the `ChainstateManager` construction, but I like this more. Taking.

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2112657721)
Thanks for the reviews, going to take @ryanofsky's suggestion, hope it is not much work to review again

Updated 27af3910e5fa9915d4913b82443db216d1b1d61b -> 33f2a708e392edb2501f555efef34a558da9d717 ([mempoolArgs_8](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_8) -> [mempoolArgs_9](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_8..mempoolArgs_9))
* Addressed @ryanofsky's [comment](https://github.com
...

📝 glozow opened a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111)
See #27463 for full project tracking.

This contains the first 4 commits of #30110, which require some thinking about thread safety in review.
- Introduce a new `m_tx_download_mutex` which guards the transaction download data structures including `m_txrequest`, the rolling bloom filters, and `m_orphanage`. Later this should become the mutex guarding `TxDownloadManager`.
- `m_txrequest` doesn't need to be guarded using `cs_main` anymore
- `m_recent_confirmed_transactions` doesn't need it
...

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1601750695)
This commented line maybe requires more explanation?

👍 ryanofsky approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2058165341)
Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601733952)
In commit "rpc: identify JSON-RPC 2.0 requests" (2ca1460ae3a7217eaa8c5972515bf622bedadfce)

I think it would be a little clearer to say "continue to accept that" instead of "maintain that." Otherwise it sounds like we are trying to maintain incorrectly including the field, not just allowing it if it is specified.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601741212)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1601581136

> Would have gone the opposite way and called it `throw_errors` since it is an.. exception.. to maintain legacy behavior. Sorry for not catching that earlier.

I think either way is fine, but catch_errors does seem more literally correct since the argument is just controlling whether the exceptions will be caught. Errors will be still be thrown regardless.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2112708036)
@willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack

👋 furszy's pull request is ready for review: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574)

💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601790030)
> I was just re-using the pattern used for the `ChainstateManager` construction, but I like this more. Taking.

Oh, that's interesting. Looking at that, I can see how it's better than what I suggested because it allows callers to potentially move options without copying them at all. Current init code does copy the options because it is looping, but we don't need to require other callers to copy as well.

So you might want to keep this code like it is, though I would still suggest combining e
...

💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2112753291)
> @willcl-ark, @tdb3, any interest in re-acking? This seems like it could definitely be merged with another ack

Definitely. I'll plan to take a look tonight.

💬 willcl-ark commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2112786215)
You benchmark looks good and reports the result I'd expect too!

I'll investigate my own measurements some more in the mean time...

📝 0xB10C opened a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112)
The Bitcoin Core RPC interface [is implicitly versioned on the major version of Bitcoin Core](https://github.com/bitcoin/bitcoin/blob/42d5a1ff25a8045b6f4c09fa1fb71736dbccc034/doc/JSON-RPC-interface.md?plain=1#L67-L68). Some downstream RPC consumers and clients, for example rust-bitcoincore-rpc, need to know about the underlying node version to determine the available RPC calls and how to interpret the RPC responses (e.g. https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/355).

The [
...