💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144188630)
why not? `DataStream` is defined there.

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1144191381)
k, either way is fine for me. Cool about the special case exception removal and the new test.
just updated 27195 with further tests too.

👍 vasild approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK 60c3f4918190900e5f79341abcc0878214656257

💬 vasild commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1144224404)
This is repeated in a few places and can be deduplicated (untested):

```cpp
void PushWarnings(const std::vector<bilingual_str>& warnings, UniValue& to)
{
if (IsDeprecatedRPCEnabled("warningfield")) {
// The "warning" field is deprecated in v25 for removal in v26.
to.pushKV("warning", Join(warnings, Untranslated("\n")).original);
}
if (warnings.empty()) {
return;
}
to.pushKV("warnings", UniValue{UniValue::VARR});
for (const auto& s : w
...

💬 vasild commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1478914811)
> we have authenticated connections ... CJDNS(?)

Yes, in CJDNS the address is (a representation) of the owner's public key.

> there's no relationship between the address and our route?

My understanding is that this is true for Tor, I2P and CJDNS.

💬 jarolrod commented on pull request "Adjust plural forms for translations":
(https://github.com/bitcoin-core/gui/pull/716#issuecomment-1478968364)
concept ack

💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144287702)
Yeah totally, not sure what I was thinking here.
Please resolve this.

💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1479058400)
For Wasabi's purposes this output is needed purely for fee estimation, so it'd be ideal if Core would estimate fees properly. Here's how I see things:

- `estimatesmartfee` estimates fees based on the **PAST**
- This PR enables us to put minimums on Core's fee estimations. Eg. if Core estimates 6 hours at 10 sat/b, based on the past, but the mempool is already filled with 12 hours of 11 sat/b estimations, then what we do (currently with Knots) is that based on the mempool histogram we upgrade
...

💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421842)
I can make `chainname` a struct instead and use the same pattern as it was before in `chainparamsbase`, but I'm not sure if this is so much better. Isn't the default string constructor called anyway for each time this header is included? How about taking this a step further and making them a `const std::string_view` instead? I pushed https://github.com/TheCharlatan/bitcoin/commit/1e4ba77db8121f4bc58653c786595325b308ac31 as a proof of concept. Looking at the diff, I'm not sure if it is worth it t
...

💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421895)
Thank you for this concrete split suggestion. I did not add a scripted diff, to show the change of header and definition namespacing in the same commit. This makes it easier to see that for every changed file the new header is included as well. To me this feels like it will make for a cleaner commit history. However, I'm new to contributing and reviewing large code moves to the project. I'll ponder over your suggested split, I usually come around to these suggestions after a bit :)

💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1144434340)
I realised my mistake. You can resolve this and ignore my comments. I'll go over the code one more time before I ack

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144467948)
Thanks, helpful comments!

- `m_addr_name`: my reasoning for passing `m_addr_name` as string instead of binary is that we likely don't want to implement the binary decoding in each tracing script. I think the string is closer to what users need. However, not opposing to add the binary if there's need.
- `nKeyedNetGroup`: I added the netgroup as argument to better identify connections coming from the same network. For my use-case, it's not problematic if `nKeyedNetGroup` isn't consistent acros
...

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457784)
Can you apply the changes to the other loops too? i.e.
```suggestion
for (size_t put_index{0}; put_index < num_puts; ++put_index) {
```

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144470215)
Wait, if you're using `txns[put_index]` *and* you're using the same `txns` reference each time you call `add_parents_child`, aren't you having all subsequent packages spend outputs from txns[0:24] too? That means all these packages conflict with each other.

You could fix this by adding `set_num * package_size` or something to the index. But what would probably make a better interface is if `add_parents_child` returns the list of transactions it creates, and you append them to your larger list
...

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457060)
Didn't remove?

💬 MarcoFalke commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144475716)
```suggestion
Ticks<std::chrono::seconds>(m_connected);
```

nit, use `Ticks`, or `count_seconds` (if you want the compile failure if m_connected is changed to higher precision than seconds)?

🚀 fanquake merged a pull request: "Refactor: Remove unused FlatFilePos::SetNull"
(https://github.com/bitcoin/bitcoin/pull/27289)

💬 fanquake commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1479227767)
Door open for anyone to followup with more `::SetNull()` removal.

💬 fanquake commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479232890)
> What is the combined log?

Unfortunately don't have one, and cannot seem to recreate this failure. Happy to close until have more useful info.

💬 MarcoFalke commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479235761)
ok. Let's reopen then