💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1949144664)
With https://github.com/stratum-mining/stratum/pull/737, https://github.com/stratum-mining/stratum/pull/752, https://github.com/stratum-mining/stratum/pull/729 and https://github.com/stratum-mining/stratum/pull/722 merged this PR is now compatible with SRI's `main` branch. This should make testing a lot easier.

🤔 furszy reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1885842494)
Reviewed. Need to update the second commit description.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492891474)
In 65c7997221:

This is not needed. The benchmark uses a db mock.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492899741)
In bf898af0190d:

It was removed on the last push: `LoadDescriptorScriptPubKeyMan` now returns the spkm but no function utilizes it.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1949200230)
Possibly relevant: the test introduced here, and not modified later, failed at least once in the parent PR: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1948481906

👍 hebasto approved a pull request: "[26.1] final changes for 26.1rc1"
(https://github.com/bitcoin/bitcoin/pull/29440#pullrequestreview-1885871860)
re-ACK 1e7fb270d36310efff6fc968f1c52291043d461b

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492910941)
Removed

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492911131)
Oops, somehow that usage got dropped. Fixed.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1949209816)
> Need to update the second commit description.

Done

💬 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_r1492915210)
Yes, I can't think of a scenario where that will happen!

💬 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