💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126832163)
In commit "Split non/kernel chainparams" (4820450688e371a99d66cfad24714fa6e041e43e)

Is calling SetGlobalBaseParams() actually necessary here? I'm guessing something breaks if isn't added but it's not clear what. It'd be good drop the call or change the generic comment above "// SETUP: Misc Globals" into a specific comment about why it's needed currently, and maybe how it can go away in the future.

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126616994)
In commit "Decouple RegTestChainParams from ArgsManager" (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

I think it would be better to change existing `UpdateActivationParametersFromArgs(args)` line to `UpdateActivationParametersFromOptions(options)` and have that function do this stuff, instead of moving more code into this giant constructor, especially when none of the other code in the constructor touches the options struct.

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126829031)
In commit "Split non/kernel chainparams" (4820450688e371a99d66cfad24714fa6e041e43e)

Existing `chainparams` variable is used exactly one place below so would suggest just writing `auto chainparams = CChainParams::Main();` and using it directly instead of having two similar variables.

💬 dougEfresh commented on pull request "doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2":
(https://github.com/bitcoin/bitcoin/pull/26968#issuecomment-1456693331)
@stickies-v I agree with your change, have pushed it to this PR.
Thanks

👍 hernanmarino approved a pull request: "Mask values on Transactions View"
(https://github.com/bitcoin-core/gui/pull/708)
Tested ACK 5fb3face477c30f576e14541550fcf64b17a94d6

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126878745)
Yeah, the rename was meant to be in the first commit. Thanks

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1456759368)
Moved rename of `GatherClusters(…)` to the right commit. Looking into the crash of the fuzzer.

💬 stickies-v commented on pull request "doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2":
(https://github.com/bitcoin/bitcoin/pull/26968#issuecomment-1456783035)
I think ci is failing because you didn't fully apply my commit:

<details>
<summary>git diff 3280c03 3ecb9458c05ac869bc2d77ea983bc85baf9baa07 -- src/rpc/rawtransaction.cpp</summary>

```diff
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 21d49fda9..986e7c0cc 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -219,7 +219,7 @@ static RPCHelpMan getrawtransaction()
{RPCResult::Type::OBJ, "", "utxo bein
...

👍 john-moffett approved a pull request: "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk"
(https://github.com/bitcoin/bitcoin/pull/27189)
ACK fa1b4e5c3294fc9aec033892a4a4d7b5cfc015aa

Good conceptual change, as there's no reason for (eg) an NTP adjustment to affect these things.

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126719573)
nit: this can just be `cluster.empty()`

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126727129)
Could add above this line an early return:
```c++
if (txids.size > 500) return {};
```

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126716115)
nit:
```c++
const txiter& tx_iter = clustered_txs.at(i);
for (const auto& entries : {tx_iter->GetMemPoolParentsConst(), tx_iter->GetMemPoolChildrenConst()}) {
for (const CTxMemPoolEntry& entry : entries) {
const auto entry_it = mapTx.iterator_to(entry);
if (!visited(entry_it)) {
clustered_txs.push_back(entry_it);
++to_process_count; // we still need to process this
}
}
}
```

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126736075)
'outpoints' naming shadows the function's arg. Could
```c++
for (const auto& [txid, _] : m_requested_outpoints_by_txid) {
```

💬 S3RK commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456850195)
ReACK 6a302d4
Thanks for addressing feedback

📝 amitiuttarwar opened a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213)
This is joint work with mzumsande.

The current logic that makes automatic outbound connections doesn’t try to diversify with respect to reachable networks - the decision to connect to a particular address from `AddrMan` is based purely on the frequency of available addresses.

For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're cap
...

📝 amitiuttarwar opened a pull request: "addrman: Enable selecting addresses by network "
(https://github.com/bitcoin/bitcoin/pull/27214)
For the full context & motivation of this patch, see #27213

This is joint work with mzumsande.

This PR adds functionality to `AddrMan::Select` to enable callers to specify a network they are interested in.

Along the way, it refactors the function to deduplicate the logic, updates the local variables to match modern conventions, adds test coverage for both the new and existing `Select` logic, and adds bench tests for the worst case performance of both the new and existing `Select` logic.
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1456867760)
We have 3 open questions that we are interested in hearing feedback from reviewers. We have considered many potential interactions and have laid out a couple specific ones along with tradeoffs of different approaches.

### IBD
The first is regarding the interactions between this patch & IBD. Peers on privacy network might be slower than clearnet peers (eg. Tor or I2P). Our current proposal does not have any special case logic for disabling the network prioritization during IBD.

This coul
...

💬 brunoerg commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1456947717)
@theStack have in mind re-ACK it?

📝 achow101 opened a pull request: "wallet: Turn `destdata` entries into `CAddressBookData` fields"
(https://github.com/bitcoin/bitcoin/pull/27215)
`destdata` is a generic strings map in `CAddressBookEntry`. It is opaque and thus hard to know what is actually being stored in a `CAddressBookEntry`. It is a map with fixed fields, but these are not documented nor are they immediately obvious when looking at the class definition. This PR changes those to be member variables inside of `CAddressBookEntry` to make it easier to understand what kind of data is being stored.

👍 theStack approved a pull request: "wallet: group outputs only once, decouple it from Coin Selection"
(https://github.com/bitcoin/bitcoin/pull/25806)
re-ACK 6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44 :coconut:

(verified via `git range-diff 0bb90cab...6a302d40` that since my last ACK, most review comments by Murch and S3RK have been tackled, mostly trivial refactor + terminology/naming improvements and [a minor unit test adaption](https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126088720))