💬 ryanofsky commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1662982423)
re: https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658934242

> I did this now, but was not completely sure where to put the nonce generation.

For reference, this was implemented in 45e0c96e81b5f0c364af717b7cb2d9bc094372de but later dropped from the PR

💬 ryanofsky commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1662892902)
In commit "kernel: De-globalize script execution cache hasher" (468c98c8e083c68194422fc1ffc7fd77f4e0383d)

re: https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645009465

Marked resolved because the main concern that `m_script_execution_cache_hasher` was a public member that could be modified unintentionally has been resolved. Now it is private with a const accessor.

I agree with stickies it would make code more self-documenting and modular if the private member were const and i
...

💬 maflcko commented on issue "importaddress failed, Only legacy wallets are supported by this command.":
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2204115545)
Not sure what to do here, other than possibly adjusting the error message https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2029810953 or closing this as a duplicate of https://github.com/bitcoin/bitcoin/issues/30175

💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1663040526)
I think the test needs coverage for:

1) retrying broadcasts
2) sending the same tx again via sendrawtransaction
3) sending a different wtxid via sendrawtransaction
4) sending a tx with in-mempool dependencies (both that succeed and fail due to existing/missing parent)
5) rpc failing if it detects `!g_reachable_nets`

💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1662998546)
s/node1/tx_receiver/

💬 instagibbs commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1662855604)
```Suggestion
ADDRMAN_ADDRESSES = [
```

💬 maflcko commented on pull request "optimization: Moved repeated `-printpriority` fetching out of AddToBlock":
(https://github.com/bitcoin/bitcoin/pull/30324#issuecomment-2204201089)
ACK 323ce303086d42292ed9fe7c98e8b459bdf6d1a6

💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2204283415)
ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#issuecomment-2204306273)
Thank you for the review @ryanofsky!

Updated 7dd92d6fd8f0f52fcfc32739c303122c7e7630ab -> c1d6e525131cb9e54bbedb79ea64e2469a77aed8 ([noGlobalScriptCache_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_12) -> [noGlobalScriptCache_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalScriptCache_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalScriptCache_12..noGlobalScriptCache_13))

* Applied @ryanofsky's [patch](https://github.com/bitcoin/bitcoi
...

💬 achow101 commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1663112431)
In b6cea03b20bfe9486ff8f38a1c40c985d066e72d "test: add test for modififed walletprocesspsbt calls"

Removing `address_type` here and below would allow this test to run on legacy wallets so it does not need the `if self.options.descriptors` below (which would remove the conflict with #28710 and save me a rebase).

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2204312170)
I refactored `TemplateProvider` to move the lower level connection handling code into the newly introduced `Sv2Connman` from #30332.

I noticed a deadlock that was already present in the previous push (f7bf25d12d73471287c8758482014e2a429b4ff7), where if you use Bitcoin QT and start mining, an RPC call (at least to `getblockchaininfo`) will freeze and new blocks won't be submitted.

💬 achow101 commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2204312911)
ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e

💬 maflcko commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2204320590)
> A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.

The policy header include should probably be removed from mempool_options. Any policy settings defaults that can be modified should be included in the corresponding header file itself. That is, the following symbols should be moved to the header in `./kernel/`:

```
./kernel/mempool_limits.h:20:28: error: use of undeclared identifier 'DEFAULT_ANCES
...

💬 achow101 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2204322088)
ACK 8ec24bdad89e2a72c394060ba5661a91f374b874

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663121757)
That's covered in `SanityCheck`.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663124799)
The way the bound was computed was inaccuracy; I have fixed that (256 does get reached (but not exceeded) now in the bench(99) benchmark if you increase the iteration count to 100000). I've also rewritten the comments and computation a bit.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663124892)
I have rewritten this.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663125068)
Gone.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663125681)
I think my in-progress delving post will cover this. I will postpone addressing this until that's out.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1663126806)
Done. I've added an `assert(todo[j]);` before `todo.Reset(j);` to make sure infinite iteration is not possible.