💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594255018)
Sjors suggested here https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399137900 that I add a way to do it by transaction hash, which would be easy. Let me know if you have other suggestions...

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1594262607)
See line 528 below: https://github.com/sdaftuar/bitcoin/blob/c49e0444e5c474d5e1ac0af89e9bc27958f3ed31/src/kernel/txgraph.cpp#L528

💬 jadijadi commented on pull request "Keep focus on "Hide" while ModalOverlay is visible":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-2100893822)
Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594266520)
I haven't been able to reproduce the issue in my local setup, but I think we may be able to improve on this (plus reduce the waiting time) by mocking the node's time after restarting it

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1594268166)
Tried that (plus some minimal improvements) in [138a2e2](https://github.com/bitcoin/bitcoin/pull/29605/commits/138a2e2a79846863fdce26102f800117f407ef97)

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

Might be good to add a comment noting that the "jsonrpc" version field was added in the JSON-RPC 2.0 spec, so value "1.0" is nonstandard. But it is accepted for backwards compatibility, because some old documentation (incorrectly) suggested adding it.

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

Variable name `valJsonRPC` doesn't really follow the suggested [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) which calls for snake_case. Could rename it to "jsonrpc_version"

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

Technically JSON-RPC version would be more accurate the JSON version. could s/JSON/JSONRPC/ throughout this enum

👍 ryanofsky approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-2045965827)
Code review ACK 8583c2f93d4eb020bf1e3b74860b691ec12e5eee. Looks very good! I left several comments, but none are critical and they could be followed up later.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594260831)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

This is probably supposed to say 1.1 not 1.2

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594242203)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

I think the commit message has a typo. It says "Non-batch requests already did not return HTTP errors previously." but I think it is supposed to say "Batch requests already did not return HTTP errors previously."

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594211478)
In commit "rpc: refactor single/batch requests" (a64a2b77e09bff784a2635ba19ff4aa6582bb5a5)

re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594034591

HTTP headers use \r\n, but this line is generating the JSON object in the body of the response, after the headers. And the JSON could be written with either \r\n or \n or no trailing line break at all. I think it would probably make sense not to include any trailing line break, but existing code uses \n, so in a refactoring comm
...

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1594282860)
In commit "rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests" (56b6c33646f398e1b2efab64cbaf7ed86e3bbd06)

Returning RPC_PARSE_ERROR does not seem accurate if the request was parsed successfully but there was an error executing it. This was also a preexisting problem in `HTTPReq_JSONRPC` for batch requests, but now the behavior will happen for all version 2 single requests as well. Would suggest cleaning this up with a change like:

```diff
--- a/src/httprpc.cpp
+++ b/src/httprpc.c
...

🤔 glozow reviewed a pull request: "rpc: improve submitpackage documentation and other improvements"
(https://github.com/bitcoin/bitcoin/pull/29292#pullrequestreview-2046138558)
utACK 78e52f663f3e3ac86260b913dad777fd7218f210

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1594328715)
This is going out of scope anyway, no need?

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1594325841)
Will anything ever use this key directly? Or will it always just be turned back into a `secp256k1_keypair`? It seems the CKey output is just a wrapper that causes overhead here. I guess for the sake of not exposing `secp256k1_keypair` to the caller?

💬 ajtowns commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2101001968)
> In the current implementation it's far quicker to jack up the difficulty by 1000x than it takes to drop it. I didn't do the math on the way up, the way down takes 40 weeks (4 week retarget period, each cutting difficulty in half).

If your target difficulty takes you 10 minutes to mine a block on average, and the current difficulty is 0.1% of that, it'd take 10k blocks over about 5 days to increase the difficulty 1000x, or about the same work as you'd need to mine ~690 blocks at the target d
...

🤔 mzumsande reviewed a pull request: "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate"
(https://github.com/bitcoin/bitcoin/pull/30039#pullrequestreview-2046205765)
I've played around with this branch a bit, upgrading and downgrading between it and master with existing datadirs on signet and didn't run into any issues.
Also just noting that this will affect all leveldb databases, also the indexes and the `block/index` db.

🤔 glozow reviewed a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2046213151)
This seems useful, and makes sense as a field of `getmempoolinfo` like the others

💬 glozow commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1594348765)
I slightly prefer it being called datacarriersize to match the config option, but don't feel strongly