💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1949216375)
>> Apart from that, just as an extra note for the future: It seems that we will need to rearrange code so that these functions aren't called so many times within the same process.
> I'm not sure what this is referring to, so would be curious

@ryanofsky.
An easy example is `CWallet::AvailableCoins`, which calls `CWallet::IsMine` first, few lines later calls `CWallet::GetSolvingProvider` and then calls `CWallet::CalculateMaximumSignedInputSize` which internally calls `CWallet::GetSolvingProvi
...

💬 ismaelsadeeq commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1492915977)
nit:
```suggestion
const auto low_tx = make_tx(/*inputs=*/ {m_coinbase_txns[0]}, /*output_values=*/ {10 * COIN});
```

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-1949222350)
> About the graph in the OP: "... until it finds an address from a network that compose 20% ...". The chance of not getting the result after N tries is (8/10)^N. For example for N=25 that is less than 0.004. What is the explanation of having so many dots so high? Uneven distribution within addrman?

That was what I was expecting too. I'll investigate but yes I think it might be an uneven distribution. Btw, this is not deterministic.

👍 ryanofsky approved a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1885956108)
Code review ACK e041ed9b755468d205ed48b29f6c4b9e9df8bc9f. Just suggested changes since last review

💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1492983890)
> Something along the lines of:
>
> ```python
> def p2p_lock(self):
> assert not self._send_lock.locked()
> return p2p_lock
> ```

I think that this is not right (I tried it out, and it asserts everywhere :sweat_smile:):
To prevent deadlocks, the lock order must be preserved only within each thread:
- It's possible that a given thread first takes `p2p_lock`, and then `send_lock`. See for example: `on_message` -> `p2p_lock` taken -> call `on_inv` -> call `send_message` -> `_sen
...

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1493011255)
You're right. I don't think there is any easy way of keeping track of who's thread is holding what lock in Python though, so I don't think there a straightforward way of addressing this :(

💬 brunoerg commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#issuecomment-1949378356)
crACK dddd7be9bf038c25f3e53c5bd708fb9cf73d4493

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1493013878)
I changed that bit of the commit message to:

```
This patches the aforementioned behavior by going over all resolved IPs until we find one
we can connect to or until we exhaust them.
```

I think that should be clear enough given the variable name, but let me know otherwise

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1493027486)
This was added by me as a suggestion from @vasild in https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296832854 to maintain the older logic, but it comes all the way from https://github.com/bitcoin/bitcoin/commit/6387f397b323b0fb4ca303fe418550f5465147c6#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R430.

There is no comment nor commit message motivating it though.

I think randomizing the order in which the resolved addresses are tried wouldn't hurt though.

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1493029119)
I'm a bit curious about this. I think I'll try to dig into how bad could it be if a malicious DNS seed provides us with 256+ addresses that try to delay the TPC handshake as much as possible

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1949408988)
Thanks for the reviews @mzumsande @vasild and @naumenkogs!

I have some updates pending on the nits provided by @naumenkogs, but I'll wait until we resolve https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1492192396 to push them all. I'd also look into https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1491629378 out of curiosity

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1949439983)
Reviewed 58cb22c.

Great to finally add support for JSON-RPC 2.0, thanks for doing this!

(Super-late concept reflection: it might be more modern to provide something like [OpenAPI](https://swagger.io/) instead of JSON-RPC in the future ([HN discussion](https://news.ycombinator.com/item?id=34211796)). This would facilitate using tools like Postman and more easily signal which requests are cacheable/idempotent etc. On the other hand the codebase already has Cap'n'Proto and ZMQ, so also makes
...

💬 aureleoules commented on pull request "test: add missing tests for Assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-1949561588)
> Looks like [corecheck.dev/bitcoin/bitcoin/pulls/29428](https://corecheck.dev/bitcoin/bitcoin/pulls/29428) is dead?

@maflcko thanks for the ping. The code I added to filter out flaky coverage was too slow and isn't very reliable so I removed it. Unfortunately, the UI will now display flaky coverage.

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1949572923)
Are ocean contributors (miners) complying with the nodes?

💬 ariard commented on pull request "Add imbued v3 based on template-matching":
(https://github.com/bitcoin/bitcoin/pull/29427#issuecomment-1949579407)
one design feedback, look on how any imbuance mechanism would have to fit with dynamic upgrades, whatever the bitcoin use-cases. especially matters for time-sensitive flows: https://github.com/lightning/bolts/pull/1117

💬 mrsteve0924 commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-1949611717)
after 4 years why not close this? obvious no one is going to fix it

💬 ismaelsadeeq commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1493302062)
Maybe weshould also check that we can have more than two directly conflicting transactions as long as they are all in a cluster size <=2.

<details>
<summary>diff </summary>

```diff
diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp
index e15fd29da3..561ebd3881 100644
--- a/src/test/rbf_tests.cpp
+++ b/src/test/rbf_tests.cpp
@@ -431,6 +431,53 @@ BOOST_FIXTURE_TEST_CASE(calc_feerate_diagram_rbf, TestChain100Setup)
BOOST_CHECK(new_diagram[1] == FeeFrac(high_fee, low_size
...

🤔 ismaelsadeeq reviewed a pull request: "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2"
(https://github.com/bitcoin/bitcoin/pull/29242#pullrequestreview-1886534107)
Thanks for adding the test!
re-ACK d57fbda350ee9051931d9a0dad4beb55f6b2e574

💬 ismaelsadeeq commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1493302276)
here an another place below!

💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1949937829)
Yeah, good point. So I guess the only way to remove unused includes would be to write another check in the linter. I'll drop the last commit and leave everything else for a follow-up.