💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144026131)
nit, this would test slightly more
```suggestion
assert(chunk_ptr_remaining == chunk_it->ptr + chunk_it->size); // ensure we are at the end of the chunks
++chunk_it;
assert(chunk_it == chunks.end());
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144072741)
```suggestion
// can't use resource because alignment is too big, allocate system memory
b = resource.Allocate(8, 16);
BOOST_TEST(block == b);
block = b;
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144070292)
nit, and would this make the test too specific? I don't think so, but something to consider.
```suggestion
void* b = resource.Allocate(8, 1);
BOOST_TEST(block == b);
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144079597)
`counts` is unused

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144076687)
```suggestion
// can't use resource because size is too big, allocate system memory
b = resource.Allocate(16, 8);
BOOST_TEST(block != b);
block = b;

```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1144084403)
```suggestion
// set each byte to num_bytes
```

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol dialog":
(https://github.com/bitcoin-core/gui/pull/719#issuecomment-1478817516)
> While you are here, could also align the "bytes" and "change" labels to left. Feel free to squash this: [furszy/bitcoin-core@53165e3](https://github.com/furszy/bitcoin-core/commit/53165e3cc02cf4fdff2b274ad98e1868196e9644)

Could you post a screenshot on how dialogs look before/after this change? On my end this doesn't change anything and the alignment seems to be fine even on master.

💬 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) {
```